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 DD4E721A08D82 for ; Thu, 25 May 2017 15:42:10 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga105.fm.intel.com with ESMTP; 25 May 2017 15:42:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,393,1491289200"; d="scan'208,217";a="1134813954" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by orsmga001.jf.intel.com with ESMTP; 25 May 2017 15:42:10 -0700 Received: from orsmsx115.amr.corp.intel.com (10.22.240.11) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 25 May 2017 15:42:09 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.59]) by ORSMSX115.amr.corp.intel.com ([169.254.4.44]) with mapi id 14.03.0319.002; Thu, 25 May 2017 15:42:09 -0700 From: "Kinney, Michael D" To: "afish@apple.com" , Laszlo Ersek , "Kinney, Michael D" CC: Ard Biesheuvel , "Wu, Hao A" , "edk2-devel@lists.01.org" , Felix Poludov , "Fan, Jeff" Thread-Topic: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Thread-Index: AQHS1BtKrH1XaxmZJEaVg215IKXpe6IDPWgAgAI9aZCAAHhigIAABMyAgAACfgCAAAcvgIAABliA//+ealA= Date: Thu, 25 May 2017 22:42:09 +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> In-Reply-To: <340C28EC-60CF-4390-A8AD-25F9DA22538F@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjVjYTEzMTMtYWFhYy00YmUzLTk2NjYtZGI2ZGY4ODQ0MDQxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlNJWDJobWJGYXZ1U1JpNzFqcHkxaWdxUWxnVnNZa2FnMCtuMGgzNk4ybW89In0= 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 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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: Thu, 25 May 2017 22:42:11 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was added ref= erred 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 opti= mize away global variables without being decorated with__declspec( selectan= y ). 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, Micha= el D ; Fan, Jeff Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix du= plicate 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 crank= ed 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 he= ader 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 p= roject. selectany can be used in initializing global data defined by header= s, 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