Hello Zheng Bao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45893
to review the following change.
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
amdfwtool: Fix the gcc warning about sign comparison
New (maybe) compile tool complains the warning below. warning: comparison between signed and unsigned integer expressions [-Wsign-compare] Fix all of them.
Change-Id: I59624326233284e6c3595df49625563254949c45 Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/45893/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index f385068..e6341a7 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -646,7 +646,7 @@ return -2; }
- if (fd_stat.st_size > room) { + if ((size_t)fd_stat.st_size > room) { printf("Error: %s will not fit. Exiting.\n", src_file); close(fd); return -3; @@ -667,7 +667,7 @@ amd_fw_entry *fw_table) { ssize_t bytes; - int i; + uint32_t i;
ctx->current += sizeof(embedded_firmware); ctx->current = ALIGN(ctx->current, BLOB_ALIGNMENT); @@ -841,7 +841,7 @@ static int locate_bdt2_bios(bios_directory_table *level2, uint64_t *source, uint32_t *size) { - int i; + uint32_t i;
*source = 0; *size = 0; @@ -1154,7 +1154,7 @@
static void register_fw_fuse(char *str) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { if (amd_psp_fw_table[i].type != AMD_PSP_FUSE_CHAIN) @@ -1167,7 +1167,7 @@
static void register_fw_token_unlock(void) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { if (amd_psp_fw_table[i].type != AMD_TOKEN_UNLOCK) @@ -1202,7 +1202,7 @@
static void register_bdt_data(amd_bios_type type, int sub, int ins, char name[]) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_bios_table) / sizeof(amd_bios_entry); i++) { if (amd_bios_table[i].type == type @@ -1217,7 +1217,7 @@ static void register_fw_addr(amd_bios_type type, char *src_str, char *dst_str, char *size_str) { - int i; + uint32_t i; for (i = 0; i < sizeof(amd_bios_table) / sizeof(amd_bios_entry); i++) { if (amd_bios_table[i].type != type) continue;
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45893 )
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
Patch Set 1: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45893 )
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
Patch Set 1: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45893 )
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
amdfwtool: Fix the gcc warning about sign comparison
New (maybe) compile tool complains the warning below. warning: comparison between signed and unsigned integer expressions [-Wsign-compare] Fix all of them.
Change-Id: I59624326233284e6c3595df49625563254949c45 Signed-off-by: Zheng Bao fishbaozi@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45893 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M util/amdfwtool/amdfwtool.c 1 file changed, 7 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Felix Held: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index f385068..e6341a7 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -646,7 +646,7 @@ return -2; }
- if (fd_stat.st_size > room) { + if ((size_t)fd_stat.st_size > room) { printf("Error: %s will not fit. Exiting.\n", src_file); close(fd); return -3; @@ -667,7 +667,7 @@ amd_fw_entry *fw_table) { ssize_t bytes; - int i; + uint32_t i;
ctx->current += sizeof(embedded_firmware); ctx->current = ALIGN(ctx->current, BLOB_ALIGNMENT); @@ -841,7 +841,7 @@ static int locate_bdt2_bios(bios_directory_table *level2, uint64_t *source, uint32_t *size) { - int i; + uint32_t i;
*source = 0; *size = 0; @@ -1154,7 +1154,7 @@
static void register_fw_fuse(char *str) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { if (amd_psp_fw_table[i].type != AMD_PSP_FUSE_CHAIN) @@ -1167,7 +1167,7 @@
static void register_fw_token_unlock(void) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_psp_fw_table) / sizeof(amd_fw_entry); i++) { if (amd_psp_fw_table[i].type != AMD_TOKEN_UNLOCK) @@ -1202,7 +1202,7 @@
static void register_bdt_data(amd_bios_type type, int sub, int ins, char name[]) { - int i; + uint32_t i;
for (i = 0; i < sizeof(amd_bios_table) / sizeof(amd_bios_entry); i++) { if (amd_bios_table[i].type == type @@ -1217,7 +1217,7 @@ static void register_fw_addr(amd_bios_type type, char *src_str, char *dst_str, char *size_str) { - int i; + uint32_t i; for (i = 0; i < sizeof(amd_bios_table) / sizeof(amd_bios_entry); i++) { if (amd_bios_table[i].type != type) continue;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45893 )
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45893/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/45893/2/util/amdfwtool/amdfwtool.c@... PS2, Line 670: uint32_t i; What is the reason to limit this to 32 bit, and not to take the default length: unsigned int or size_t?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45893 )
Change subject: amdfwtool: Fix the gcc warning about sign comparison ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45893/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/45893/2/util/amdfwtool/amdfwtool.c@... PS2, Line 670: uint32_t i;
What is the reason to limit this to 32 bit, and not to take the default length: unsigned int or size […]
it doesn't need to be an explicit 32 bit type, but since the fw_table will certainly have less than 2^32-1 entries, it shouldn't matter either. but yeah, using unsigned int instead wouldn't hurt and might make that slightly clearer
-
Bao Zheng (Code Review) -
Felix Held (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)