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

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

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