From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3BC8C219FFEE7 for ; Wed, 24 May 2017 01:49:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 60A07448D6B; Wed, 24 May 2017 08:49:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 60A07448D6B Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 60A07448D6B Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-23.phx2.redhat.com [10.3.116.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id CC45917C41; Wed, 24 May 2017 08:48:58 +0000 (UTC) To: "Kinney, Michael D" , "afish@apple.com" Cc: "edk2-devel@lists.01.org" , "Fan, Jeff" , "Wu, Hao A" , Ard Biesheuvel References: <1495581673-10788-1-git-send-email-michael.d.kinney@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 24 May 2017 10:48:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 24 May 2017 08:49:00 +0000 (UTC) Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 May 2017 08:49:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit CC Ard On 05/24/17 02:27, Kinney, Michael D wrote: > Andrew, > > I agree in this specific case, making the global variable static > should also resolve this issue. > > In general, we do not make module global variables static, so the > module global can be shared across multiple source files in the > module implementation. I think the default should be the reverse: give objects with static storage duration ("global variables") internal linkage ("STATIC") by default, and turn the linkage into external only if multiple source files of the same module actually use the same object together. (In this case the object will have to be declared in a module-internal header file anyway.) I grepped the tree for "mMemoryDiscoveredNotifyList", and there are more instances, all exhibiting the same issue: (1) MdeModulePkg/Core/DxeIplPeim/DxeLoad.c (2) QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c (3) SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c (4) Vlv2TbltDevicePkg/PlatformPei/Platform.c In each of these source files, the "mMemoryDiscoveredNotifyList" variable - has an initializer, - is declared in file scope, - has external linkage, - has static storage duration, thus the declaration qualifies as an "external definition" (of which there may be at most one, for any given object, in the final linking). In each of the four modules listed above, the "mMemoryDiscoveredNotifyList" variable is only used in the same source file that declares / defines the variable. Thus, the variable should be made "STATIC" in every one of them. > Not sure why this issue has not been seen with other tool chains. I think it is either a gcc or a BaseTools (toolchain config) bug. Namely, we faced a similar issue before: please refer to commit 214a3b79417f ("BaseTools GCC: avoid the use of COMMON symbols", 2015-12-08). In that commit, we made sure that gcc wouldn't silently merge multiple external definitions (because that violated ISO C and caused actual runtime bugs). As a result, uninitialized globals were no longer placed in the COMMON section, but in the data section, and multiple external definitions triggered a link editing error. However, in this case we have initialized global variables, which are *already* placed in the data section. I just built OVMF with SOURCE_DEBUG_ENABLE, and verified the following: (a) > $ nm Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib \ > | grep mMemoryDiscoveredNotifyList > 0000000000000000 D mMemoryDiscoveredNotifyList > > $ nm Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib \ > | grep mMemoryDiscoveredNotifyList > 0000000000000000 D mMemoryDiscoveredNotifyList The "D" mark means: - "D" / "d": The symbol is in the initialized data section. - uppercase: the symbol is global (external) In other words, linking these two object archives together should fail. (b) > $ egrep 'SecPeiDebugAgentLib\.lib|DxeIpl\.lib' \ > Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst > .../Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib > .../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib This means that the build process will link them together. Indeed I can find the following *successful* command in the build log (see the reference to the above "static_library_files.lst" object list file): > "gcc" \ > -o \ > Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll \ > -nostdlib \ > -Wl,-n,-q,--gc-sections \ > -z common-page-size=0x20 \ > -Wl,--entry,_ModuleEntryPoint \ > -u _ModuleEntryPoint \ > -Wl,-Map,Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.map \ > -Wl,-melf_x86_64,--oformat=elf64-x86-64 \ > -Wl,--start-group,@Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst,--end-group \ > -g \ > -fshort-wchar \ > -fno-builtin \ > -fno-strict-aliasing \ > -Wall \ > -Werror \ > -Wno-array-bounds \ > -ffunction-sections \ > -fdata-sections \ > -include AutoGen.h \ > -fno-common \ > -DSTRING_ARRAY_NAME=DxeIplStrings \ > -m64 \ > -fno-stack-protector \ > "-DEFIAPI=__attribute__((ms_abi))" \ > -maccumulate-outgoing-args \ > -mno-red-zone \ > -Wno-address \ > -mcmodel=small \ > -fpie \ > -fno-asynchronous-unwind-tables \ > -Wno-address \ > -Os \ > -mno-mmx \ > -mno-sse \ > -D DISABLE_NEW_DEPRECATED_INTERFACES \ > -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \ > -Wl,--script=BaseTools/Scripts/GccBase.lds (c) Re-running the command manually succeeds. (d) Just to see if "-fdata-sections" made any difference ("Place each data item into its own section in the output file"), I removed it. Even that way, the command succeeded. I think this is either a gcc / GNU linker bug, or else our command line (or linker script, "GccBase.lds") is buggy. This link command should not succeed. Anyway, regarding the patch, I think that all four declarations of "mMemoryDiscoveredNotifyList" should be made STATIC instead. Thanks, Laszlo >> -----Original Message----- >> From: afish@apple.com [mailto:afish@apple.com] >> Sent: Tuesday, May 23, 2017 4:26 PM >> To: Kinney, Michael D >> Cc: edk2-devel@lists.01.org; Fan, Jeff ; Wu, Hao A >> ; Laszlo Ersek >> Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol >> >> Mike, >> >> Do the other compilers promote (or is that demote) to static? Would not making these >> lib globals, and private functions static solve this class of issue? >> >> Thanks, >> >> Andrew Fish >> >>> On May 23, 2017, at 4:21 PM, Michael Kinney wrote: >>> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=573 >>> >>> The SecPeiDebugAgentLib uses the global variable >>> mMemoryDiscoveredNotifyList for a PPI notification on >>> the Memory Discovered PPI. This same variable name is >>> used in the DxeIplPeim for the same PPI notification. >>> >>> The XCODE5 tool chain detects this duplicate symbol >>> when the OVMF platform is built with the flag >>> -D SOURCE_DEBUG_ENABLE. >>> >>> The fix is to rename this global variable in the >>> SecPeiDebugAgentLib library. >>> >>> Cc: Andrew Fish >>> Cc: Jeff Fan >>> Cc: Hao Wu >>> Cc: Laszlo Ersek >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael D Kinney >>> --- >>> .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git >> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> index b717e33..9f5223a 100644 >>> --- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c >>> @@ -32,7 +32,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR >> mVectorHandoffInf >>> } >>> }; >>> >>> -GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR >> mMemoryDiscoveredNotifyList[1] = { >>> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR >> mDebugAgentMemoryDiscoveredNotifyList[1] = { >>> { >>> (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | >> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), >>> &gEfiPeiMemoryDiscoveredPpiGuid, >>> @@ -554,7 +554,7 @@ InitializeDebugAgent ( >>> // Register for a callback once memory has been initialized. >>> // If memery has been ready, the callback funtion will be invoked immediately >>> // >>> - Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0]); >>> + Status = PeiServicesNotifyPpi (&mDebugAgentMemoryDiscoveredNotifyList[0]); >>> if (EFI_ERROR (Status)) { >>> DEBUG ((EFI_D_ERROR, "DebugAgent: Failed to register memory discovered >> callback function!\n")); >>> CpuDeadLoop (); >>> -- >>> 2.6.3.windows.1 >>> >