Ard: I check VS build. Two copies of mVirtualAddressChangeEvent are mapped to the same memory location. For RuntimeDxe case, it doesn't cause any issue, because their notification function doesn't use Event.
I agree this is a potential issue. But, VS and GCC compiler doesn't report warning for it. Your fix is for GCC tool chain. I will investigate VS solution. Thanks Liming -----Original Message----- From: Ard Biesheuvel [mailto:[email protected]] Sent: Friday, December 04, 2015 5:26 PM To: Gao, Liming Cc: [email protected]; Justen, Jordan L; Zhu, Yonghong Subject: Re: [PATCH] BaseTools GCC: avoid the use of COMMON symbols On 4 December 2015 at 10:17, Gao, Liming <[email protected]> wrote: > Good fix. How do you find this issue? > I got build failures with RVCT when using the UEFI Secure Boot versions of VariableRuntimeDxe. Could you check how VS deals with this? I.e., if the two copies of mVirtualAddressChangeEvent are mapped onto the same memory location? > Reviewed-by: Liming Gao <[email protected]> > Thanks > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Thursday, December 03, 2015 8:54 PM > To: [email protected]; Justen, Jordan L; Gao, Liming; Zhu, > Yonghong > Cc: Ard Biesheuvel > Subject: [PATCH] BaseTools GCC: avoid the use of COMMON symbols > > 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 appearing by > the same name in different C files typically do not refer to the same > conceptual data item. > > For instance, the definitions of EFI_EVENT mVirtualAddressChangeEvent in that > 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. > > So prevent GCC from emitting variables into the COMMON section, by passing > -fno-common to the compiler, and discarding the COMMON section in the GNU ld > linker script. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > > NOTE: this patch will result in build failures until we fix the code to make > at least some instances of mVirtualAddressChangeEvent STATIC > > BaseTools/Conf/tools_def.template | 2 +- > BaseTools/Scripts/GccBase.lds | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/BaseTools/Conf/tools_def.template > b/BaseTools/Conf/tools_def.template > index 564d1336650c..2216e747c02d 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 > 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) > } > } > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
