On 6 December 2015 at 01:44, Jordan Justen <jordan.l.jus...@intel.com> wrote: > On 2015-12-04 11:21:18, Ard Biesheuvel wrote: >> On 4 December 2015 at 20:13, Jordan Justen <jordan.l.jus...@intel.com> wrote: >> > On 2015-12-04 08:43:29, Ard Biesheuvel wrote: >> >> On 4 December 2015 at 17:27, Laszlo Ersek <ler...@redhat.com> wrote: >> >> > On 12/04/15 17:23, Paolo Bonzini wrote: >> >> >> >> >> >> >> >> >> On 04/12/2015 11:39, Laszlo Ersek wrote: >> >> >>> (4) Linking those two files into a complete program is a violation of >> >> >>> "6.7 External definitions": >> >> >>> >> >> >>> [...] If an identifier declared with external linkage is used in >> >> >>> an >> >> >>> expression (other than as part of the operand of a *sizeof* >> >> >>> operator), somewhere in the entire program there shall be exactly >> >> >>> one external definition for the identifier [...] >> >> >>> >> >> >>> Again, how does the argument go? >> >> >>> >> >> >>> - In each file we have exactly one tentative definition, and no >> >> >>> declaration that would be, in its own right, an external definition. >> >> >>> Based on (2), the files must behave exactly as-if there was one >> >> >>> external *definition* for "x" in each. >> >> >>> >> >> >>> - Argument (3) explains why "x" has external *linkage*. >> >> >>> >> >> >>> - Argument (4) applies to "x" because "x" has external linkage, and is >> >> >>> used in a non-sizeof expression. And the requirement set forth in >> >> >>> (4) >> >> >>> is broken by the program because the program is required to behave >> >> >>> exactly as if "x" had two external definitions. >> >> >>> >> >> >>> In practical terms: >> >> >>> >> >> >>> - If you compile the first version of the program (without any special >> >> >>> options), it links together successfully: >> >> >>> >> >> >>> $ gcc -std=c89 -pedantic -Wall -Wextra -o f f1.c f2.c >> >> >>> >> >> >>> - If you compile the second (equivalent) version of the program, >> >> >>> linkage fails: >> >> >>> >> >> >>> $ gcc -std=c89 -pedantic -Wall -Wextra -o f-b f1-b.c f2-b.c >> >> >>> >> >> >>> /tmp/cc8Isbn8.o:(.bss+0x0): multiple definition of `x' >> >> >>> /tmp/cciQUlDz.o:(.bss+0x0): first defined here >> >> >>> collect2: error: ld returned 1 exit status >> >> >>> >> >> >>> - The gcc bug is that linking the first version *too* should fail, >> >> >>> without any particular options. >> >> >> >> >> >> That's true, but it would break existing code that declares variables >> >> >> in >> >> >> headers without "extern". >> >> > >> >> > I agree. After I posted the email I pondered what it would take to fix >> >> > this in gcc. It would be impossible. :) >> >> > >> >> >> That's why Visual Studio does the same. >> >> > >> >> > Looks plausible. >> >> > >> >> > Thanks! >> >> > Laszlo >> >> > >> >> >> >> Indeed. Liming is looking into that. I spotted very few issues with >> >> -fno-common, only two in CryptoPkg, so I think we should disallow >> >> common allocations going forward if we can, especially since the >> >> modularity of EDK2 makes it almost impossible to guarantee that it >> >> never leads to problems. >> >> >> >> Qin has already acked a fix for one of those issues, I will ping him >> >> for the other one. In the mean time, perhaps GCC users could double >> >> check whether there is any breakage? Laszlo, could you check your OVMF >> >> builds, please? >> > >> > I'm seeing a build error with MdeModulePkg and OvmfPkg. This is with >> > gcc 5.2 using the GCC49 toolchain. >> > >> > Both cases seem to be unitialized global variables declared in a >> > single .c file. I'm not sure what needs to be changed to fix this. >> > >> > MdeModulePkg: >> > >> > "ld" -o >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/DEBUG/MemoryProfileInfo.dll >> > -nostdlib -n -q --gc-sections -z common-page-size=0x40 --entry >> > _ModuleEntryPoint -u _ModuleEntryPoint -Map >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/DEBUG/MemoryProfileInfo.map >> > -melf_x86_64 --oformat=elf64-x86-64 --start-group >> > @Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/static_library_files.lst >> > --end-group --defsym=PECOFF_HEADER_SIZE=0x228 >> > --script=BaseTools/Scripts/GccBase.lds >> > `mNameString' referenced in section `.text.GetDriverNameString' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj): >> > defined in discarded section `COMMON' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj) >> > `mNameString' referenced in section `.text.GetDriverNameString' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj): >> > defined in discarded section `COMMON' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj) >> > `mNameString' referenced in section `.text.GetDriverNameString' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj): >> > defined in discarded section `COMMON' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj) >> > `mNameString' referenced in section `.text.GetDriverNameString' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj): >> > defined in discarded section `COMMON' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj) >> > `mNameString' referenced in section `.text.DumpMemoryProfileDriverInfo' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj): >> > defined in discarded section `COMMON' of >> > Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/OUTPUT/MemoryProfileInfo.lib(MemoryProfileInfo.obj) >> > make: *** >> > [Build/MdeModule/DEBUG_GCC49/X64/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo/DEBUG/MemoryProfileInfo.dll] >> > Error 1 >> > >> > OvmfPkg: >> > >> > "ld" -o >> > Build/OvmfX64/DEBUG_GCC49/X64/IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei/DEBUG/StatusCodePei.dll >> > -nostdlib -n -q --gc-sections -z common-page-size=0x40 --entry >> > _ModuleEntryPoint -u _ModuleEntryPoint -Map >> > Build/OvmfX64/DEBUG_GCC49/X64/IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei/DEBUG/StatusCodePei.map >> > -melf_x86_64 --oformat=elf64-x86-64 --start-group >> > @Build/OvmfX64/DEBUG_GCC49/X64/IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei/OUTPUT/static_library_files.lst >> > --end-group --defsym=PECOFF_HEADER_SIZE=0x228 >> > --script=BaseTools/Scripts/GccBase.lds >> > `gNullVaList' referenced in section `.text.AsciiBSPrint' of >> > Build/OvmfX64/DEBUG_GCC49/X64/MdePkg/Library/BasePrintLib/BasePrintLib/OUTPUT/BasePrintLib.lib(PrintLib.obj): >> > defined in discarded section `COMMON' of >> > Build/OvmfX64/DEBUG_GCC49/X64/MdePkg/Library/BasePrintLib/BasePrintLib/OUTPUT/BasePrintLib.lib(PrintLib.obj) >> > GNUmakefile:406: recipe for target >> > 'Build/OvmfX64/DEBUG_GCC49/X64/IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei/DEBUG/StatusCodePei.dll' >> > failed >> > make: *** >> > [Build/OvmfX64/DEBUG_GCC49/X64/IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei/DEBUG/StatusCodePei.dll] >> > Error 1 >> > >> >> Is this a clean rebuild? > > Yes, it was. >
And did you regenerate your Conf/tools_def.txt as well? I cannot reproduce the behavior where COMMON sections are created by GCC in the presence of -fno-common. > The discussion with Daryl along with the fact that these were > uninitialized global variables seems to explain this. > > This means that with this change we couldn't have any uninitialized > global variables? > No, that is not the consequence. The consequence is that uninitialized global variables with external linkage that happen to have the same name (like the mVirtualAddressChangeEvent example I gave) are no longer projected onto the same memory location. So what it means is that, unless a global is declared static, there can only be a single instance, or you would get a link error. The modularity of EDK2 makes that it is difficult to decide a priori that a static library you link to (and whose binary is all you have access to) does not define uninitialized variables with clashing names. So it is better to flag it than to silently let the linker merge those variables into a single one -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
