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 <ard.biesheu...@linaro.org>
> Reviewed-by: Liming Gao <liming....@intel.com>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> ---
> 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 <ler...@redhat.com>
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
