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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 24 May 2017 at 13:18, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Laszlo,
>
> I agree with the request to add 'static' to the variable declaration
> in the SecPeiDebugAgentLib.  The variable name change will be retained
> because the same symbol name can still be confusing when debugging.
>
> The part that is more concerning is why this duplicate symbol reference
> was not triggered by MSFT or GCC tool chain families, so I did some
> more investigation from the MSFT side this morning.
>
> My first thought was that the optimizer in the compiler/linker was
> optimizing away the duplicate symbol before the final link stage,
> so I tried -b NOOPT builds.  That also did not trigger a duplicate
> symbol error and the MAP file contains only the single reference to
> _mMemoryDiscoveredNotifyList from the DxeIpl.
>
>   0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj
>
> I then evaluated what functions in the DebugAgentLib the DxeIpl
> is using.  It turns out that it is only using a single function
> SaveAndSetDebugTimerInterrupt() that is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
>
> The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
>
> Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> linker only pulls in symbols from the obj in the DebugAgentLib that
> contains the one SaveAndSetDebugTimerInterrupt() function.  The symbols
> from the other objs in the DebugAgentLib are not included in the final
> DxeIpl PE/COFF image.
>
>   0001:000081b0       _InitializeDebugTimer      00008410 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:00008330       _IsDebugTimerTimeout       00008590 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f   SecPeiDebugAgentLib:DebugTimer.obj
>
> So it appears that even with -b NOOPT builds, the linker is
> filtering out unreferenced objs which explains why the duplicate
> symbol error is not triggered.
>

It does not need to filter out what it never considered for inclusion
in the first place. That is why it is called a library: only objects
that satisfy some as yet unresolved symbol reference will get pulled
in.

> The XCODE5 tool chain appears to be evaluating symbols
> across all objs in a lib and detects a duplicate symbol.
>
> The question is what is the required linker behavior for EDK II
> builds?  Require library filtering at obj level, or require no
> duplicates across all objs in all libs?
>

Require no *externally visible* duplicates at all. That is really the
only scaleable solution. Libraries declare their externally visible
symbols in their .h file, and ideally, nothing else should be exposed.
I know this is infeasible in the general case, due to the fact that
libraries typically  consist of several objects, but it should be the
goal not to export anything if we can avoid it. Note that this applies
to functions as well as variables