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:fel...@ami.com] > Sent: Friday, May 26, 2017 3:12 PM > To: Gao, Liming <liming....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; af...@apple.com; Laszlo Ersek > <ler...@redhat.com> > Cc: Wu, Hao A <hao.a...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff > <jeff....@intel.com>; Ard Biesheuvel <ard.biesheu...@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....@intel.com] > Sent: Friday, May 26, 2017 4:42 AM > To: Kinney, Michael D; af...@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....@intel.com>; af...@apple.com; Laszlo Ersek > ><ler...@redhat.com>; Kinney, Michael D <michael.d.kin...@intel.com> > >Cc: Wu, Hao A <hao.a...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff > ><jeff....@intel.com>; Felix Poludov <fel...@ami.com>; Ard Biesheuvel > ><ard.biesheu...@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.kin...@intel.com>; af...@apple.com; > >Laszlo Ersek > >> <ler...@redhat.com>; Kinney, Michael D <michael.d.kin...@intel.com> > >> Cc: Wu, Hao A <hao.a...@intel.com>; edk2-devel@lists.01.org; Fan, > >> Jeff <jeff....@intel.com>; Felix Poludov <fel...@ami.com>; Ard > >> Biesheuvel <ard.biesheu...@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-boun...@lists.01.org] On Behalf > >> >Of Kinney, Michael D > >> >Sent: Friday, May 26, 2017 6:42 AM > >> >To: af...@apple.com; Laszlo Ersek <ler...@redhat.com>; Kinney, > >> >Michael > >D > >> ><michael.d.kin...@intel.com> > >> >Cc: Wu, Hao A <hao.a...@intel.com>; edk2-devel@lists.01.org; Fan, > >> >Jeff <jeff....@intel.com>; Felix Poludov <fel...@ami.com>; Ard > >> >Biesheuvel <ard.biesheu...@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: af...@apple.com [mailto:af...@apple.com] > >> >Sent: Thursday, May 25, 2017 2:26 PM > >> >To: Laszlo Ersek <ler...@redhat.com> > >> >Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Wu, Hao A > >> ><hao.a...@intel.com>; edk2-devel@lists.01.org; Felix Poludov > >> ><fel...@ami.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > >> >Fan, Jeff <jeff....@intel.com> > >> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: > >Fix > >> >duplicate symbol > >> > > >> > > >> >On May 25, 2017, at 2:02 PM, Laszlo Ersek > >> ><ler...@redhat.com<mailto:ler...@redhat.com>> wrote: > >> > > >> >On 05/25/17 22:37, Andrew Fish wrote: > >> > > >> > > >> > > >> >On May 25, 2017, at 1:28 PM, Laszlo Ersek > >> ><ler...@redhat.com<mailto:ler...@redhat.com>> wrote: > >> > > >> >On 05/25/17 22:11, Ard Biesheuvel wrote: > >> > > >> >On 25 May 2017 at 13:06, Kinney, Michael D > >> ><michael.d.kin...@intel.com<mailto:michael.d.kin...@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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
