core/assert: halt running thread instead of panic by benpicco · Pull Request #20627 · RIOT-OS/RIOT

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 "backtrace.h"
 #endif

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


Architecture.h

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

core/lib/assert.c

-__NORETURN void _assert_failure(const char *file, unsigned line)
+__NORETURN static inline void _assert_common(void)
 {
-    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

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


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

core/lib/assert.c

    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 MrKevinWeiss and chrysn 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

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

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

mguetschow added this to the Release 2024.07 milestone Jul 5, 2024

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

Conversation

benpicco commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

riot-ci commented Apr 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Apr 28, 2024

Uh oh!

Tanyajackson638 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu commented Apr 29, 2024

Uh oh!

benpicco commented Apr 29, 2024

Uh oh!

maribu commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benpicco commented Apr 26, 2024 •

edited

Loading

riot-ci commented Apr 27, 2024 •

edited

Loading