From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 CEFED21A16ECB for ; Fri, 26 May 2017 16:06:06 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP; 26 May 2017 16:06:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,399,1491289200"; d="scan'208";a="1174901625" Received: from orsmsx110.amr.corp.intel.com ([10.22.240.8]) by fmsmga002.fm.intel.com with ESMTP; 26 May 2017 16:06:06 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.59]) by ORSMSX110.amr.corp.intel.com ([169.254.10.119]) with mapi id 14.03.0319.002; Fri, 26 May 2017 16:06:05 -0700 From: "Kinney, Michael D" To: Felix Poludov , "Gao, Liming" , "afish@apple.com" , Laszlo Ersek , "Kinney, Michael D" CC: "Wu, Hao A" , "edk2-devel@lists.01.org" , "Fan, Jeff" , Ard Biesheuvel Thread-Topic: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Thread-Index: AQHS1BtKrH1XaxmZJEaVg215IKXpe6IDPWgAgAI9aZCAAHhigIAABMyAgAACfgCAAAcvgIAABliA//+ealCAAOaEAP//mRnwgACe64CAAOJcgP//mJzA Date: Fri, 26 May 2017 23:06:05 +0000 Message-ID: References: <1495581673-10788-1-git-send-email-michael.d.kinney@intel.com> <542CF652F8836A4AB8DBFAAD40ED192A4C5E94B8@shsmsx102.ccr.corp.intel.com> <56801ADE-446F-43C2-9C99-5500E8EE5881@apple.com> <340C28EC-60CF-4390-A8AD-25F9DA22538F@apple.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D734727@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D734868@shsmsx102.ccr.corp.intel.com> <9333E191E0D52B4999CE63A99BA663A00302B22075@atlms1.us.megatrends.com> In-Reply-To: <9333E191E0D52B4999CE63A99BA663A00302B22075@atlms1.us.megatrends.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYWMwYzkxMjMtODM3My00NDI4LTk2YTUtNmY0ZmY1MDVlMzVlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ikc0cjF1ZEIwXC9RbWxFZEZUVmpCUVBKdWhnNEZHY25JZHl5ZEx1djZNWjljPSJ9 dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.22.254.138] 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: Fri, 26 May 2017 23:06:07 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Felix, Yes. I agree. I will work on a Bugzilla issue for this topic and I prefer the idea of updating Base.h to check _MSC_VER value. The one challenge is that 'static' could be added in front of GLOBAL_REMOVE_IF_UNREFERENCED, and it will build correctly with VS2012 and newer, but would fail with older VS versions that=20 require __declspec(selectany) for size optimization. Thanks, Mike > -----Original Message----- > From: Felix Poludov [mailto:Felixp@ami.com] > Sent: Friday, May 26, 2017 3:12 PM > To: Gao, Liming ; Kinney, Michael D > ; afish@apple.com; Laszlo Ersek > Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff > ; Ard Biesheuvel > Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix > duplicate symbol >=20 > Another option to support older VS tool chains is to define > GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany) only for these too= l > chains. > One way to do it is to use _MSC_VER macro: > #if _MSC_VER < .... > #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany) > #endif >=20 > Alternatively GLOBAL_REMOVE_IF_UNREFERENCED can be defined for specific t= ool > chains in the tools_def.txt using /D compiler switch. >=20 > -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Friday, May 26, 2017 4:42 AM > To: Kinney, Michael D; afish@apple.com; Laszlo Ersek > Cc: Wu, Hao A; edk2-devel@lists.01.org; Fan, Jeff; Felix Poludov; Ard Bie= sheuvel > Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix > duplicate symbol >=20 > Mike: > Yes. /Gw option is added since VS2013. The older VS version can't use t= his > option. I suggest we always define GLOBAL_REMOVE_IF_UNREFERENCED as empty= , and > drop this size optimization for the older version MS compiler. I collect = its size > of OvmfIa32X64 DEBUG tip with VS2015 tool chain on. After define it as em= pty, DXE > Raw size increases ~55K, but PEI raw size and the compressed size doesn't > increase big. >=20 > 1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Spac= e > Information SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_= COMPACT > [62%Full] 1753088 total, 1099872 used, 653216 free DXEFV [39%Full] 104857= 60 > total, 4099344 used, 6386416 free PEIFV [18%Full] 917504 total, 172072 us= ed, > 745432 free >=20 > 2. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty. FV Space Information SE= CFV > [10%Full] 212992 total, 22912 used, 190080 free FVMAIN_COMPACT [63%Full] = 1753088 > total, 1105992 used, 647096 free DXEFV [39%Full] 10485760 total, 4154480 = used, > 6331280 free PEIFV [18%Full] 917504 total, 173448 used, 744056 free >=20 > FVMAIN_COMPACT +6120 > DXEFV + 55136 > PEIFV + 1376 >=20 > 3. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty and append /Gw option. F= V Space > Information. > SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%= Full] > 1753088 total, 1099552 used, 653536 free DXEFV [39%Full] 10485760 total, = 4097456 > used, 6388304 free PEIFV [18%Full] 917504 total, 171944 used, 745560 free >=20 > FVMAIN_COMPACT -320 > DXEFV -1888 > PEIFV -128 >=20 > Thanks > Liming > >-----Original Message----- > >From: Kinney, Michael D > >Sent: Friday, May 26, 2017 2:20 PM > >To: Gao, Liming ; afish@apple.com; Laszlo Ersek > >; Kinney, Michael D > >Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, Jeff > >; Felix Poludov ; Ard Biesheuvel > > > >Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: > >Fix duplicate symbol > > > >Liming, > > > >I agree with /Gw. That works for newer versions of VS. We will need > >to adjust the behavior of GLOBAL_REMOVE_IF_UNREFERENCED based on VS > >version as well. > > > >We can not define GLOBAL_REMOVE_IF_UNREFERENCED to static. We also use > >this macro for globals that are required to be exported from a library. > >So static should be added to the globals that are not exported. > > > >The challenge is that older versions of VS require > >GLOBAL_REMOVE_IF_UNREFERENCED to be mapped to __declspec(selectany) and > >static can not be combined with __declspec(selectany). > > > >Mike > > > >> -----Original Message----- > >> From: Gao, Liming > >> Sent: Thursday, May 25, 2017 10:21 PM > >> To: Kinney, Michael D ; afish@apple.com; > >Laszlo Ersek > >> ; Kinney, Michael D > >> Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, > >> Jeff ; Felix Poludov ; Ard > >> Biesheuvel > >> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: > >> Fix duplicate symbol > >> > >> Mike: > >> I remember community suggests to use VS /Gw option to remove the > >global data, > >> and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or > >static. > >> > >> Thanks > >> Liming > >> >-----Original Message----- > >> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > >> >Of Kinney, Michael D > >> >Sent: Friday, May 26, 2017 6:42 AM > >> >To: afish@apple.com; Laszlo Ersek ; Kinney, > >> >Michael > >D > >> > > >> >Cc: Wu, Hao A ; edk2-devel@lists.01.org; Fan, > >> >Jeff ; Felix Poludov ; Ard > >> >Biesheuvel > >> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: > >Fix > >> >duplicate symbol > >> > > >> >Andrew, > >> > > >> >The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED > >was > >> >added referred to __declspec( selectany ) as putting the symbol into > >> >its > >own > >> >comdat, so it was then available to be optimized away with the use > >> >of > >OPT:REF. > >> > > >> >I think it is time to re-evaluate the VS optimizers to see if they > >> >can optimize away global variables without being decorated with__decl= spec( > selectany ). > >If > >> >we can remove __declspec( selectany ), then we have a path to use > >STATIC > >> >properly to hide global variables that are not declared as extern in > >> >the > >library > >> >class. > >> > > >> >I will investigate some more. > >> > > >> >Mike > >> > > >> >From: afish@apple.com [mailto:afish@apple.com] > >> >Sent: Thursday, May 25, 2017 2:26 PM > >> >To: Laszlo Ersek > >> >Cc: Ard Biesheuvel ; Wu, Hao A > >> >; edk2-devel@lists.01.org; Felix Poludov > >> >; Kinney, Michael D ; > >> >Fan, Jeff > >> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: > >Fix > >> >duplicate symbol > >> > > >> > > >> >On May 25, 2017, at 2:02 PM, Laszlo Ersek > >> >> wrote: > >> > > >> >On 05/25/17 22:37, Andrew Fish wrote: > >> > > >> > > >> > > >> >On May 25, 2017, at 1:28 PM, Laszlo Ersek > >> >> wrote: > >> > > >> >On 05/25/17 22:11, Ard Biesheuvel wrote: > >> > > >> >On 25 May 2017 at 13:06, Kinney, Michael D > >> >> > >wrote: > >> > > >> >Laszlo and Andrew, > >> > > >> >With the information that has been collected on this thread, I still > >> >think this patch in its original form is a good change to resolve > >> >the this one specific duplicate symbol issue for all tool chains. > >> >'static' can not be mixed with GLOBAL_REMOVE_IF_UNREFERENCED for > >> >MSFT tool chains, so renaming the global variable is the easiest way > >> >to remove the duplicate. > >> > > >> >GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it > >> >was Felix who reported on this recently? > >> > > >> >STATIC is really the only sensible way to deal with this for symbols > >> >that are only referenced by a single compilation unit. > >> > > >> > > >> >I will continue to work on ways to detect duplicate symbols for all > >> >tool chains and will enter a Bugzilla issue to for that feature. > >> > > >> >In addition, the idea of detecting if a library is exporting more > >> >than the library class defines is another good feature to consider > >> >and I will enter a Bugzilla issue for that one as well. > >> > > >> >If we can find ways to both restrict the symbols exported by a > >> >library and strip all symbols that are unused, then we can have > >> >additional Bugzilla issues to perform that clean up on each library > >> >instance that is exporting more than the library class. > >> > > >> >A static library is nothing more than an archive containing a > >> >collection of object files. Sadly, that implies that we cannot > >> >distinguish between symbols that may only be referenced by other > >> >objects in the same static library and symbols that are exported to > >> >the library client. > >> > > >> >Do we know for a fact that, with /OPT:REF, VS does not strip unused > >> >*static* variables and functions? > >> > > >> >I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED > >> >with STATIC in this case would lead to a size increase? > >> > > >> >If that's the case, then I'm fine if we go ahead with this patch, > >> >I'd just like to request that Mike please file some of those BZs, > >> >and please reference them from the commit message (as the longer > >> >term solution), before committing the patch. > >> > > >> >Clang will warn if you have unused static variables when warnings > >> >are > >cranked > >> >up. > >> > > >> >~/work/Compiler>cat static.c > >> >static unsigned char gTest[] =3D { 42 }; > >> > > >> >static int test () > >> >{ > >> > return 1; > >> >} > >> > > >> >int main () > >> >{ > >> > return 0; > >> >} > >> >~/work/Compiler>clang -Os static.c -Wall > >> >static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable] > >> >static unsigned char gTest[] =3D { 42 }; > >> > ^ > >> >static.c:3:12: warning: unused function 'test' [-Wunused-function] > >> >static int test () > >> > ^ > >> >2 warnings generated. > >> > > >> >Sorry, my question was imprecise. > >> > > >> >Assume there is a public library function ("external linkage") that > >> >calls a static function in the same library instance and uses a > >> >static variable in the same library instance. Then this library > >> >instance is linked into a driver, but the driver never actually > >> >calls the extern function -- so the static variable and the static > >> >function too become useless. > >> > > >> >In this case, will /OPT:REF remove the static variable and the > >> >static function too? > >> > > >> >It seems counter-intuitive to me that an internal-only function or > >> >an internal-only variable has to be declared extern (via > >> >GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link > >> >time, if it is never referenced (transitively). > >> > > >> > > >> >Laszlo, > >> > > >> >I agree. The LLVM LTO does not have an issue "doing the right thing". > >Seems > >> >like static is also more of a compile time concept vs a link time > >> >(global > >> >optimization) kind of thing? > >> > > >> >Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec( > >> >selectany ) I would guess maybe it has more to due with supporting > >> >old non standard header files that can't change without > >breaking > >> >compatibility. > >> > > >> >MSDN on __declspec( selectany ) : > >> >A global data item can normally be initialized only once in an EXE > >> >or DLL > >> project. > >> >selectany can be used in initializing global data defined by > >> >headers, when > >the > >> >same header appears in more than one source file. selectany is > >> >available in both the C and C++ compilers. > >> > > >> >Thanks, > >> > > >> >Andrew Fish > >> > > >> > > >> > > >> >Thanks > >> >Laszlo > >> >_______________________________________________ > >> >edk2-devel mailing list > >> >edk2-devel@lists.01.org > >> >https://lists.01.org/mailman/listinfo/edk2-devel > >> > > >> >_______________________________________________ > >> >edk2-devel mailing list > >> >edk2-devel@lists.01.org > >> >https://lists.01.org/mailman/listinfo/edk2-devel >=20 > Please consider the environment before printing this email. >=20 > The information contained in this message may be confidential and proprie= tary to > American Megatrends, Inc. This communication is intended to be read only= by the > individual or entity to whom it is addressed or by their designee. If the= reader > of this message is not the intended recipient, you are on notice that any > distribution of this message, in any form, is strictly prohibited. Pleas= e > promptly notify the sender by reply e-mail or by telephone at 770-246-860= 0, and > then delete or destroy all copies of the transmission.