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

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

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