core/lib: make the use of DEBUG_BREAKPOINT on assert optional

[gschorcht]

Contribution description

This PR makes the use of DEBUG_BREAKPOINT on failed assertion optional.

The behavior of assert has been changed with PR #19368. Instead of printing some useful information, either a breakpoint is inserted and the execution of the MCU stops in debugger or a endless while loop is executed.

Before PR #19368 the user got on failed assertion:

Starting ESP32x with ID: 7cdfa1e36a34
ESP-IDF SDK Version v4.4.1-0-g1329b19fe49
...
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

This was very helpful during development, especially to identify quickly the cause of problems with DEBUG_ASSERT_VERBOSE enabled, e.g. when misconfiguration led to failed assertions.

With PR #19368 the user gets an address in best case (or even 0 on platforms like ESP32), in worst case the MCU seems to stuck, e.g.

Starting ESP32x with ID: 7cdfa1e36a34
ESP-IDF SDK Version v4.4.1-0-g1329b19fe49
...
0

The problem with the new behavior is that

• a user doesn't get a quick indication of what happened
• there is not always an easy way to attach a debugger

This PR therefore makes the use of DEBUG_BREAKPOINT optional using DEBUG_ASSERT_BREAKPOINT define.

Testing procedure

Add assert(0) in examples/hello-world/main.c and compile with and w/o CFLAGS='-DDEBUG_ASSERT_BREAKPOINT'.

With DEBUG_ASSERT_BREAKPOINT the execution should stop in assert_failue. Without DEBUG_ASSERT_BREAKPOINT, the information as generated before PR #19368 and the execution should stop in panic_arch.

Issues/PRs references

[riot-ci]

Murdock results

✔️ PASSED

6eb358b tests/sys/sema_inv: boards with insufficient RAM

Success	Failures	Total	Runtime
6930	0	6930	12m:17s

Artifacts

• Documentation preview

[maribu]

Makes sense. I think the reason this got in was that behavior on a break instruction when no debugger is attached is platform dependent. Some just treat the break instruction as a nop without a debugger attached, which would be fine here

[maribu]

bors merge

[bors]

Build failed:

• Murdock

[maribu]

The CI failure is quite unexpected and interesting

[gschorcht]

The CI failure is quite unexpected and interesting

Compilation works with DEBUG_ASSERT_BREAKPOINT defined. So I guess that the compiler optimizes out something in assert so that the RAM is sufficient.

No wonder, you removed Atmega328p boards in Makefile.ci with 7e58bea 😉

[maribu]

No wonder, you removed Atmega328p boards in Makefile.ci with 7e58bea 😉

That one is not related. The commit allowing it to compile there was storing string literals (the first argument to printf() to be nore precise) in flash rather than in RAM for AVR, which requires some extra effort to do so.

[maribu]

Ahh, AVR doesn't implement ARCHITECTURE_BREAKPOINT() and the fallback is a spinning endless loop. That yields some optimization potential.

[gschorcht]

That one is not related. The commit allowing it to compile there was storing string literals (the first argument to printf() to be nore precise) in flash rather than in RAM for AVR,

OK 🤔 then the RAM usage has been increased after PR #19368 but we didn't notice that because it was previously reduced by PR #19368:

                   data   bss    total
Before PR #19368   212    1835   2047
After PR #19368    194    1835   2029

That is, without PR #19368 there was only one byte left 😎 With PR #19368 we had 19 Bytes left, but now we have only 7 bytes left, too less to reenable assert output 😟

                   data   bss    total
Current master     194	  1847   2041

[maribu]

Sorry if I made the impression that bumping Makefile.ci is an issue to me. It is not.

I just was confused as to why it was needed and thought that this may trigger (but not cause) an unrelated issue. But now that the reason is obvious, I can sleep well again.

[gschorcht]

That one is not related. The commit allowing it to compile there was storing string literals (the first argument to printf() to be nore precise) in flash rather than in RAM for AVR, which requires some extra effort to do so.

Indeed, reducing the string in this call reduces the RAM usage

RIOT/core/lib/assert.c

Line 35 in b209fdd

core_panic(PANIC_ASSERT_FAIL, "FAILED ASSERTION.");

[maribu]

bors retry

[bors]

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[gschorcht]

bors merge

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Murdock
• static-tests
• tools-build-success

[maribu]

Thx for the fix :)