assert(0) doesnt cause trap without GDB and breaks execution path

[romansavrulin]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [ESP-12]
• Core Version: [2.4.0]
• Development Env: [Arduino IDE]
• Operating System: [Windows]

Settings in IDE

• Module: [Nodemcu]

Problem Description

assert(0) doesn't cause neither trap, nor backtrace, all instructions after that call gets optimized-out, breaking execution logic.

Because assert(0) returns, MCU continues code execution with broken logic and can cause fault later on, that is very hard to catch.

MCVE Sketch

#include <ESP8266WiFi.h>
#include "assert.h"
/*********************** Global vars ***********************/
char *buffer;
const char *watermark = "ZZZZ";
char *wmBuffer = NULL;
unsigned int capacity;
/*********************** Global vars ***********************/

unsigned char changeBuffer(unsigned int maxStrLen);


void setup() {
    Serial.begin(115200);
    Serial.setDebugOutput(true);
}

void loop() {
    delay(400);

    changeBuffer(14);       //random value
}



unsigned char changeBuffer(unsigned int maxStrLen){
    auto watermarkLen = strlen(watermark);

    size_t newSize = (uint32_t)(maxStrLen + 16 + watermarkLen * 2 + 2) & (~0xf);
    int s = newSize;
    ets_printf("\nnew size %d  || ", newSize);

    char *newbuffer = (char *) realloc(wmBuffer, newSize);

    if (!newbuffer) {
        ets_printf("gotcha! %d %d\n", newSize, watermarkLen);
        return 0;
    }

    if(newbuffer) {
        assert(0); //<-- expected to trap
        size_t oldSize = capacity + 1; // include NULL.
    wmBuffer = newbuffer;

    memcpy(newbuffer + newSize - watermarkLen,  watermark, strlen(watermark));

        capacity = newSize - 1 - watermarkLen;

        ets_printf(" 1 ");
        ets_printf(" 2 ");
        buffer = newbuffer + watermarkLen;
        ets_printf(" 3 ");
        //assert(0);

        ets_printf("leaving one");
        return 1;
    }
    ets_printf("leaving zero");
    return 0; 
}

Debug Messages

Debug messages go here

$ xtensa-lx106-elf-objdump.exe -d bug_catcher.ino.elf | sed '/<_Z12changeBufferj>:/,/^$/!d'

40202098 <_Z12changeBufferj>:
40202098:       f0c112          addi    a1, a1, -16
4020209b:       21c9            s32i.n  a12, a1, 8
4020209d:       02cd            mov.n   a12, a2
4020209f:       fff721          l32r    a2, 4020207c <setup+0x2c>
402020a2:       3109            s32i.n  a0, a1, 12
402020a4:       0228            l32i.n  a2, a2, 0
402020a6:       11d9            s32i.n  a13, a1, 4
402020a8:       01e9            s32i.n  a14, a1, 0
402020aa:       12ccc2          addi    a12, a12, 18
402020ad:       01de85          call0   40203e98 <strlen>
402020b0:       90c2c0          addx2   a12, a2, a12
402020b3:       02ed            mov.n   a14, a2
402020b5:       027c            movi.n  a2, -16
402020b7:       10cc20          and     a12, a12, a2
402020ba:       fff121          l32r    a2, 40202080 <setup+0x30>
402020bd:       0c3d            mov.n   a3, a12
402020bf:       fc1801          l32r    a0, 40201120 <ets_puts_P+0xac>
402020c2:       0000c0          callx0  a0
402020c5:       ffef21          l32r    a2, 40202084 <setup+0x34>
402020c8:       0c3d            mov.n   a3, a12
402020ca:       0228            l32i.n  a2, a2, 0
402020cc:       ff6f01          l32r    a0, 40201e88 <_free_r+0x14>
402020cf:       0000c0          callx0  a0
402020d2:       02dd            mov.n   a13, a2
402020d4:       c28c            beqz.n  a2, 402020e4 <_Z12changeBufferj+0x4c>
402020d6:       ffed21          l32r    a2, 4020208c <setup+0x3c>
402020d9:       ffed41          l32r    a4, 40202090 <setup+0x40>
402020dc:       ffee51          l32r    a5, 40202094 <setup+0x44>
402020df:       932c            movi.n  a3, 41
402020e1:       ff1b85          call0   4020129c <__assert_func>   

