From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 86D8521952CFE for ; Wed, 24 May 2017 13:18:04 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 May 2017 13:18:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,388,1491289200"; d="scan'208";a="1134404442" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by orsmga001.jf.intel.com with ESMTP; 24 May 2017 13:18:04 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.59]) by ORSMSX105.amr.corp.intel.com ([169.254.2.29]) with mapi id 14.03.0319.002; Wed, 24 May 2017 13:18:04 -0700 From: "Kinney, Michael D" To: Ard Biesheuvel , Laszlo Ersek , "Kinney, Michael D" CC: "Wu, Hao A" , "edk2-devel@lists.01.org" , "afish@apple.com" , "Fan, Jeff" Thread-Topic: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Thread-Index: AQHS1Bvps+cEwgPBI0yVHuet7kPBjKICn1nwgAEC8ICAADNMgP//3r4g Date: Wed, 24 May 2017 20:18:03 +0000 Message-ID: References: <1495581673-10788-1-git-send-email-michael.d.kinney@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTU4YmFiN2EtYWVhYy00ZjM2LTkwZjYtNzdkMzY4ZDU0MzI4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InhEOVhMMDNRYTNqaDZONVBrMW1HQlJtdVVjZ1lrbzVUZlp2Z1NRWDRwaGM9In0= dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.22.254.139] MIME-Version: 1.0 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 20:18:04 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I agree with the request to add 'static' to the variable declaration in the SecPeiDebugAgentLib. The variable name change will be retained because the same symbol name can still be confusing when debugging. The part that is more concerning is why this duplicate symbol reference was not triggered by MSFT or GCC tool chain families, so I did some more investigation from the MSFT side this morning. My first thought was that the optimizer in the compiler/linker was=20 optimizing away the duplicate symbol before the final link stage, so I tried -b NOOPT builds. That also did not trigger a duplicate=20 symbol error and the MAP file contains only the single reference to _mMemoryDiscoveredNotifyList from the DxeIpl. 0002:000001b8 _mMemoryDiscoveredNotifyList 000164f8 DxeIpl:DxeL= oad.obj I then evaluated what functions in the DebugAgentLib the DxeIpl is using. It turns out that it is only using a single function SaveAndSetDebugTimerInterrupt() that is implemented in=20 SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c The mMemoryDiscoveredNotifyList duplicate symbol is implemented in SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentL= ib.c Even with a MSFT -b NOOPT build, when I look at the MAP file, the linker only pulls in symbols from the obj in the DebugAgentLib that contains the one SaveAndSetDebugTimerInterrupt() function. The symbols from the other objs in the DebugAgentLib are not included in the final DxeIpl PE/COFF image. 0001:000081b0 _InitializeDebugTimer 00008410 f SecPeiDebugAg= entLib:DebugTimer.obj 0001:00008330 _IsDebugTimerTimeout 00008590 f SecPeiDebugAg= entLib:DebugTimer.obj 0001:000083d0 _SaveAndSetDebugTimerInterrupt 00008630 f SecPeiDeb= ugAgentLib:DebugTimer.obj So it appears that even with -b NOOPT builds, the linker is=20 filtering out unreferenced objs which explains why the duplicate symbol error is not triggered. The XCODE5 tool chain appears to be evaluating symbols across all objs in a lib and detects a duplicate symbol. The question is what is the required linker behavior for EDK II builds? Require library filtering at obj level, or require no duplicates across all objs in all libs? Best regards, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ar= d Biesheuvel > Sent: Wednesday, May 24, 2017 4:53 AM > To: Laszlo Ersek > Cc: Kinney, Michael D ; Wu, Hao A ; > edk2-devel@lists.01.org; afish@apple.com; Fan, Jeff > Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix = duplicate > symbol >=20 > On 24 May 2017 at 01:48, Laszlo Ersek wrote: > > 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 thi= s > > case the object will have to be declared in a module-internal header > > file anyway.) > > >=20 > I strongly agree with Laszlo here. Omitting static defeats any kind of > optimization the compiler can perform when it knows it can see all > references to a variable, such as constant folding or emitting the > variable into .rodata if it does not observe any modifications to it. > In theory, this could have security implications as well as > performance implications (e.g., a variable which only gets set in > DEBUG builds) >=20 > > I grepped the tree for "mMemoryDiscoveredNotifyList", and there are mor= e > > instances, all exhibiting the same issue: > > > > (1) MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > > (2) QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c > > (3) SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebug= AgentLib.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/SecP= eiDebugAgentL > ib/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. > > >=20 > Yes, but given that they are part of a static library, objects are > only pulled in on-demand, and so if all references already happen to > be satisfied, the 'offending' object may never be loaded. >=20 > > (b) > > > >> $ egrep 'SecPeiDebugAgentLib\.lib|DxeIpl\.lib' \ > >> > Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/= static_librar > y_files.lst > >> > .../Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/= SecPeiDebugAg > entLib/OUTPUT/SecPeiDebugAgentLib.lib > >> > .../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUT= PUT/DxeIpl.li > b > > > > 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/D= xeIpl.dll \ > >> -nostdlib \ > >> -Wl,-n,-q,--gc-sections \ > >> -z common-page-size=3D0x20 \ > >> -Wl,--entry,_ModuleEntryPoint \ > >> -u _ModuleEntryPoint \ > >> -Wl,- > Map,Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEB= UG/DxeIpl.map > \ > >> -Wl,-melf_x86_64,--oformat=3Delf64-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=3DDxeIplStrings \ > >> -m64 \ > >> -fno-stack-protector \ > >> "-DEFIAPI=3D__attribute__((ms_abi))" \ > >> -maccumulate-outgoing-args \ > >> -mno-red-zone \ > >> -Wno-address \ > >> -mcmodel=3Dsmall \ > >> -fpie \ > >> -fno-asynchronous-unwind-tables \ > >> -Wno-address \ > >> -Os \ > >> -mno-mmx \ > >> -mno-sse \ > >> -D DISABLE_NEW_DEPRECATED_INTERFACES \ > >> -Wl,--defsym=3DPECOFF_HEADER_SIZE=3D0x228 \ > >> -Wl,--script=3DBaseTools/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 no= t > > succeed. > > >=20 > Depending on link order, this may succeed given the reasoning above. >=20 > > Anyway, regarding the patch, I think that all four declarations of > > "mMemoryDiscoveredNotifyList" should be made STATIC instead. > > >=20 > Yes, please. Especially when it comes to static libraries (due to the > flexible way we allow them to be specified in EDK2), I think it is > really poor hygiene to expose library internals to the library user. I > know we cannot always avoid it, but we should if we can imo. >=20 > -- > Ard. >=20 >=20 > >>> -----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 dup= licate symbol > >>> > >>> Mike, > >>> > >>> Do the other compilers promote (or is that demote) to static? Would n= ot 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=3D573 > >>>> > >>>> 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/SecPeiDebug= AgentLib.c > >>> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebug= AgentLib.c > >>>> index b717e33..9f5223a 100644 > >>>> --- > a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgen= tLib.c > >>>> +++ > b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgen= tLib.c > >>>> @@ -32,7 +32,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPT= OR > >>> mVectorHandoffInf > >>>> } > >>>> }; > >>>> > >>>> -GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR > >>> mMemoryDiscoveredNotifyList[1] =3D { > >>>> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR > >>> mDebugAgentMemoryDiscoveredNotifyList[1] =3D { > >>>> { > >>>> (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 invoke= d immediately > >>>> // > >>>> - Status =3D PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0= ]); > >>>> + Status =3D PeiServicesNotifyPpi (&mDebugAgentMemoryDiscoveredNo= tifyList[0]); > >>>> if (EFI_ERROR (Status)) { > >>>> DEBUG ((EFI_D_ERROR, "DebugAgent: Failed to register memory di= scovered > >>> callback function!\n")); > >>>> CpuDeadLoop (); > >>>> -- > >>>> 2.6.3.windows.1 > >>>> > >> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel