From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 6CF7321A02909 for ; Thu, 25 May 2017 23:20:21 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 May 2017 23:20:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,395,1491289200"; d="scan'208";a="107220862" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by fmsmga006.fm.intel.com with ESMTP; 25 May 2017 23:20:20 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.59]) by ORSMSX109.amr.corp.intel.com ([169.254.11.138]) with mapi id 14.03.0319.002; Thu, 25 May 2017 23:20:19 -0700 From: "Kinney, Michael D" 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 Thread-Topic: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Thread-Index: AQHS1BtKrH1XaxmZJEaVg215IKXpe6IDPWgAgAI9aZCAAHhigIAABMyAgAACfgCAAAcvgIAABliA//+ealCAAOaEAP//mRnw Date: Fri, 26 May 2017 06:20:19 +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> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D734727@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjNiMTgxODEtOWI3NS00NDMzLTk3MDAtMjBhMWQwYzlhMjcxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImhpcU5iaFBmd2ViekM4VVo4cW9INmVKdncrU3ZFbXBLOGRWZ3NYUElqaWM9In0= 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: Fri, 26 May 2017 06:20:21 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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=20 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; Lasz= lo 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 >=20 > 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. >=20 > 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 OP= T:REF. > > > >I think it is time to re-evaluate the VS optimizers to see if they can o= ptimize > >away global variables without being decorated with__declspec( 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 cr= anked > >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". Se= ems > >like static is also more of a compile time concept vs a link time (globa= l > >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 break= ing > >compatibility. > > > >MSDN on __declspec( selectany ) : > >A global data item can normally be initialized only once in an EXE or DL= L > project. > >selectany can be used in initializing global data defined by headers, wh= en 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