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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Felix,

Yes.  I agree.  I will work on a Bugzilla issue for this topic
and I prefer the idea of updating Base.h to check _MSC_VER value.

The one challenge is that 'static' could be added in front of
GLOBAL_REMOVE_IF_UNREFERENCED, and it will build correctly with
VS2012 and newer, but would fail with older VS versions that 
require __declspec(selectany) for size optimization.

Thanks,

Mike

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, May 26, 2017 3:12 PM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com; Laszlo Ersek <lersek@redhat.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> 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.