On 7 December 2015 at 16:23, Laszlo Ersek <[email protected]> wrote: > On 12/07/15 10:52, Ard Biesheuvel wrote: >> The default behavior of the GCC compiler is to emit uninitialized globals >> into a COMMON section, where duplicate definitions are merged. This may >> result in unexpected behavior, since global variables defined under the >> same name in different C files may not refer to the same logical data item. >> >> For instance, the definitions of EFI_EVENT mVirtualAddressChangeEvent that >> [used to] appear in the following files: >> >> CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> >> will be folded into a single instance of the variable when the latter >> module includes the former library, which can lead to unexpected results. >> >> Even if some may argue that there are legal uses for COMMON allocation, the >> high modularity of EDK2 combined with the low level of awareness of the >> intracicies surrounding common allocation and the generally poor EDK2 >> developer discipline regarding the use of the STATIC keyword* make a strong >> case for disabling it by default, and re-enabling it explicitly for packages >> that depend on it. >> >> So prevent GCC from emitting variables into the COMMON section, by passing >> -fno-common to the compiler, and discarding the section in the GNU ld linker >> script. >> >> * Any function or variable that is only referenced from the translation unit >> that defines it could be made STATIC. This does not only prevent issues >> like >> the above, it also allows the compiler to generate better code, e.g., drop >> out of line function definitions after inlining all invocations or perform >> constant propagation on variables. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <[email protected]> >> Reviewed-by: Liming Gao <[email protected]> >> Reviewed-by: Laszlo Ersek <[email protected]> >> --- >> v2: added the -fno-common argument to GCC44_ALL_CC_FLAGS as well, since GCC4x >> for X64 and IA32 does not include GCC_ALL_CC_FLAGS >> >> BaseTools/Conf/tools_def.template | 4 ++-- >> BaseTools/Scripts/GccBase.lds | 3 ++- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/BaseTools/Conf/tools_def.template >> b/BaseTools/Conf/tools_def.template >> index d6b0af43d772..1a44fbd1d3eb 100644 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -4318,7 +4318,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO >> /NODEFAULTLIB /LTCG /DLL /OPT:REF >> DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = >> --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug >> RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG = >> >> -DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar >> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h >> +DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar >> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h >> -fno-common >> DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 >> -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 >> -mno-stack-arg-probe >> DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone >> -Wno-address -mno-stack-arg-probe >> DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) >> -minline-int-divide-min-latency >> @@ -4349,7 +4349,7 @@ DEFINE GCC_IPF_RC_FLAGS = -I binary -O >> elf64-ia64-little -B ia64 >> DEFINE GCC_ARM_RC_FLAGS = -I binary -O elf32-littlearm -B >> arm --rename-section .data=.hii >> DEFINE GCC_AARCH64_RC_FLAGS = -I binary -O elf64-littleaarch64 -B >> aarch64 --rename-section .data=.hii >> >> -DEFINE GCC44_ALL_CC_FLAGS = -g -fshort-wchar >> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections >> -fdata-sections -c -include AutoGen.h -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> +DEFINE GCC44_ALL_CC_FLAGS = -g -fshort-wchar >> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections >> -fdata-sections -c -include AutoGen.h -fno-common >> -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> DEFINE GCC44_IA32_CC_FLAGS = DEF(GCC44_ALL_CC_FLAGS) -m32 >> -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables >> DEFINE GCC44_X64_CC_FLAGS = DEF(GCC44_ALL_CC_FLAGS) -m64 >> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" >> -DNO_BUILTIN_VA_FUNCS -mno-red-zone -Wno-address -mcmodel=large >> -fno-asynchronous-unwind-tables >> DEFINE GCC44_IA32_X64_DLINK_COMMON = -nostdlib -n -q --gc-sections -z >> common-page-size=0x20 >> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds >> index 4ee6d998532c..32310bc75dcc 100644 >> --- a/BaseTools/Scripts/GccBase.lds >> +++ b/BaseTools/Scripts/GccBase.lds >> @@ -46,7 +46,7 @@ SECTIONS { >> */ >> .data ALIGN(ALIGNOF(.text)) : ALIGN(CONSTANT(COMMONPAGESIZE)) { >> *(.data .data.* .gnu.linkonce.d.*) >> - *(.bss .bss.* *COM*) >> + *(.bss .bss.*) >> } >> >> .eh_frame ALIGN(CONSTANT(COMMONPAGESIZE)) : { >> @@ -66,5 +66,6 @@ SECTIONS { >> *(.dynamic) >> *(.hash) >> *(.comment) >> + *(COMMON) >> } >> } >> > > I build tested this patch with the following OVMF settings: > > # DSC SECURE_BOOT_ENABLE NETWORK_IP6_ENABLE HTTP_BOOT_ENABLE > SMM_REQUIRE > - ------- ------------------ ------------------ ---------------- > ----------- > 1 X64 X > 2 X64 X X X > 3 X64 X X X > 4 Ia32 X X X > 5 Ia32X64 X X X > > In my local tree the FatPkg driver is also built from source (currently > at SVN r96), so the above should cover that as well. > > Tested-by: Laszlo Ersek <[email protected]> >
Committed as SVN r19164 Thanks everyone, Ard. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
