Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Felix Poludov <Felixp@ami.com>]

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 Biesheuvel
Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate 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 collect 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 compressed size doesn't increase big.

1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Space Information SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%Full] 1753088 total, 1099872 used, 653216 free DXEFV [39%Full] 10485760 total, 4099344 used, 6386416 free PEIFV [18%Full] 917504 total, 172072 used, 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] 1753088 total, 1105992 used, 647096 free DXEFV [39%Full] 10485760 total, 4154480 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 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

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 <liming.gao@intel.com>; afish@apple.com; Laszlo Ersek
><lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>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 <michael.d.kinney@intel.com>; afish@apple.com;
>Laszlo Ersek
>> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
>> Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> 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 <lersek@redhat.com>; Kinney,
>> >Michael
>D
>> ><michael.d.kinney@intel.com>
>> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
>> >Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
>> >Biesheuvel <ard.biesheuvel@linaro.org>
>> >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__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 <lersek@redhat.com>
>> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
>> ><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
>> ><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> >Fan, Jeff <jeff.fan@intel.com>
>> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> >duplicate symbol
>> >
>> >
>> >On May 25, 2017, at 2:02 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:37, Andrew Fish wrote:
>> >
>> >
>> >
>> >On May 25, 2017, at 1:28 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:11, Ard Biesheuvel wrote:
>> >
>> >On 25 May 2017 at 13:06, Kinney, Michael D
>> ><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>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[] = { 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[] = { 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<mailto: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 proprietary 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.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.