/// <-- no if(newbuffer) branch at all after that, but assert returns and we get unexpected logic.

402020e4:       ffe921          l32r    a2, 40202088 <setup+0x38>
402020e7:       0c3d            mov.n   a3, a12
402020e9:       0e4d            mov.n   a4, a14
402020eb:       fc0d01          l32r    a0, 40201120 <ets_puts_P+0xac>
402020ee:       0000c0          callx0  a0
402020f1:       3108            l32i.n  a0, a1, 12
402020f3:       0d2d            mov.n   a2, a13
402020f5:       21c8            l32i.n  a12, a1, 8
402020f7:       11d8            l32i.n  a13, a1, 4
402020f9:       01e8            l32i.n  a14, a1, 0
402020fb:       10c112          addi    a1, a1, 16
402020fe:       f00d            ret.n

[earlephilhower]

assert(0) will always fail, and assert_fcn never returns, by definition. All assert.h defintions I've ever seen have __attribute__ ((__noreturn__));. That means the compiler can assume anything after it in the control block never gets executed.

I'm not seeing any issue or problem with the behavior you're showing here. Maybe you meant to put an assert(1), which will always be true and be compiled into a no-op?

[romansavrulin]

@earlephilhower I know what you are talking about. but the problem is that assert(0) returns. We've spent a day catching it and can stand for it. Please run the sketch and assure yourself.

[romansavrulin]

to be clear

Arduino/cores/esp8266/core_esp8266_postmortem.c

Lines 205 to 215 in a662e81

	void __assert_func(const char *file, int line, const char *func, const char *what) {
	(void) what;
	s_panic_file = file;
	s_panic_line = line;
	s_panic_func = func;
	gdb_do_break();
	}
	void __panic_func(const char* file, int line, const char* func) {
	s_panic_file = file;
	s_panic_line = line;

if gdb_do_break is default weak function it will return

[romansavrulin]

because it boils down to

Arduino/cores/esp8266/core_esp8266_main.cpp

Line 143 in 8053f28

extern "C" void __gdb_do_break(){}

[earlephilhower]

Got it, I misunderstood the problem you were seeing. And you beat me to the code link by 30 seconds. :)

I could swear I saw assertions hang the machine in a prior release, will need to check the relevant histories. I added some code in #4187 which I remember seeing dump the assertion text and halt, because w/o a panic() those stored bits aren't actually dumped to the serial port. Odd.

[romansavrulin]

@earlephilhower For us, it seems that conditional assertions won't optimize anything from original function, but will also return and cause damage later on, because of the main problem that assertion should trap. But we didnt checked that case for sure yet.

[igrr]

This seems to be a bug in my original implementation of __assert_func. I'll send a fix.

[earlephilhower]

Shouldn't we just add an abort() to the end of _assert_func() (esp. since we call is "noreturn")? I can throw the PR in, but not sure now since it seems like you've a different idea...

[igrr]

Pretty much, yes, plus i also threw in some cleanup.

[igrr]

With changes in #4482, the reproducer correctly panics:

new size 32  || 
Panic /Users/ivan/Documents/Arduino/b4480_assert/b4480_assert.ino:41 unsigned char changeBuffer(unsigned int)

ctx: cont 
sp: 3ffefa80 end: 3ffefc80 offset: 01b0

>>>stack>>>
3ffefc30:  3ffe8860 00000020 000000ff 3ffeec54  
3ffefc40:  3fffdad0 00000004 00000020 4020231c  
3ffefc50:  3fffdad0 00000000 3ffeec4c 4020234a  
3ffefc60:  feefeffe feefeffe 3ffeec4c 40202e68  
3ffefc70:  feefeffe feefeffe 3ffeec60 40100a09  
<<<stack<<<

[devyte]

setting milestone to 2.4.2