Ryan O'Leary has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Fix a handful of compiler warnings
These warnings (which I believe are not enabled by default) were generated by -Wformat-security and -Wenum-conversion.
Change-Id: I3d01f4ff620f4c4111dd675d49ce3254f2360b29 Signed-off-by: Ryan O'Leary ryanoleary@google.com --- M libflashrom.c M print.c M programmer.h 3 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/36946/1
diff --git a/libflashrom.c b/libflashrom.c index 0dec22e..b15b5f6 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -188,7 +188,8 @@ for (; i < boards_known_size; ++i) { supported_boards[i].vendor = binfo[i].vendor; supported_boards[i].name = binfo[i].name; - supported_boards[i].working = binfo[i].working; + supported_boards[i].working = + (enum flashrom_test_state)binfo[i].working; } } else { msg_gerr("Memory allocation error!\n"); @@ -226,8 +227,9 @@ supported_chipsets[i].chipset = chipset[i].device_name; supported_chipsets[i].vendor_id = chipset[i].vendor_id; supported_chipsets[i].chipset_id = chipset[i].device_id; - supported_chipsets[i].status = chipset[i].status; - } + supported_chipsets[i].status = + (enum flashrom_test_state)chipset[i].status; + } } else { msg_gerr("Memory allocation error!\n"); } diff --git a/print.c b/print.c index 596fc53..173c7da 100644 --- a/print.c +++ b/print.c @@ -401,7 +401,7 @@ for (i = 0; i < maxboardlen - strlen(b->name); i++) msg_ginfo(" ");
- msg_pinfo(test_state_to_text(b->working)); + msg_pinfo("%s", test_state_to_text(b->working));
for (e = board_matches; e->vendor_name != NULL; e++) { if (strcmp(e->vendor_name, b->vendor) diff --git a/programmer.h b/programmer.h index 5a21b2e..96add05 100644 --- a/programmer.h +++ b/programmer.h @@ -307,7 +307,7 @@
/* cbtable.c */ int cb_parse_table(const char **vendor, const char **model); -int cb_check_image(const uint8_t *bios, unsigned int size); +int cb_check_image(const uint8_t *image, unsigned int size);
/* dmi.c */ #if defined(__i386__) || defined(__x86_64__)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 1:
(1 comment)
Thanks for taking care of this. Please, whenever fixing warnings that aren't enabled yet, also add a commit on top that enables them :)
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c@192 PS1, Line 192: (enum flashrom_test_state)binfo[i].working; IMHO, it would be better to use `enum flashrom_test_state` everywhere. That we have competing enums seems like a bug that would only be covered up by the cast.
Hello ron minnich, Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/36946
to look at the new patch set (#2).
Change subject: Fix a handful of compiler warnings ......................................................................
Fix a handful of compiler warnings
These warnings were generated by -Wformat-security (supported by both clang and gcc) and -Wenum-conversion (clang only) and -Wmissing-braces (which is more strick in clang).
Change-Id: I3d01f4ff620f4c4111dd675d49ce3254f2360b29 Signed-off-by: Ryan O'Leary ryanoleary@google.com --- M Makefile M flash.h M libflashrom.c M libflashrom.h M print.c M print_wiki.c M programmer.h M rayer_spi.c M util/ich_descriptors_tool/ich_descriptors_tool.c 9 files changed, 29 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/36946/2
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
(1 comment)
Thanks for taking care of this. Please, whenever fixing warnings that aren't enabled yet, also add a commit on top that enables them :)
Thanks! I found out the environment I was building under had set CC=clang which explains the difference in warnings. I've double checked there are no warnings under gcc or clang.
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c@192 PS1, Line 192: (enum flashrom_test_state)binfo[i].working;
IMHO, it would be better to use `enum flashrom_test_state` everywhere. That […]
Done. I added some defines for OK, BAD, etc.., otherwise it would be very verbose.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/flashrom/+/36946/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/36946/2//COMMIT_MSG@11 PS2, Line 11: strick strict?
But anyway, IIRC, this was fixed lately in Clang...
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/36946/1/libflashrom.c@192 PS1, Line 192: (enum flashrom_test_state)binfo[i].working;
Done. I added some defines for OK, BAD, etc.., otherwise it would be very verbose.
Looks good :)
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... PS2, Line 155: struct ich_descriptors desc = { 0 }; We just changed it to this to calm compiler warnings ;) I assume Clang complained about this? what version? IIRC, they changed this lately for better compatibility.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 6:
(2 comments)
The first fix seems very questionable to me. The inventors of the C language moved to using enums as used in flash.h decades ago. Ref: many parts of Plan 9.
https://review.coreboot.org/c/flashrom/+/36946/6/flash.h File flash.h:
https://review.coreboot.org/c/flashrom/+/36946/6/flash.h@146 PS6, Line 146: #define OK FLASHROM_TESTED_OK What C compiler mandated this? It's poor practice. The people who invented C and the preprocessor, changed their thinking in the 1980s and recommended the use of enums, and even anonymous enums. I'm shocked that a modern C compiler would drive a change like this.
https://review.coreboot.org/c/flashrom/+/36946/6/util/ich_descriptors_tool/i... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/36946/6/util/ich_descriptors_tool/i... PS6, Line 155: struct ich_descriptors desc = { }; if it needs to be zero, and it is an auto, just put an explicit memset. That said, if it needs to be 0, since this is main(), just make it a static to get the same effect. It's main.
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/flashrom/+/36946/6/flash.h File flash.h:
https://review.coreboot.org/c/flashrom/+/36946/6/flash.h@146 PS6, Line 146: #define OK FLASHROM_TESTED_OK
What C compiler mandated this? It's poor practice. […]
The problem is we have two identical enums flashrom_test_state and test_state. The compiler warned because there were implicit casts between them.
The solution is to remove test_state in favor of flashrom_test_state. These defines alias FLAHSROM_TESTED_X to X, which is different from defining new enum values.
If X was renamed to FLASHROM_TEStED_X in all places, the code would become very verbose. These values are used in 1000s of places and this might add around an additional 100chars to some lines.
Hello ron minnich, Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/36946
to look at the new patch set (#7).
Change subject: Fix a handful of compiler warnings ......................................................................
Fix a handful of compiler warnings
These warnings were generated by -Wformat-security (supported by both clang and gcc), -Wenum-conversion (clang only) and -Wmissing-braces (which is more strict in clang).
Change-Id: I3d01f4ff620f4c4111dd675d49ce3254f2360b29 Signed-off-by: Ryan O'Leary ryanoleary@google.com --- M Makefile M flash.h M libflashrom.c M libflashrom.h M print.c M print_wiki.c M programmer.h M rayer_spi.c M util/ich_descriptors_tool/ich_descriptors_tool.c 9 files changed, 29 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/36946/7
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... PS2, Line 155: struct ich_descriptors desc = { 0 };
It looks like gcc and clang support a feature (possibly an extension to the language) where you use […]
Ron suggested using static which works well.
https://review.coreboot.org/c/flashrom/+/36946/6/util/ich_descriptors_tool/i... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/36946/6/util/ich_descriptors_tool/i... PS6, Line 155: struct ich_descriptors desc = { };
if it needs to be zero, and it is an auto, just put an explicit memset. […]
Thanks! static worked well.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
Sorry, I feel like I have to make a stand here. This whole story (including earlier patches) is getting incredibly ridiculous. There are nice warning options in todays compilers, but if a warning option sucks and makes us write worse code, then we shouldn't use it. I've wasted so much time discussing such options, that I could have spend reviewing and fixing actual bugs instead. IMHO, some warning options just make things worse.
If we feel like we need to write /* zero initialized */ because we do that implicitly and rely on that, while the language actually offers a nice and clean way to do that explicitly, then we failed, desperately. If we want a nice Clang experience, please let's just disable that ridiculous warning.
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... File util/ich_descriptors_tool/ich_descriptors_tool.c:
https://review.coreboot.org/c/flashrom/+/36946/2/util/ich_descriptors_tool/i... PS2, Line 155: struct ich_descriptors desc = { 0 };
Ron suggested using static which works well.
Well, here is how much I remember of the story:
* empty braces {} are not allowed in C * { 0 } is valid C for any aggregate even if it starts with a sub-aggregate (the 0 will just fill the first field of the first sub recursively, all other fields will be filled like static objects) * GCC folks made {} and { 0 } exceptions to -Wmissing-field-initializers, otherwise nobody would use that option because people would get mad, writing all the 0s. * Clang folks had a patch to make { 0 } an exception too, maybe they rolled it back, idk.
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 7:
Patch Set 7: Code-Review-1
(1 comment)
Sorry, I feel like I have to make a stand here. This whole story (including earlier patches) is getting incredibly ridiculous. There are nice warning options in todays compilers, but if a warning option sucks and makes us write worse code, then we shouldn't use it. I've wasted so much time discussing such options, that I could have spend reviewing and fixing actual bugs instead. IMHO, some warning options just make things worse.
If we feel like we need to write /* zero initialized */ because we do that implicitly and rely on that, while the language actually offers a nice and clean way to do that explicitly, then we failed, desperately. If we want a nice Clang experience, please let's just disable that ridiculous warning.
Sounds reasonable. These warnings were more ridiculous than I expected.
Ryan O'Leary has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Abandoned
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 7:
Sorry, I feel like I have to make a stand here. This whole story (including earlier patches) is getting incredibly ridiculous. There are nice warning options in todays compilers, but if a warning option sucks and makes us write worse code, then we shouldn't use it. I've wasted so much time discussing such options, that I could have spend reviewing and fixing actual bugs instead. IMHO, some warning options just make things worse.
If we feel like we need to write /* zero initialized */ because we do that implicitly and rely on that, while the language actually offers a nice and clean way to do that explicitly, then we failed, desperately. If we want a nice Clang experience, please let's just disable that ridiculous warning.
Sounds reasonable. These warnings were more ridiculous than I expected.
Sorry, I didn't mean all the warning options. My rant was only about -Wmissing-field-initializers, it's just not useful with Clang, IMHO.
The other options seem reasonable. Format, enum, they actually spotted regressions in the code, so I'd agree to enable them.
Ryan O'Leary has restored this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Restored
Hello ron minnich, Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/36946
to look at the new patch set (#8).
Change subject: Fix a handful of compiler warnings ......................................................................
Fix a handful of compiler warnings
These warnings were generated by -Wformat-security (supported by both clang and gcc) and -Wenum-conversion (clang only).
In order to fix the implicit enum casts, enum test_state has been removed in favor of flashrom_test_state.
Change-Id: I3d01f4ff620f4c4111dd675d49ce3254f2360b29 Signed-off-by: Ryan O'Leary ryanoleary@google.com --- M Makefile M flash.h M libflashrom.c M libflashrom.h M print.c M print_wiki.c M programmer.h M rayer_spi.c 8 files changed, 28 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/36946/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/36946/7/Makefile File Makefile:
https://review.coreboot.org/c/flashrom/+/36946/7/Makefile@33 PS7, Line 33: -Wmissing-field-initializers For GCC, this is already part of -Wextra. But I guess we should disable it for Clang, right? i.e. += -Wno-missing-field-initializers. Can be done in a separate patch, though.
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/flashrom/+/36946/8/Makefile File Makefile:
https://review.coreboot.org/c/flashrom/+/36946/8/Makefile@40 PS8, Line 40: ifeq ($(CC),clang) In my experience this can be dropped, as there now should not be enum conversion warnings anymore. Tested on FreeBSD 13-CURRENT with gcc9 and clang 9.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/flashrom/+/36946/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/36946/8//COMMIT_MSG@10 PS8, Line 10: -Wenum-conversion (clang only) GCC warns about that too.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 9: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36946 )
Change subject: Fix a handful of compiler warnings ......................................................................
Patch Set 9:
Needs a rebase
-
Angel Pons (Code Review) -
Idwer Vollering (Code Review)
-
Lean Sheng Tan (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
ron minnich (Code Review)
-
Ryan O'Leary (Code Review)