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

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

Andrew,

I think I have found an alternate fix for this XCODE5 specific
build failure.  Since there appears to be a difference in the 
linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
the 'ld' command line options used in XCODE5 tool chain in
tools_def.txt.

There is a flag set call '-all_load'.  The description of this
flag is 'Loads all members of static archive libraries.'.

I tried removing this flag from the XCODE5 specific SLINK_FLAGS
and DLINK_FLAGS statements in tools_def.txt, and the duplicate
symbol build failure is no longer present.  I am able to build
and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.

This seems to make XCODE5 linker behavior match the MSFT and GCC
linker behavior.

Do you know why '-all_load' is used in XCODE5 and what impacts
there may be from removing it?

Best regards,

Mike


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, May 24, 2017 5:38 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; 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
> 
> 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