Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable dangerous optimizations ......................................................................
xcompile: Disable dangerous optimizations
According to the C standard, GCC is allowed to perform optimizations based on undefined behaviour. However, much of this undefined behaviour is difficult to detect and sometimes is even intentional (eg. accessing memory at address 0), so these optimizations can silently break code. This patch disables the following optimizations in the coreboot tree, which is already done in several other projects (eg. Linux kernel, Chromium EC):
-fno-delete-null-pointer-checks -fno-strict-aliasing -fno-strict-overflow
Disabling these optimizations is particularly important when using link time optimization, since LTO allows the compiler to perform deeper code inspection and potentially uncover issues that weren't previously apparent. For example, coreinfo compiled with LTO libpayload crashes without -fno-delete-null-pointer-checks, presumably because the compiler is optimizing something out that it shouldn't.
Change-Id: I4492277f02418ade3fe7a75304e8e0611f49ef36 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M Makefile.inc M src/vendorcode/amd/agesa/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Makefile.inc M util/xcompile/xcompile 5 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38289/1
diff --git a/Makefile.inc b/Makefile.inc index 002d3e7..caf82f4 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -414,7 +414,6 @@ CFLAGS_common += -fno-common -ffreestanding -fno-builtin -fomit-frame-pointer CFLAGS_common += -ffunction-sections -fdata-sections -fno-pie ifeq ($(CONFIG_COMPILER_GCC),y) -CFLAGS_common += -fno-delete-null-pointer-checks # Don't add these GCC specific flags when running scan-build ifeq ($(CCC_ANALYZER_OUTPUT_FORMAT),) CFLAGS_common += -Wno-packed-not-aligned diff --git a/src/vendorcode/amd/agesa/Makefile.inc b/src/vendorcode/amd/agesa/Makefile.inc index b96af84..6381324 100644 --- a/src/vendorcode/amd/agesa/Makefile.inc +++ b/src/vendorcode/amd/agesa/Makefile.inc @@ -12,7 +12,7 @@ libagesa-generic-ccopts += -fno-zero-initialized-in-bss libagesa-generic-ccopts += $(AGESA_INC) $(AGESA_AUTOINCLUDES)
-AGESA_CFLAGS := -march=k8-sse3 -mtune=k8-sse3 -fno-strict-aliasing +AGESA_CFLAGS := -march=k8-sse3 -mtune=k8-sse3
CFLAGS_x86_32 += $(AGESA_CFLAGS) CFLAGS_x86_64 += $(AGESA_CFLAGS) diff --git a/src/vendorcode/amd/pi/00670F00/Makefile.inc b/src/vendorcode/amd/pi/00670F00/Makefile.inc index f6cd8eb..cee4672 100644 --- a/src/vendorcode/amd/pi/00670F00/Makefile.inc +++ b/src/vendorcode/amd/pi/00670F00/Makefile.inc @@ -55,7 +55,7 @@ AGESA_INC += -I$(src)/commonlib/include AGESA_INC += -I$(VBOOT_SOURCE)/firmware/include
-AGESA_CFLAGS += -march=amdfam10 -fno-strict-aliasing -D__LIBAGESA__ +AGESA_CFLAGS += -march=amdfam10 -D__LIBAGESA__
CC_bootblock := $(CC_bootblock) $(BINARY_PI_INC) CC_romstage := $(CC_romstage) $(BINARY_PI_INC) diff --git a/src/vendorcode/amd/pi/Makefile.inc b/src/vendorcode/amd/pi/Makefile.inc index 9b3a0e6..d55de29 100644 --- a/src/vendorcode/amd/pi/Makefile.inc +++ b/src/vendorcode/amd/pi/Makefile.inc @@ -69,7 +69,7 @@ AGESA_INC += -I$(VBOOT_SOURCE)/firmware/include
AGESA_CFLAGS += -march=amdfam10 -mno-3dnow -AGESA_CFLAGS += -fno-strict-aliasing -D__LIBAGESA__ +AGESA_CFLAGS += -D__LIBAGESA__ CFLAGS_x86_32 += $(AGESA_CFLAGS) CFLAGS_x86_64 += $(AGESA_CFLAGS)
diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile index 8335c34..cc4641e 100755 --- a/util/xcompile/xcompile +++ b/util/xcompile/xcompile @@ -222,6 +222,7 @@ GCC_CFLAGS_${TARCH}:=${CFLAGS_GCC} # Generally available for GCC's cc1: GCC_CFLAGS_${TARCH}+=-Wlogical-op +GCC_CFLAGS_${TARCH}+=-fno-delete-null-pointer-checks -fno-strict-aliasing -fno-strict-overflow GCC_ADAFLAGS_${TARCH}:=${CFLAGS_GCC} GCC_COMPILER_RT_${TARCH}:=${CC_RT_GCC} GCC_COMPILER_RT_FLAGS_${TARCH}:=${CC_RT_EXTRA_GCC}
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable dangerous optimizations ......................................................................
Patch Set 1: Code-Review-1
Please keep the optimisations enabled where they have not been proven to cause problems.
AFAIR removing strict-aliasing creates poor code around MMIO write operations.
Hello Kyösti Mälkki, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38289
to look at the new patch set (#2).
Change subject: xcompile: Disable null pointer optimizations ......................................................................
xcompile: Disable null pointer optimizations
According to the C standard, accessing the NULL pointer (memory at address zero) is undefined behaviour, and so GCC is allowed to optimize it out. Of course, accessing this memory location is sometimes necessary, so this optimization can be disabled using -fno-delete-null-pointer-checks. This is already done in coreboot, but adding it to xcompile will also disable it for all the payloads. For example, coreinfo compiled with LTO libpayload crashes when this flag isn't set, presumably because the compiler is optimizing something out that it shouldn't.
Change-Id: I4492277f02418ade3fe7a75304e8e0611f49ef36 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M Makefile.inc M util/xcompile/xcompile 2 files changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38289/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable null pointer optimizations ......................................................................
Patch Set 2:
Patch Set 1: Code-Review-1
Please keep the optimisations enabled where they have not been proven to cause problems.
AFAIR removing strict-aliasing creates poor code around MMIO write operations.
Alright, I've only disabled the null pointer optimization. coreinfo still seems to work with the other optimizations enabled, but I think more testing should still be done (perhaps LTO should be labeled as experimental for now).
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable null pointer optimizations ......................................................................
Patch Set 2: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable null pointer optimizations ......................................................................
Patch Set 3: Code-Review+2
*mutters something about undefined behavior being highly cursed*
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable null pointer optimizations ......................................................................
xcompile: Disable null pointer optimizations
According to the C standard, accessing the NULL pointer (memory at address zero) is undefined behaviour, and so GCC is allowed to optimize it out. Of course, accessing this memory location is sometimes necessary, so this optimization can be disabled using -fno-delete-null-pointer-checks. This is already done in coreboot, but adding it to xcompile will also disable it for all the payloads. For example, coreinfo compiled with LTO libpayload crashes when this flag isn't set, presumably because the compiler is optimizing something out that it shouldn't.
Change-Id: I4492277f02418ade3fe7a75304e8e0611f49ef36 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Reviewed-on: https://review.coreboot.org/c/coreboot/+/38289 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Makefile.inc M util/xcompile/xcompile 2 files changed, 1 insertion(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index e9c5054..4ca173b 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -414,7 +414,6 @@ CFLAGS_common += -fno-common -ffreestanding -fno-builtin -fomit-frame-pointer CFLAGS_common += -ffunction-sections -fdata-sections -fno-pie ifeq ($(CONFIG_COMPILER_GCC),y) -CFLAGS_common += -fno-delete-null-pointer-checks # Don't add these GCC specific flags when running scan-build ifeq ($(CCC_ANALYZER_OUTPUT_FORMAT),) CFLAGS_common += -Wno-packed-not-aligned diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile index 8335c34..a116407 100755 --- a/util/xcompile/xcompile +++ b/util/xcompile/xcompile @@ -221,7 +221,7 @@ GCC_CC_${TARCH}:=${GCC} GCC_CFLAGS_${TARCH}:=${CFLAGS_GCC} # Generally available for GCC's cc1: -GCC_CFLAGS_${TARCH}+=-Wlogical-op +GCC_CFLAGS_${TARCH}+=-fno-delete-null-pointer-checks -Wlogical-op GCC_ADAFLAGS_${TARCH}:=${CFLAGS_GCC} GCC_COMPILER_RT_${TARCH}:=${CC_RT_GCC} GCC_COMPILER_RT_FLAGS_${TARCH}:=${CC_RT_EXTRA_GCC}
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38289 )
Change subject: xcompile: Disable null pointer optimizations ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/414 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/413 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/412
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Jacob Garber (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)