Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
libpayload: Include missing compiler.h
These header files define various structs like so:
struct struct_name { ... } __packed;
However, these header files do not include the compiler.h macro that defines what __packed is, so they are actually defining a variable named __packed and *not* declaring a packed struct. This leads to defining the same variable multiple times, which was caught by GCC 10. Include the missing <compiler.h> header to fix this.
Change-Id: Ia67182520dc94149e06fe9e03a14b3fc2ee29973 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M payloads/libpayload/drivers/usb/uhci_private.h M payloads/libpayload/drivers/video/bitmap.h M payloads/libpayload/include/arm/arch/exception.h M payloads/libpayload/include/arm64/arch/exception.h M payloads/libpayload/include/fmap_serialized.h M payloads/libpayload/include/x86/arch/exception.h 6 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/47153/1
diff --git a/payloads/libpayload/drivers/usb/uhci_private.h b/payloads/libpayload/drivers/usb/uhci_private.h index 9df8f3d..9854f15 100644 --- a/payloads/libpayload/drivers/usb/uhci_private.h +++ b/payloads/libpayload/drivers/usb/uhci_private.h @@ -29,6 +29,8 @@ #ifndef __UHCI_PRIVATE_H #define __UHCI_PRIVATE_H
+#include <compiler.h> + typedef enum { UHCI_SETUP = 0x2d, UHCI_IN = 0x69, UHCI_OUT = 0xe1 } uhci_pid_t;
typedef u32 flistp_t; diff --git a/payloads/libpayload/drivers/video/bitmap.h b/payloads/libpayload/drivers/video/bitmap.h index 7a596f2..0178961 100644 --- a/payloads/libpayload/drivers/video/bitmap.h +++ b/payloads/libpayload/drivers/video/bitmap.h @@ -29,6 +29,7 @@ #ifndef __BITMAP_H__ #define __BITMAP_H__
+#include <compiler.h> #include <stdint.h>
struct bitmap_file_header { diff --git a/payloads/libpayload/include/arm/arch/exception.h b/payloads/libpayload/include/arm/arch/exception.h index ee8520e..2360fd1 100644 --- a/payloads/libpayload/include/arm/arch/exception.h +++ b/payloads/libpayload/include/arm/arch/exception.h @@ -29,6 +29,7 @@ #ifndef _ARCH_EXCEPTION_H #define _ARCH_EXCEPTION_H
+#include <compiler.h> #include <stdint.h>
void exception_dispatch(u32 idx); diff --git a/payloads/libpayload/include/arm64/arch/exception.h b/payloads/libpayload/include/arm64/arch/exception.h index 1b4d58a..754c0ce 100644 --- a/payloads/libpayload/include/arm64/arch/exception.h +++ b/payloads/libpayload/include/arm64/arch/exception.h @@ -53,6 +53,7 @@
#ifndef __ASSEMBLER__
+#include <compiler.h> #include <stddef.h> #include <stdint.h>
diff --git a/payloads/libpayload/include/fmap_serialized.h b/payloads/libpayload/include/fmap_serialized.h index 53a09af..f5c7d03 100644 --- a/payloads/libpayload/include/fmap_serialized.h +++ b/payloads/libpayload/include/fmap_serialized.h @@ -36,6 +36,7 @@ #ifndef FLASHMAP_SERIALIZED_H__ #define FLASHMAP_SERIALIZED_H__
+#include <compiler.h> #include <stdint.h>
#define FMAP_SIGNATURE "__FMAP__" diff --git a/payloads/libpayload/include/x86/arch/exception.h b/payloads/libpayload/include/x86/arch/exception.h index d88029b..c4e8fd3 100644 --- a/payloads/libpayload/include/x86/arch/exception.h +++ b/payloads/libpayload/include/x86/arch/exception.h @@ -29,6 +29,7 @@ #ifndef _ARCH_EXCEPTION_H #define _ARCH_EXCEPTION_H
+#include <compiler.h> #include <stdint.h>
void exception_init_asm(void);
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
Patch Set 1:
This was a sneaky one...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
Patch Set 1:
I'd say we should move `__packed` before the opening brace to avoid this.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
Patch Set 1:
I'd say we should move `__packed` before the opening brace to avoid this.
Yes, please :)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Include missing compiler.h ......................................................................
Patch Set 1:
Alternatively (to the #includes) we could add `compiler.h` to the compiler parameters like we do for coreboot.
Hello Felix Singer, build bot (Jenkins), Nico Huber, Paul Menzel, Angel Pons, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47153
to look at the new patch set (#2).
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
libpayload: Add compiler.h to compiler parameters
Headers in libpayload define various structs like so:
struct struct_name { ... } __packed;
However, these header files do not include the compiler.h macro that defines what __packed is, so they are actually defining a variable named __packed and *not* declaring a packed struct. This leads to defining the same variable multiple times, which was caught by GCC 10. Add compiler.h to the compiler parameters so it is included in all files automatically.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: Ia67182520dc94149e06fe9e03a14b3fc2ee29973 --- M payloads/coreinfo/Makefile M payloads/libpayload/Makefile.inc M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/include/libpayload.h 4 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/47153/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
Alternatively (to the #includes) we could add `compiler.h` to the compiler parameters like we do for coreboot.
That's a better idea,updated.
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... PS2, Line 47: #include <compiler.h> This header file can technically be removed now, but doing so may break other projects that implicitly rely on it being included, so perhaps it should stay in.
Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47153
to look at the new patch set (#3).
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
libpayload: Add compiler.h to compiler parameters
Headers in libpayload define various structs like so:
struct struct_name { ... } __packed;
However, these header files do not include the compiler.h macro that defines what __packed is, so they are actually defining a variable named __packed and *not* declaring a packed struct. This leads to defining the same variable multiple times, which was caught by GCC 10. Add compiler.h to the compiler parameters so it is included in all files automatically.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: Ia67182520dc94149e06fe9e03a14b3fc2ee29973 --- M payloads/libpayload/Makefile.inc 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/47153/3
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... PS2, Line 47: #include <compiler.h>
This header file can technically be removed now, but doing so may break other projects that implicit […]
After talking to Jenkins, I have determined it should stay. Reverted.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 3: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... PS2, Line 47: #include <compiler.h>
After talking to Jenkins, I have determined it should stay. Reverted.
Payloads are supposed to use `bin/lpgcc`. It should `-include compiler.h` too.
Hello Felix Singer, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47153
to look at the new patch set (#4).
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
libpayload: Add compiler.h to compiler parameters
Headers in libpayload define various structs like so:
struct struct_name { ... } __packed;
However, these header files do not include the compiler.h macro that defines what __packed is, so they are actually defining a variable named __packed and *not* declaring a packed struct. This leads to defining the same variable multiple times, which was caught by GCC 10. Add compiler.h to the compiler parameters so it is included in all files automatically.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: Ia67182520dc94149e06fe9e03a14b3fc2ee29973 --- M payloads/libpayload/Makefile.inc M payloads/libpayload/bin/lpgcc M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/include/libpayload.h 4 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/47153/4
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/47153/2/payloads/libpayload/include... PS2, Line 47: #include <compiler.h>
Payloads are supposed to use `bin/lpgcc`. It should `-include compiler.h` too.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
Patch Set 4: Code-Review+2
Thanks!
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47153 )
Change subject: libpayload: Add compiler.h to compiler parameters ......................................................................
libpayload: Add compiler.h to compiler parameters
Headers in libpayload define various structs like so:
struct struct_name { ... } __packed;
However, these header files do not include the compiler.h macro that defines what __packed is, so they are actually defining a variable named __packed and *not* declaring a packed struct. This leads to defining the same variable multiple times, which was caught by GCC 10. Add compiler.h to the compiler parameters so it is included in all files automatically.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: Ia67182520dc94149e06fe9e03a14b3fc2ee29973 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47153 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/Makefile.inc M payloads/libpayload/bin/lpgcc M payloads/libpayload/include/cbfs_core.h M payloads/libpayload/include/libpayload.h 4 files changed, 4 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/payloads/libpayload/Makefile.inc b/payloads/libpayload/Makefile.inc index 1b2a883..6188dde 100644 --- a/payloads/libpayload/Makefile.inc +++ b/payloads/libpayload/Makefile.inc @@ -55,7 +55,8 @@ subdirs-$(CONFIG_LP_LZMA) += liblzma subdirs-$(CONFIG_LP_LZ4) += liblz4
-INCLUDES := -Iinclude -Iinclude/$(ARCHDIR-y) -I$(obj) -include include/kconfig.h +INCLUDES := -Iinclude -Iinclude/$(ARCHDIR-y) -I$(obj) +INCLUDES += -include include/kconfig.h -include include/compiler.h
CFLAGS += $(EXTRA_CFLAGS) $(INCLUDES) -Os -pipe -nostdinc -ggdb3 CFLAGS += -nostdlib -fno-builtin -ffreestanding -fomit-frame-pointer diff --git a/payloads/libpayload/bin/lpgcc b/payloads/libpayload/bin/lpgcc index 2657a1a..aa09c1d 100755 --- a/payloads/libpayload/bin/lpgcc +++ b/payloads/libpayload/bin/lpgcc @@ -152,7 +152,8 @@ trygccoption -fno-stack-protector [ $? -eq 0 ] && _CFLAGS="$_CFLAGS -fno-stack-protector"
-_CFLAGS="$_CFLAGS -include $BASE/../include/kconfig.h -I`$DEFAULT_CC $_ARCHEXTRA -print-search-dirs | head -n 1 | cut -d' ' -f2`include" +_CFLAGS="$_CFLAGS -include $BASE/../include/kconfig.h -include $BASE/../include/compiler.h" +_CFLAGS="$_CFLAGS -I`$DEFAULT_CC $_ARCHEXTRA -print-search-dirs | head -n 1 | cut -d' ' -f2`include"
_LDFLAGS="-L$BASE/../lib -L$_LIBDIR $_LDSCRIPT -static"
diff --git a/payloads/libpayload/include/cbfs_core.h b/payloads/libpayload/include/cbfs_core.h index 397e08b3..fc4caa4 100644 --- a/payloads/libpayload/include/cbfs_core.h +++ b/payloads/libpayload/include/cbfs_core.h @@ -49,7 +49,6 @@ #include <stddef.h> #include <stdint.h> #include <stdlib.h> -#include <compiler.h>
/** These are standard values for the known compression alogrithms that coreboot knows about for stages and diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index fa501a7..f206fea 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -44,7 +44,6 @@
#include <stdbool.h> #include <libpayload-config.h> -#include <compiler.h> #include <cbgfx.h> #include <ctype.h> #include <die.h>
-
Angel Pons (Code Review) -
Jacob Garber (Code Review)
-
Nico Huber (Code Review)