core/assert: check assert at compile time, if possible

[maribu]

Contribution description

If value of the expression expr in assert(expr) is know at compile time, this abused the optimizer and linker to fail compilation if the expression is not true. It falls back to runtime checking if the value is not known at compile time.

[!WARNING} This will break the common pattern to add an assert(0); to label code that should be unreachable, as clearly the value of the literal 0 is known at compile time and never true.

For this reason, assert_unreachable() is added that can be used instead. A semantic patch has been added, so that the CI will ask contributors to replace assert(0) with assert_unreachable().

Testing procedure

This should compile fine:

diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a0..134d88683e 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,13 @@
  */
 
 #include <stdio.h>
+#include <string.h>
+
+#include "assert.h"
+
+static inline int return_fourty_two(void) {
+    return 42;
+}
 
 int main(void)
 {
@@ -28,5 +35,7 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);
 
+    assert(return_fourty_two() == 42);
+
     return 0;
 }

This should fail to compile:

diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a0..fffd81f710 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,13 @@
  */
 
 #include <stdio.h>
+#include <string.h>
+
+#include "assert.h"
+
+static inline int return_fourty_two(void) {
+    return 42;
+}
 
 int main(void)
 {
@@ -28,5 +35,7 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);
 
+    assert(return_fourty_two() != 42);
+
     return 0;
 }

Issues/PRs references

• [x] Depends on and includes https://github.com/RIOT-OS/RIOT/pull/17273
• Inspired by discussion in https://github.com/RIOT-OS/RIOT/pull/17342

[kaspar030]

This seems extremely useful!

Can we somehow condense assert, static_assert, magic_assert and magic_static_assert?

[maribu]

I'm not so sure. assert() and magic_assert() are too distinct to handle both the same way (e.g. placing an assert(0) at unreachable code paths is sensible, doing so with magic_assert(0) is not).

magic_static_assert() could replace static_assert(), but the letter has better diagnostics and fails already at the compilation step (not at the linking step). I hope that constexpr gets part of the C language and we can just get rid of magic_static_assert().

[benpicco]

I like the idea, but I don't like the name - how about dynamic_assert or const_assert?

[maribu]

I like the idea, but I don't like the name - how about dynamic_assert or const_assert?

I chose magic because I wanted the function to be upfront about what it does.

Maybe about dark_magic_assert() or abuse_linker_assert()?

Btw: If we would add a special assert_unreachable(void) and have a semantic patch insisting on assert(0) being replaced by that, we could just make the regular assert() bail out at compile time, if the compiler can proof the assert() to blow at compile time.

[Teufelchen1]

Maybe about dark_magic_assert() or abuse_linker_assert()?

I'm not a fan of magic_assert() but these suggestions are worse imo. Let's stick with magic_assert, maybe not perfect but easy to search for in our codebase and with the newly added comment also easy to understand. Not sure about dynamic_/const_assert. I could imagine people mistaking them to be compiler build-ins or something? magic is more telling "we made this".

[Teufelchen1]

Reminder: Soft-freeze is soon!

[riot-ci]

Murdock results

:x: FAILED

68b233b3daab776c5cb5a5f42249381ccda653ae treewide: use assert_unreachable() were needed

Success	Failures	Total	Runtime
622	1	9095	01m:59s

Build failures (1)

Application	Target	Toolchain	Runtime (s)	Worker
tests/net/nanocoap_cli	native	llvm	6.82	mobi3

Artifacts

[benpicco]

For this reason, assert_unreachable() is added that can be used instead. A semantic patch has been added, so that the CI will ask contributors to replace assert(0) with assert_unreachable().

This is fine with in-tree code, but what about external code and packages? assert(0) in an unreachable branch is a pretty common pattern.

[maribu]

This is fine with in-tree code, but what about external code and packages? assert(0) in an unreachable branch is a pretty common pattern.

I changed the header so that when included from a package, it will only check at runtime. Hence, assert(0); will work there.