Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 13, 2021

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

Sorry, something went wrong.

@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: core Area: RIOT kernel. Handle PRs marked with this with care! Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Dec 13, 2021
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 13, 2021
@kaspar030
Copy link
Contributor

This seems extremely useful!

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

@maribu
Copy link
Member Author

maribu commented Dec 14, 2021

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().

@maribu maribu closed this May 27, 2022
@maribu maribu deleted the core/assert branch May 27, 2022 09:08
@maribu maribu restored the core/assert branch May 30, 2022 08:30
@maribu maribu reopened this May 30, 2022
@github-actions github-actions bot added Area: drivers Area: Device drivers and removed Area: tests Area: tests and testing framework labels May 30, 2022
@benpicco
Copy link
Contributor

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

core/lib/include/assert.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Jan 24, 2024
@maribu
Copy link
Member Author

maribu commented Jan 24, 2024

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
Copy link
Contributor

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
Copy link
Contributor

Reminder: Soft-freeze is soon!

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: network Area: Networking Area: tests Area: tests and testing framework Area: pkg Area: External package ports Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: BLE Area: Bluetooth Low Energy support Area: arduino API Area: Arduino wrapper API Area: boards Area: Board ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: USB Area: Universal Serial Bus Area: sys Area: System labels Mar 28, 2024
@maribu maribu changed the title core/assert: add magic_assert() and magic_static_assert() core/assert: check assert at compile time, if possible Mar 28, 2024
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 28, 2024
@riot-ci
Copy link

riot-ci commented Mar 28, 2024

Murdock results

FAILED

68b233b 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
Copy link
Contributor

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 maribu force-pushed the core/assert branch 2 times, most recently from 4a7ee85 to 54cd48a Compare March 28, 2024 16:08
@github-actions github-actions bot added Area: tools Area: Supplementary tools Area: LoRa Area: LoRa radio support labels Mar 28, 2024
maribu and others added 2 commits March 28, 2024 18:03

Verified

This commit was signed with the committer’s verified signature. The key has expired.
maribu Marian Buschsieweke
This allows header to not export RIOT specific headers to external code.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
maribu Marian Buschsieweke
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.

**Beware**: 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()`.
@github-actions github-actions bot added the Area: build system Area: Build system label Mar 28, 2024

Verified

This commit was signed with the committer’s verified signature. The key has expired.
maribu Marian Buschsieweke
Replace all instances where an `assert(0)` / `assert(false)` / ...
was used to check for code being unreachable with
`assert_unreachable()`.
@maribu
Copy link
Member Author

maribu commented Mar 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: arduino API Area: Arduino wrapper API Area: BLE Area: Bluetooth Low Energy support Area: boards Area: Board ports Area: build system Area: Build system Area: CoAP Area: Constrained Application Protocol implementations Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: AVR Platform: This PR/issue effects AVR-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform Process: missing approvals Integration Process: PR needs more ACKS (handled by action) Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants