Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: [TEST] Kill the dead_code macro ......................................................................
[TEST] Kill the dead_code macro
It broke master quite spectacularly not too long ago.
Change-Id: I06f825e045d3305dd8ab81311fd2d003d03ae43a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/assert.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/string.c M src/mainboard/google/gru/board.h M src/security/vboot/misc.h M src/soc/intel/braswell/pmutil.c 7 files changed, 31 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40141/1
diff --git a/src/include/assert.h b/src/include/assert.h index 990cee1..9476acd 100644 --- a/src/include/assert.h +++ b/src/include/assert.h @@ -53,30 +53,4 @@
#define assert(statement) ASSERT(statement)
-/* - * These macros can be used to assert that a certain branch of code is dead and - * will be compile-time eliminated. This differs from _Static_assert(), which - * will generate a compiler error even if the scope it was called from is dead - * code. This may be useful to double-check things like constants that are only - * valid if a certain Kconfig option is set. - * - * 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' - */ -#define __dead_code(line) do { \ - extern void dead_code_assertion_failed_at_line_##line(void) \ - __attribute__((noreturn)); \ - dead_code_assertion_failed_at_line_##line(); \ -} while (0) -#define _dead_code(line) __dead_code(line) -#define dead_code() _dead_code(__LINE__) - -/* This can be used in the context of an expression of type 'type'. */ -#define dead_code_t(type) ({ \ - dead_code(); \ - *(type *)(uintptr_t)0; \ -}) - #endif // __ASSERT_H__ diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index f82b154..0d7456e 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -20,17 +20,18 @@ gfx_init_done = done; }
+/* Must always select MUST_REQUEST_DISPLAY when using this function. */ +#if CONFIG(VBOOT) && !CONFIG(VBOOT_MUST_REQUEST_DISPLAY) +#error "VBOOT selected, but VBOOT_MUST_REQUEST_DISPLAY was not selected!" +#endif + int display_init_required(void) { - /* For vboot, always honor VB2_CONTEXT_DISPLAY_INIT. */ - if (CONFIG(VBOOT)) { - /* Must always select MUST_REQUEST_DISPLAY when using this - function. */ - if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) - dead_code(); - return vboot_get_context()->flags & VB2_CONTEXT_DISPLAY_INIT; + if (!CONFIG(VBOOT)) { + /* By default always initialize display. */ + return 1; }
- /* By default always initialize display. */ - return 1; + /* For vboot, always honor VB2_CONTEXT_DISPLAY_INIT. */ + return vboot_get_context()->flags & VB2_CONTEXT_DISPLAY_INIT; } diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 13b5483..5152a74 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -17,6 +17,7 @@
static struct imd imd;
+#if ENV_ROMSTAGE || ENV_POSTCAR || ENV_RAMSTAGE void *cbmem_top(void) { if (ENV_ROMSTAGE) { @@ -28,9 +29,8 @@ } if (ENV_POSTCAR || ENV_RAMSTAGE) return (void *)_cbmem_top_ptr; - - dead_code(); } +#endif
static inline const struct cbmem_entry *imd_to_cbmem(const struct imd_entry *e) { diff --git a/src/lib/string.c b/src/lib/string.c index f0c24ed..eedcd5a 100644 --- a/src/lib/string.c +++ b/src/lib/string.c @@ -5,11 +5,9 @@ #include <stddef.h> #include <stdlib.h>
+#if ENV_RAMSTAGE char *strdup(const char *s) { - if (!ENV_RAMSTAGE) - dead_code(); /* This can't be used without malloc(). */ - size_t sz = strlen(s) + 1; char *d = malloc(sz); if (d) @@ -19,9 +17,6 @@
char *strconcat(const char *s1, const char *s2) { - if (!ENV_RAMSTAGE) - dead_code(); /* This can't be used without malloc(). */ - size_t sz_1 = strlen(s1); size_t sz_2 = strlen(s2); char *d = malloc(sz_1 + sz_2 + 1); @@ -31,6 +26,7 @@ } return d; } +#endif
size_t strnlen(const char *src, size_t max) { diff --git a/src/mainboard/google/gru/board.h b/src/mainboard/google/gru/board.h index c29aa51..7efaab5 100644 --- a/src/mainboard/google/gru/board.h +++ b/src/mainboard/google/gru/board.h @@ -19,6 +19,9 @@ #include <assert.h> #include <gpio.h>
+/* This will never be declared, so unless the compiler optimizes it away, the build will fail */ +extern gpio_t this_gpio_is_not_applicable(void); + #define GPIO_POWEROFF GPIO(1, A, 6) #define GPIO_RESET GPIO(0, B, 3) #define GPIO_SDMMC_PWR GPIO(4, D, 5) @@ -28,16 +31,16 @@ #define GPIO_BACKLIGHT GPIO(4, C, 6) #define GPIO_EC_IN_RW GPIO(0, A, 1) #define GPIO_EC_IRQ GPIO(1, C, 2) -#define GPIO_P15V_EN dead_code_t(gpio_t) -#define GPIO_P18V_AUDIO_PWREN dead_code_t(gpio_t) +#define GPIO_P15V_EN this_gpio_is_not_applicable() +#define GPIO_P18V_AUDIO_PWREN this_gpio_is_not_applicable() #define GPIO_P30V_EN GPIO(0, B, 1) #define GPIO_SPK_PA_EN GPIO(0, A, 2) -#define GPIO_TP_RST_L dead_code_t(gpio_t) +#define GPIO_TP_RST_L this_gpio_is_not_applicable() #define GPIO_TPM_IRQ GPIO(1, C, 1) #define GPIO_WP GPIO(0, B, 5) #else #define GPIO_BL_EN GPIO(1, C, 1) -#define GPIO_BACKLIGHT dead_code_t(gpio_t) +#define GPIO_BACKLIGHT this_gpio_is_not_applicable() #define GPIO_EC_IN_RW GPIO(3, B, 0) #define GPIO_EC_IRQ GPIO(0, A, 1) #define GPIO_P15V_EN GPIO(0, B, 2) @@ -52,7 +55,7 @@ #if CONFIG(GRU_HAS_WLAN_RESET) #define GPIO_WLAN_RST_L GPIO(1, B, 3) #else -#define GPIO_WLAN_RST_L dead_code_t(gpio_t) +#define GPIO_WLAN_RST_L this_gpio_is_not_applicable() #endif
void setup_chromeos_gpios(void); diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index fd422b2..8463b74 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -37,6 +37,11 @@ */ int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw);
+/* Check that we are not trying to build something with invalid settings */ +#if CONFIG(VBOOT) && !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) && !CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) +#error "vboot must start somewhere" +#endif + /* * The stage loading code is compiled and entered from multiple stages. The * helper functions below attempt to provide more clarity on when certain @@ -52,9 +57,7 @@ return ENV_ROMSTAGE; else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) return ENV_BOOTBLOCK; - else - dead_code(); -} +} /* Should never be reached, at least one of the checks should be true */
static inline int verstage_should_load(void) { @@ -81,10 +84,7 @@ } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { /* Post-RAM stages are "after the romstage" */ return !ENV_ROMSTAGE_OR_BEFORE; - } else { - dead_code(); } -} - +} /* Should never be reached, at least one of the checks should be true */
#endif /* __VBOOT_MISC_H__ */ diff --git a/src/soc/intel/braswell/pmutil.c b/src/soc/intel/braswell/pmutil.c index 016c45c..db740f7 100644 --- a/src/soc/intel/braswell/pmutil.c +++ b/src/soc/intel/braswell/pmutil.c @@ -357,15 +357,12 @@ write32((void *)(PMC_BASE_ADDRESS + PRSTS), prsts); }
+#if !ENV_RAMSTAGE int rtc_failure(void) { uint32_t gen_pmcon1; int rtc_fail;
- /* not usable in ramstage as GEN_PMCON1 gets reset */ - if (ENV_RAMSTAGE) - dead_code(); - gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
rtc_fail = !!(gen_pmcon1 & RPS); @@ -374,6 +371,7 @@
return rtc_fail; } +#endif
int vbnv_cmos_failed(void) {
Hello build bot (Jenkins), Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40141
to look at the new patch set (#2).
Change subject: [TEST] Kill the dead_code macro ......................................................................
[TEST] Kill the dead_code macro
It broke master quite spectacularly not too long ago.
Change-Id: I06f825e045d3305dd8ab81311fd2d003d03ae43a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/assert.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/string.c M src/mainboard/google/gru/board.h M src/security/vboot/misc.h M src/soc/intel/braswell/pmutil.c 7 files changed, 34 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40141/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: [TEST] Kill the dead_code macro ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40141/2/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/40141/2/src/lib/string.c@a11 PS2, Line 11: /* This can't be used without malloc(). */ Keep this comment
https://review.coreboot.org/c/coreboot/+/40141/2/src/soc/intel/braswell/pmut... File src/soc/intel/braswell/pmutil.c:
https://review.coreboot.org/c/coreboot/+/40141/2/src/soc/intel/braswell/pmut... PS2, Line 365: /* not usable in ramstage as GEN_PMCON1 gets reset */ Keep this comment
Hello build bot (Jenkins), Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40141
to look at the new patch set (#3).
Change subject: [TEST] Kill the dead_code macro ......................................................................
[TEST] Kill the dead_code macro
It broke master quite spectacularly not too long ago.
Change-Id: I06f825e045d3305dd8ab81311fd2d003d03ae43a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/assert.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/string.c M src/mainboard/google/gru/board.h M src/security/vboot/misc.h M src/soc/intel/braswell/pmutil.c 7 files changed, 36 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40141/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: [TEST] Kill the dead_code macro ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40141/2/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/40141/2/src/lib/string.c@a11 PS2, Line 11: /* This can't be used without malloc(). */
Keep this comment
Done
https://review.coreboot.org/c/coreboot/+/40141/2/src/soc/intel/braswell/pmut... File src/soc/intel/braswell/pmutil.c:
https://review.coreboot.org/c/coreboot/+/40141/2/src/soc/intel/braswell/pmut... PS2, Line 365: /* not usable in ramstage as GEN_PMCON1 gets reset */
Keep this comment
Done
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40141
to look at the new patch set (#4).
Change subject: Kill the dead_code macro ......................................................................
Kill the dead_code macro
It broke master quite spectacularly not too long ago.
Change-Id: I06f825e045d3305dd8ab81311fd2d003d03ae43a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/assert.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/string.c M src/mainboard/google/gru/board.h M src/security/vboot/misc.h M src/soc/intel/braswell/pmutil.c 7 files changed, 36 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40141/4
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40141
to look at the new patch set (#5).
Change subject: Kill the dead_code macro ......................................................................
Kill the dead_code macro
It broke master quite spectacularly not too long ago. Off it goes!
Change-Id: I06f825e045d3305dd8ab81311fd2d003d03ae43a Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/include/assert.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/string.c M src/mainboard/google/gru/board.h M src/security/vboot/misc.h M src/soc/intel/braswell/pmutil.c 7 files changed, 36 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/40141/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40141/5/src/lib/bootmode.c File src/lib/bootmode.c:
https://review.coreboot.org/c/coreboot/+/40141/5/src/lib/bootmode.c@37 PS5, Line 37: /* Must always select MUST_REQUEST_DISPLAY when using this function. */ : return you_need_to_select_vboot_must_request_display(); i'd put this in an else block and not just after the if/else if
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
Would be nice to use __builtin_unreachable
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40141/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40141/5//COMMIT_MSG@9 PS5, Line 9: quite spectacularly Actually rather boring compared to coreboot's history.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40141/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40141/5//COMMIT_MSG@9 PS5, Line 9: broke What was the exact issue? This commit description doesn't provide an informative explanation for the change.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5: Code-Review-1
Uhh... context? I rather like my macro and would prefer to keep it unless there are real issues with it that cannot be solved otherwise. Can you explain how it "broke master quite spectacularly"?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
Uhh... context? I rather like my macro and would prefer to keep it unless there are real issues with it that cannot be solved otherwise. Can you explain how it "broke master quite spectacularly"?
see https://qa.coreboot.org/job/coreboot-gerrit/121768/testReport/junit/(root)/b... for the breakage it caused
the non-functional spdx license header changes caused some line numbers to shift which resulted in the same symbol being generated twice by the macro, which made the build fail. so yes, i don't like that macro too much. Maybe add a __COUNTER__ instead of the tag added in https://review.coreboot.org/c/coreboot/+/40140 as a hotfix, that made the tree build again?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
Okay, I assume this is about CB:40140. Looking into it now. Sorry that this happened on a Saturday so I wasn't there to help fix it, and thanks to those who were. But I would still prefer we talk about it and try to find other solutions rather than cutting a feature due to a one-time bug. When a patch merges cleanly but results in a broken tree, that should normally be considered a problem with the CI, not with the code itself.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
But I would still prefer we talk about it and try to find other solutions
if you have a better idea to make the macro not cause issues like that, i'm all ears. but if it's not guaranteed to be safe and not cause issues like this again, i'd like to see it removed.
rather than cutting a feature due to a one-time bug.
The bug happened once by now, but a non-functional change that basically only slightly shifts line numbers should never ever result in a build failure. this is why i'd say that the macro is buggy
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
if you have a better idea to make the macro not cause issues like that, i'm all ears. but if it's not guaranteed to be safe and not cause issues like this again, i'd like to see it removed.
Here, I think this should fix this the issue permanently: CB:40237
The bug happened once by now, but a non-functional change that basically only slightly shifts line numbers should never ever result in a build failure. this is why i'd say that the macro is buggy
Yeah I'm not saying it isn't, but let's just fix it rather than remove it immediately?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5: Code-Review-1
Yeah I'm not saying it isn't, but let's just fix it rather than remove it immediately?
That's one reason why the hotfix was fast-path merged and not removal. also because the hotfix was less invasive
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Patch Set 5:
This was a product of frustration, and as such, it is pretty bad. I know. I think CB:40240 is much better than this.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40141 )
Change subject: Kill the dead_code macro ......................................................................
Abandoned
Superseded by CB:40240
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)