core/assert: halt running thread instead of panic #20627

benpicco · 2024-04-26T12:17:45Z

Contribution description

Instead of taking down the whole system, just kill the running thread.

Testing procedure

Add an assert(0) somewhere.

Issues/PRs references

fixes #20626

maribu · 2024-04-26T13:07:30Z

core/lib/include/assert.h

+ * If the assertion failed in an interrupt, the system will still panic.
+ */
+#ifndef DEBUG_ASSERT_ZOMBIFY
+#define DEBUG_ASSERT_ZOMBIFY (1)


Suggested change

#define DEBUG_ASSERT_ZOMBIFY (1)

# define DEBUG_ASSERT_ZOMBIFY (0)

Maybe not change the default to avoid the API change discussion here to get this feature in quickly, and change the default in a follow up PR and let the discussion unfold there?

We have a pattern of including useful features that are just not used because people don't know they exist, or don't turn them on manually all the time (eg. MOST_RECENT_PORT=1), I'd rather not make this one of them.

If there is any pushback on the API change and this makes it faster, fine. If not, let's not hurry to comply with non-requirements.

OK, then let's quickly merge this before there is a large discussion 👿

core/lib/assert.c

riot-ci · 2024-04-27T20:18:56Z

Murdock results

✔️ PASSED

a28e98b examples/dtls-wolfssl: add maple-mini to Makefile.ci

Success	Failures	Total	Runtime
10059	0	10063	14m:58s

Artifacts

Documentation preview

chrysn

Thanks, LGTM.

Tested using this change

