Hello Shelley Chen, Furquan Shaikh, Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44044
to review the following change.
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
assert.h: Try to evaluate assertions at compile time
Many places in coreboot seem to like to do things like
assert(CONFIG(SOME_KCONFIG));
This is somewhat suboptimal since assert() is a runtime check, so you don't see that this fails until someone actually tries to boot it even though the compiler is totally aware of it already. We already have the dead_code() macro to do this better:
if (CONFIG(SOME_KCONFIG)) dead_code();
Rather than fixing all these and trying to carefully educate people about which type of check is more appropriate in what situation, we can just employ the magic of __builtin_constant_p() to automatically make the former statement behave like the latter.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I06691b732598eb2a847a17167a1cb92149710916 --- M src/include/assert.h 1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/44044/1
diff --git a/src/include/assert.h b/src/include/assert.h index 0463175..d9883fd 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -22,9 +22,12 @@ #define __ASSERT_LINE__ __LINE__ #endif
+#define __build_time_assert(x) (__builtin_constant_p(x) ? \ + ((x) ? 1 : dead_code_t(int)) : 0) + /* GCC and CAR versions */ #define ASSERT(x) { \ - if (!(x)) { \ + if (!__build_time_assert(x) && !(x)) { \ printk(BIOS_EMERG, \ "ASSERTION ERROR: file '%s', line %d\n", \ __ASSERT_FILE__, __ASSERT_LINE__); \ @@ -33,7 +36,7 @@ } \ } #define ASSERT_MSG(x, msg) { \ - if (!(x)) { \ + if (!__build_time_assert(x) && !(x)) { \ printk(BIOS_EMERG, \ "ASSERTION ERROR: file '%s', line %d\n", \ __ASSERT_FILE__, __ASSERT_LINE__); \
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44044
to look at the new patch set (#3).
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
assert.h: Try to evaluate assertions at compile time
Many places in coreboot seem to like to do things like
assert(CONFIG(SOME_KCONFIG));
This is somewhat suboptimal since assert() is a runtime check, so you don't see that this fails until someone actually tries to boot it even though the compiler is totally aware of it already. We already have the dead_code() macro to do this better:
if (CONFIG(SOME_KCONFIG)) dead_code();
Rather than fixing all these and trying to carefully educate people about which type of check is more appropriate in what situation, we can just employ the magic of __builtin_constant_p() to automatically make the former statement behave like the latter.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I06691b732598eb2a847a17167a1cb92149710916 --- M src/include/assert.h 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/44044/3
Hello Shelley Chen, build bot (Jenkins), Furquan Shaikh, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44044
to look at the new patch set (#4).
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
assert.h: Try to evaluate assertions at compile time
Many places in coreboot seem to like to do things like
assert(CONFIG(SOME_KCONFIG));
This is somewhat suboptimal since assert() is a runtime check, so you don't see that this fails until someone actually tries to boot it even though the compiler is totally aware of it already. We already have the dead_code() macro to do this better:
if (CONFIG(SOME_KCONFIG)) dead_code();
Rather than fixing all these and trying to carefully educate people about which type of check is more appropriate in what situation, we can just employ the magic of __builtin_constant_p() to automatically make the former statement behave like the latter.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I06691b732598eb2a847a17167a1cb92149710916 --- M src/include/assert.h 1 file changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/44044/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44044 )
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44044 )
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44044 )
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44044 )
Change subject: assert.h: Try to evaluate assertions at compile time ......................................................................
assert.h: Try to evaluate assertions at compile time
Many places in coreboot seem to like to do things like
assert(CONFIG(SOME_KCONFIG));
This is somewhat suboptimal since assert() is a runtime check, so you don't see that this fails until someone actually tries to boot it even though the compiler is totally aware of it already. We already have the dead_code() macro to do this better:
if (CONFIG(SOME_KCONFIG)) dead_code();
Rather than fixing all these and trying to carefully educate people about which type of check is more appropriate in what situation, we can just employ the magic of __builtin_constant_p() to automatically make the former statement behave like the latter.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I06691b732598eb2a847a17167a1cb92149710916 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44044 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Furquan Shaikh furquan@google.com --- M src/include/assert.h 1 file changed, 9 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/include/assert.h b/src/include/assert.h index 0463175..262b8cc 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -22,9 +22,16 @@ #define __ASSERT_LINE__ __LINE__ #endif
+#ifndef _PORTING_H_ /* TODO: Isolate AGESA properly. */ +#define __build_time_assert(x) \ + (__builtin_constant_p(x) ? ((x) ? 1 : dead_code_t(int)) : 0) +#else +#define __build_time_assert(x) 0 +#endif + /* GCC and CAR versions */ #define ASSERT(x) { \ - if (!(x)) { \ + if (!__build_time_assert(x) && !(x)) { \ printk(BIOS_EMERG, \ "ASSERTION ERROR: file '%s', line %d\n", \ __ASSERT_FILE__, __ASSERT_LINE__); \ @@ -33,7 +40,7 @@ } \ } #define ASSERT_MSG(x, msg) { \ - if (!(x)) { \ + if (!__build_time_assert(x) && !(x)) { \ printk(BIOS_EMERG, \ "ASSERTION ERROR: file '%s', line %d\n", \ __ASSERT_FILE__, __ASSERT_LINE__); \
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)