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

[Laszlo Ersek <lersek@redhat.com>]

On 05/25/17 22:37, Andrew Fish wrote:
> 
>> On May 25, 2017, at 1:28 PM, Laszlo Ersek <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> 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).

Thanks
Laszlo