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

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

Ard,

I agree that it would be good practice for a library instance to 
only have the public interfaces(functions/data) that are declared
in the library class .h file.

I would be useful to have a report that showed the extra interfaces.

One additional constraint I just found with the MSFT tool chains 
involves the GLOBAL_REMOVE_IF_UNREFERENCED.  This macro can not be
combined with static.  So the recommendation in the thread to add
static to mMemoryDiscoveredNotifyList in SecPeiDebugAgentLib
breaks the build.

d:\work\github\tianocore\edk2\SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c(35): error C2496: 'mDebugAgentMemoryDiscoveredNotifyList': 'selectany' can only be applied to data items with external linkage

The MSFT tool chains depend on the following #define in order for
The optimizer to remove global variables that are not used.  This
is a valuable size optimization for libraries that may be sparsely 
used by modules.

  #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany)

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, May 24, 2017 2:44 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-
> devel@lists.01.org; afish@apple.com; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> 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