diff --git a/examples/gcoap/server.c b/examples/gcoap/server.c
index bf2315cd01..e10885a6b6 100644
--- a/examples/gcoap/server.c
+++ b/examples/gcoap/server.c
@@ -151,2 +151,4 @@ static ssize_t _riot_board_handler(coap_pkt_t *pdu, uint8_t *buf, size_t len, co
 
+    assert(0);
+
     /* write the RIOT board name in the response buffer */

and by CoAP requesting .well-known/core (works), /riot/board (causes the panic), and while CoAP is down, the shell still works.

I've pushed a check for a missing include (irq.h, failed to build on native), please confirm and squash.

maribu · 2024-04-28T06:51:25Z

Looks like some Makefile.cis are in need of bumping

Tanyajackson638

core/lib/include/assert.h
core/lib/assert.c

Tanyajackson638 · 2024-04-29T03:32:29Z

core/lib/assert.c

 #include "panic.h"
 #if IS_USED(MODULE_BACKTRACE)
 #include "backtrace.h"
 #endif

-__NORETURN void _assert_failure(const char *file, unsigned line)


Architecture.h

Tanyajackson638 · 2024-04-29T03:42:04Z

core/lib/assert.c

 {
-    printf("%s:%u => ", file, line);


Tanyajackson638 · 2024-04-29T03:42:41Z

core/lib/assert.c

 __NORETURN void _assert_panic(void)
 {
    printf("%" PRIxTXTPTR "\n", cpu_get_caller_pc());
-#if IS_USED(MODULE_BACKTRACE)


Tanyajackson638 · 2024-04-29T03:44:59Z

core/lib/assert.c

 __NORETURN void _assert_panic(void)
 {
    printf("%" PRIxTXTPTR "\n", cpu_get_caller_pc());
-#if IS_USED(MODULE_BACKTRACE)
-    backtrace_print();


Tanyajackson638 · 2024-04-29T03:45:38Z

core/lib/assert.c

 __NORETURN void _assert_panic(void)
 {
    printf("%" PRIxTXTPTR "\n", cpu_get_caller_pc());
-#if IS_USED(MODULE_BACKTRACE)
-    backtrace_print();
-#endif


Tanyajackson638 · 2024-04-29T03:46:05Z

core/lib/assert.c

-#if IS_USED(MODULE_BACKTRACE)
-    backtrace_print();
-#endif
-#ifdef DEBUG_ASSERT_BREAKPOINT


Module backtrace

Tanyajackson638 · 2024-04-29T03:46:48Z

core/lib/assert.c

-    backtrace_print();
-#endif
-#ifdef DEBUG_ASSERT_BREAKPOINT
-    DEBUG_BREAKPOINT(1);


Backtrace.h

Tanyajackson638 · 2024-04-29T03:47:23Z

core/lib/assert.c

-#endif
-#ifdef DEBUG_ASSERT_BREAKPOINT
-    DEBUG_BREAKPOINT(1);
-#endif


Tanyajackson638 · 2024-04-29T03:49:31Z

core/lib/assert.c

-#ifdef DEBUG_ASSERT_BREAKPOINT
-    DEBUG_BREAKPOINT(1);
-#endif
-    core_panic(PANIC_ASSERT_FAIL, "FAILED ASSERTION.");


maribu · 2024-04-29T06:12:13Z

core/lib/include/assert.h
core/lib/assert.c

@Tanyajackson638 Could you please explain with a few words what you want to say regarding every single comment above? I think in most cases it coule be something like "missing include of ...", but in some of the cases this does not make sense.

Just to be sure: You are a human and not a bot, right?

benpicco · 2024-04-29T09:02:33Z

Weird that DEBUG_ASSERT_NO_PANIC adds 116 bytes of text

maribu · 2024-04-30T10:20:01Z

@Tanyajackson638 Just to be sure: We were unable to understand your review. (To be honest it left me so puzzled I wouldn't be surprised if your review would be the result of a bot/script behaving unexpectedly.)

If you do have any concerns or questions regarding this PR, please voice them here and we will try our best to address them.

core/assert: move common code to inline function

3359f26

benpicco requested a review from kaspar030 as a code owner April 26, 2024 12:17

github-actions bot added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Apr 26, 2024

benpicco requested review from chrysn and MrKevinWeiss April 26, 2024 12:23

maribu reviewed Apr 26, 2024

View reviewed changes

maribu approved these changes Apr 26, 2024

View reviewed changes

maribu enabled auto-merge April 26, 2024 13:31

maribu added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Apr 26, 2024

github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Apr 26, 2024

chrysn reviewed Apr 26, 2024

View reviewed changes

core/lib/assert.c Outdated Show resolved Hide resolved

chrysn reviewed Apr 26, 2024

View reviewed changes

core/lib/assert.c Outdated Show resolved Hide resolved

benpicco force-pushed the assert-zombi branch from 2dca654 to 08b6435 Compare April 26, 2024 14:47

benpicco changed the title ~~core/assert: zombify running thread instead of panic~~ core/assert: halt running thread instead of panic Apr 26, 2024

benpicco force-pushed the assert-zombi branch from 08b6435 to 7827146 Compare April 26, 2024 15:04

chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 27, 2024

chrysn approved these changes Apr 27, 2024

View reviewed changes

core/assert: halt running thread instead of panic

731ada0

benpicco force-pushed the assert-zombi branch from 45a25bb to 731ada0 Compare April 27, 2024 23:43

github-actions bot removed the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Apr 27, 2024

maribu added this pull request to the merge queue Apr 27, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 28, 2024

Tanyajackson638 mentioned this pull request Apr 29, 2024

### Murdock results Tanyajackson638/hello-world#24

Open

Tanyajackson638 reviewed Apr 29, 2024

View reviewed changes

examples/dtls-wolfssl: add maple-mini to Makefile.ci

a28e98b

github-actions bot added the Area: examples Area: Example Applications label Apr 29, 2024

benpicco enabled auto-merge April 29, 2024 09:02

benpicco added this pull request to the merge queue Apr 29, 2024

Merged via the queue into RIOT-OS:master with commit 6c15495 Apr 29, 2024
25 checks passed

benpicco deleted the assert-zombi branch April 29, 2024 12:33

benpicco requested a review from kfessel May 6, 2024 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/assert: halt running thread instead of panic #20627

core/assert: halt running thread instead of panic #20627

benpicco commented Apr 26, 2024 •

edited

maribu Apr 26, 2024

chrysn Apr 26, 2024

maribu Apr 26, 2024

riot-ci commented Apr 27, 2024 •

edited

chrysn left a comment

maribu commented Apr 28, 2024

Tanyajackson638 left a comment

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

Tanyajackson638 Apr 29, 2024

maribu commented Apr 29, 2024

benpicco commented Apr 29, 2024

maribu commented Apr 30, 2024

	#define DEBUG_ASSERT_ZOMBIFY (1)
	# define DEBUG_ASSERT_ZOMBIFY (0)

core/assert: halt running thread instead of panic #20627

core/assert: halt running thread instead of panic #20627

Conversation

benpicco commented Apr 26, 2024 • edited

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Apr 27, 2024 • edited

Murdock results

Artifacts

chrysn left a comment

Choose a reason for hiding this comment

maribu commented Apr 28, 2024

Tanyajackson638 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu commented Apr 29, 2024

benpicco commented Apr 29, 2024

maribu commented Apr 30, 2024

benpicco commented Apr 26, 2024 •

edited

riot-ci commented Apr 27, 2024 •

edited