Change in coreboot[master]: util/supermicro: Always include commonlib/bsd/compiler.h

[Nico Huber (Code Review)]

Hello Michael Niewöhner, Patrick Rudolph,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/41784

to review the following change.

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

util/supermicro: Always include commonlib/bsd/compiler.h

We rely on `compiler.h` for definitions like `__packed`. Without it,
`smcbiosinfo.c` simply declared a global struct with that name, but
nothing was packed.

Change-Id: Ide055317115fc374a63812bcd3791445ca4f2dcc
Signed-off-by: Nico Huber 
---
M util/supermicro/Makefile.inc
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41784/1

diff --git a/util/supermicro/Makefile.inc b/util/supermicro/Makefile.inc
index 1aa5bcb..316cb48 100644
--- a/util/supermicro/Makefile.inc
+++ b/util/supermicro/Makefile.inc
@@ -1,9 +1,11 @@
+TOOLCPPFLAGS += -include $(top)/src/commonlib/bsd/include/commonlib/bsd/compiler.h
+
 SMCBIOSINFOTOOL:= $(objutil)/supermicro/smcbiosinfo

$(SMCBIOSINFOTOOL): $(dir)/smcbiosinfo/smcbiosinfo.c
    printf "    HOSTCC     Creating SMCBIOSINFO tool\n"
    mkdir -p $(objutil)/supermicro
-	$(HOSTCC) $< -o $@
+	$(HOSTCC) $(TOOLCPPFLAGS) $< -o $@

ifeq ($(CONFIG_VENDOR_SUPERMICRO),y)
 ifneq ($(call strip_quotes, $(CONFIG_SUPERMICRO_BOARDID)),)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41784/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41784/1//COMMIT_MSG@11 
PS1, Line 11: nothing was packed.
Maybe add:
> Found-by: BUILD_TIMELESS was not reproducible.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1:

From what I have seen in reversed vendor bmc binaries this should be fine and it shouldn't even matter that the binary is shorter now.

Still needs a test to be 100% sure.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1:
> Patch Set 1:
>
> From what I have seen in reversed vendor bmc binaries this should be fine and it shouldn't even matter that the binary is shorter now.
>
> Still needs a test to be 100% sure.

Um, that's not how binary structures work...

It was probably only working by chance before. The struct layout places uint16_t and uint32_t in naturally-aligned (in this case, 4-byte aligned) offsets, so only the last uint64_t array would require padding.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 1: Code-Review+2

[Nico Huber (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41784

to look at the new patch set (#2).

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

util/supermicro: Always include commonlib/bsd/compiler.h

We rely on `compiler.h` for definitions like `__packed`. Without it,
`smcbiosinfo.c` simply declared a global struct with that name, but
nothing was packed.

Found-by: reproducibility test

Change-Id: Ide055317115fc374a63812bcd3791445ca4f2dcc
Signed-off-by: Nico Huber 
---
M util/supermicro/Makefile.inc
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41784/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

Patch Set 2:

(1 comment)

Does somebody want to test this?

https://review.coreboot.org/c/coreboot/+/41784/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41784/1//COMMIT_MSG@11 
PS1, Line 11: nothing was packed.
> Maybe add: […]

Idk if that's actually what was tested. But I'll add something about reproducibility.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41784 )

Change subject: util/supermicro: Always include commonlib/bsd/compiler.h
......................................................................

util/supermicro: Always include commonlib/bsd/compiler.h

We rely on `compiler.h` for definitions like `__packed`. Without it,
`smcbiosinfo.c` simply declared a global struct with that name, but
nothing was packed.

Found-by: reproducibility test

Change-Id: Ide055317115fc374a63812bcd3791445ca4f2dcc
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41784
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: Michael Niewöhner 
Reviewed-by: Angel Pons 
---
M util/supermicro/Makefile.inc
1 file changed, 3 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/util/supermicro/Makefile.inc b/util/supermicro/Makefile.inc
index 1aa5bcb..316cb48 100644
--- a/util/supermicro/Makefile.inc
+++ b/util/supermicro/Makefile.inc
@@ -1,9 +1,11 @@
+TOOLCPPFLAGS += -include $(top)/src/commonlib/bsd/include/commonlib/bsd/compiler.h
+
 SMCBIOSINFOTOOL:= $(objutil)/supermicro/smcbiosinfo

$(SMCBIOSINFOTOOL): $(dir)/smcbiosinfo/smcbiosinfo.c
    printf "    HOSTCC     Creating SMCBIOSINFO tool\n"
    mkdir -p $(objutil)/supermicro
-	$(HOSTCC) $< -o $@
+	$(HOSTCC) $(TOOLCPPFLAGS) $< -o $@

ifeq ($(CONFIG_VENDOR_SUPERMICRO),y)
 ifneq ($(call strip_quotes, $(CONFIG_SUPERMICRO_BOARDID)),)