From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from atlmailgw1.ami.com (atlmailgw1.ami.com [63.147.10.40]) (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 CDB8521A6F107 for ; Fri, 26 May 2017 15:12:09 -0700 (PDT) X-AuditID: ac1060b2-a5bff70000000c96-b7-5928a830b0d3 Received: from atlms2.us.megatrends.com (atlms2.us.megatrends.com [172.16.96.152]) (using TLS with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by (Symantec Messaging Gateway) with SMTP id CF.DD.03222.038A8295; Fri, 26 May 2017 18:12:03 -0400 (EDT) Received: from ATLMS1.us.megatrends.com ([fe80::8c55:daf0:ef05:5605]) by atlms2.us.megatrends.com ([fe80::29dc:a91e:ea0c:cdeb%12]) with mapi id 14.03.0123.003; Fri, 26 May 2017 18:11:59 -0400 From: Felix Poludov To: "Gao, Liming" , "Kinney, Michael D" , "afish@apple.com" , "Laszlo Ersek" CC: "Wu, Hao A" , "edk2-devel@lists.01.org" , "Fan, Jeff" , Ard Biesheuvel Thread-Topic: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Thread-Index: AQHS1ZMMc7deloERrEG1g2Okk8APUaICQfMAgAK0lgCAAAE1gIAABMyAgAACfgCAAAcvgIAABliAgAAVZ4CAAPUNgP//ivWAgAChNlCAAOol8A== Date: Fri, 26 May 2017 22:11:59 +0000 Message-ID: <9333E191E0D52B4999CE63A99BA663A00302B22075@atlms1.us.megatrends.com> 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> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D734868@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.16.99.93] content-transfer-encoding: quoted-printable MIME-Version: 1.0 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrGKsWRmVeSWpSXmKPExsWyRiBhhq7JCo1Ig5aTXBbTbz9msfj/YTej xZ5DR5ktrt76xWRxcv0SRotlx3awWKy4t4HdoqPjH5MDh8fWkz/YPBbvecnkcefaHjaP7tn/ WDze77vKFsAa1cBok5iXl1+SWJKqkJJanGyrFFCUWZaYXKmkkJliq2SopFCQk5icmpuaV2Kr lFhQkJqXomTHpYABbIDKMvMUUvOS81My89JtlTyD/XUtLEwtdQ2V7EIyUhUy89Lyi3ITSzLz 8xSS8/NKgKpTU4CiCgldnBkzrx5kKzgdWTF1dQ9rA+Ndzy5GTg4JAROJTUfOsncxcnEICSxm klh35AkjhHOYUeJf1y9GkCo2ARWJTWcvMIMkRASWM0q0/LjHAuIwC6xmlOi59RysSlggWuL9 /LnsILaIQIzEts+dUHadxMxfD9hAbBYBVYmpX14BxTk4eAUCJfb35YGEhQRms0ncPB8DEuYU CJG49soIJMwoICbx/dQaJhCbWUBc4taT+UwQVwtILNlznhnCFpV4+fgfK4StILHlPcRWZgEd iQW7P7FB2NoSyxa+BqvnFRCUODnzCcsERtFZSMbOQtIyC0nLLCQtCxhZVjEKJZbk5CZm5qSX G+ol5mbqJefnbmKEJJtNOxhbLpofYhTgYFTi4Z02WyNSiDWxrLgy9xCjBAezkgjvtylAId6U xMqq1KL8+KLSnNTiQ4xOwECZyCzFDYovYAKINzYwkBKFcQxNzEzMjcwNLU3MjY2VxHlLmPZH CAmkAxNSdmpqQWoRzBAmDk6pBsZ1z06LbGC56pKb33qbg8kqQrP2lu7ddh2rhZyhc7R4OTcv mHL3VmRuiPv1SZoMLcvKFnQfWN12sCrIYStH3PvYo0dmte0Oc2lcuchUR467756e7sRdplMs dRZ4P1P+eJNpLZ91fhHDyT3PT6lMi72gOLOBa/qN/cqNKwqfilW97ekqc/3Bf1mJpTgj0VCL uag4EQCSVLHpWQMAAA== 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 22:12:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Another option to support older VS tool chains is to define GLOBAL_REMOVE_IF= _UNREFERENCED as __declspec(selectany) only for these tool chains. One way to do it is to use _MSC_VER macro: #if _MSC_VER < .... #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany) #endif Alternatively GLOBAL_REMOVE_IF_UNREFERENCED can be defined for specific tool= chains in the tools_def.txt using /D compiler switch. -----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 Bieshe= uvel Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix dup= licate symbol Mike: Yes. /Gw option is added since VS2013. The older VS version can't use this= option. I suggest we always define GLOBAL_REMOVE_IF_UNREFERENCED as empty,= and drop this size optimization for the older version MS compiler. I collec= t its size of OvmfIa32X64 DEBUG tip with VS2015 tool chain on. After define= it as empty, DXE Raw size increases ~55K, but PEI raw size and the compress= ed size doesn't increase big. 1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Space I= nformation SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMP= ACT [62%Full] 1753088 total, 1099872 used, 653216 free DXEFV [39%Full] 10485= 760 total, 4099344 used, 6386416 free PEIFV [18%Full] 917504 total, 172072 u= sed, 745432 free 2. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty. FV Space Information SECFV= [10%Full] 212992 total, 22912 used, 190080 free FVMAIN_COMPACT [63%Full] 17= 53088 total, 1105992 used, 647096 free DXEFV [39%Full] 10485760 total, 41544= 80 used, 6331280 free PEIFV [18%Full] 917504 total, 173448 used, 744056 free FVMAIN_COMPACT +6120 DXEFV + 55136 PEIFV + 1376 3. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty and append /Gw option. FV S= pace Information. SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%Ful= l] 1753088 total, 1099552 used, 653536 free DXEFV [39%Full] 10485760 total,= 4097456 used, 6388304 free PEIFV [18%Full] 917504 total, 171944 used, 74556= 0 free FVMAIN_COMPACT -320 DXEFV -1888 PEIFV -128 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__declspe= c( 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 Please consider the environment before printing this email. The information contained in this message may be confidential and proprietar= y to American Megatrends, Inc. This communication is intended to be read on= ly 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 not= ice that any distribution of this message, in any form, is strictly prohibit= ed. Please promptly notify the sender by reply e-mail or by telephone at 77= 0-246-8600, and then delete or destroy all copies of the transmission.