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

["Gao, Liming" <liming.gao@intel.com>]

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