Change in coreboot[master]: assert: Use __COUNTER__ instead of tag or line number in dead_code()
Hello Nico Huber, Angel Pons, Felix Held,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40237
to review the following change.
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
assert: Use __COUNTER__ instead of tag or line number in dead_code()
CB:40140 added a 'tag' parameter to dead_code() so that it can be made unique in cases where the line number alone isn't enough. The disadvantage with this is that tags have to be supplied manually. This patch removes both the tag and the line number (which isn't really necessary for readability since the compiler error messages are already good enough to follow on their own) and uses the __COUNTER__ macro instead, which is guaranteed to yield a different number on every invocation.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia0837365b0f3a0b1ce599e10e95676b9867982e1 --- M src/include/assert.h M src/security/vboot/misc.h 2 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/40237/1
diff --git a/src/include/assert.h b/src/include/assert.h index 492629d..53f9df4 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -52,15 +52,15 @@ * The error message when this hits will look like this: * * ramstage/lib/bootmode.o: In function `display_init_required': - * bootmode.c:42: undefined reference to `dead_code_assertion_failed_at_line_42' + * bootmode.c:42: undefined reference to `dead_code_assertion_failed_1' */ -#define __dead_code(tag, line) do { \ - extern void dead_code_assertion_failed##tag##_at_line_##line(void) \ +#define __dead_code(ctr) do { \ + extern void dead_code_assertion_failed_##ctr(void) \ __attribute__((noreturn)); \ - dead_code_assertion_failed##tag##_at_line_##line(); \ + dead_code_assertion_failed_##ctr(); \ } while (0) -#define _dead_code(tag, line) __dead_code(tag, line) -#define dead_code(tag) _dead_code(tag, __LINE__) +#define _dead_code(ctr) __dead_code(ctr) +#define dead_code() _dead_code(__COUNTER__)
/* This can be used in the context of an expression of type 'type'. */ #define dead_code_t(type) ({ \ diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 22cc750..fd422b2 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -53,7 +53,7 @@ else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) return ENV_BOOTBLOCK; else - dead_code(_in_vboot_misc_h); + dead_code(); }
static inline int verstage_should_load(void) @@ -82,7 +82,7 @@ /* Post-RAM stages are "after the romstage" */ return !ENV_ROMSTAGE_OR_BEFORE; } else { - dead_code(_in_vboot_misc_h); + dead_code(); } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40237/1/src/include/assert.h File src/include/assert.h:
https://review.coreboot.org/c/coreboot/+/40237/1/src/include/assert.h@58 PS1, Line 58: extern void dead_code_assertion_failed_##ctr(void) \ function definition argument 'void' should also have an identifier name
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Patch Set 1: Code-Review+1
looks good to me, but i'm definitely no preprocessor macro magician, so only a +1 from me
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Patch Set 1:
Huh? must have missed something. Last time I checked the linker didn't spat the line number... at least I was sure two days ago. If we don't need the line number in the identifier, we can just use a global declaration, right? I've pushed a test, CB:40240.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Patch Set 1:
If we don't need the line number in the identifier, we can just use a global declaration, right? I've pushed a test, CB:40240.
Oh... yes, I guess that's true. Let's take your patch then.
Huh? must have missed something. Last time I checked the linker didn't spat the line number... at least I was sure two days ago.
This is how it looks for me (using crossgcc):
/home/jwerner/jenkins/util/crossgcc/xgcc/bin/aarch64-elf-ld.bfd: coreboot-builds/GOOGLE_BOB/bootblock/security/vboot/vboot_loader.o: in function `verification_should_run': /home/jwerner/jenkins/src/security/vboot/misc.h:49: undefined reference to `_dead_code_assertion_failed'
Julius Werner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Abandoned
CB:40240 is better
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40237 )
Change subject: assert: Use __COUNTER__ instead of tag or line number in dead_code() ......................................................................
Patch Set 1:
Huh? must have missed something. Last time I checked the linker didn't spat the line number... at least I was sure two days ago.
It was my host linker ._. coreboot's provides much nicer error messages.
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)