public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kinney, Michael D" <michael.d.kinney@intel.com>
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" <edk2-devel@lists.01.org>,
	"afish@apple.com" <afish@apple.com>,
	"Fan, Jeff" <jeff.fan@intel.com>
Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
Date: Thu, 25 May 2017 00:38:18 +0000	[thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F57D177C2D@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <CAKv+Gu83LfXTNiWE97=ggmiGuOvRXk56QThxj2PVf_aXDnqJ3A@mail.gmail.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

  reply	other threads:[~2017-05-25  0:38 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-23 23:21 [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol Michael Kinney
2017-05-23 23:25 ` Andrew Fish
2017-05-24  0:27   ` Kinney, Michael D
2017-05-24  8:48     ` Laszlo Ersek
2017-05-24 11:52       ` Ard Biesheuvel
2017-05-24 20:18         ` Kinney, Michael D
2017-05-24 21:44           ` Ard Biesheuvel
2017-05-25  0:38             ` Kinney, Michael D [this message]
2017-05-25  1:47               ` Kinney, Michael D
2017-05-25 16:08                 ` Laszlo Ersek
2017-05-25 16:14                   ` Andrew Fish
2017-05-25 17:38                   ` Kinney, Michael D
2017-05-25 18:06                     ` Laszlo Ersek
2017-05-25 19:55                       ` Ard Biesheuvel
2017-05-25 20:01                         ` Laszlo Ersek
2017-05-25 19:57                       ` Kinney, Michael D
2017-05-25 20:10                         ` Laszlo Ersek
2017-05-25 22:47                           ` Kinney, Michael D
2017-05-26  5:29                             ` Gao, Liming
2017-05-26  9:05                               ` Laszlo Ersek
2017-05-25 16:01           ` Laszlo Ersek
2017-05-24  2:47 ` Fan, Jeff
2017-05-25 20:06   ` Kinney, Michael D
2017-05-25 20:11     ` Ard Biesheuvel
2017-05-25 20:28       ` Laszlo Ersek
2017-05-25 20:37         ` Andrew Fish
2017-05-25 21:02           ` Laszlo Ersek
2017-05-25 21:25             ` Andrew Fish
2017-05-25 22:42               ` Kinney, Michael D
2017-05-26  5:21                 ` Gao, Liming
2017-05-26  6:20                   ` Kinney, Michael D
2017-05-26  8:41                     ` Gao, Liming
2017-05-26 22:11                       ` Felix Poludov
2017-05-26 23:06                         ` Kinney, Michael D
2017-05-27 12:27                           ` Ard Biesheuvel
2017-05-29 10:21                             ` Laszlo Ersek
  -- strict thread matches above, loose matches on Subject: below --
2017-12-07  7:48 Liming Gao
2017-12-07  7:55 ` Ni, Ruiyu
2017-12-07  7:55 ` Ni, Ruiyu
2017-12-07  8:24 ` Wu, Hao A
2017-12-07  8:46 ` Ard Biesheuvel
2017-12-07 11:18   ` Laszlo Ersek
2017-12-07 11:41     ` Ard Biesheuvel
2017-12-07 14:19       ` Gao, Liming
2017-12-07 14:21         ` Ard Biesheuvel
2017-12-07 16:52   ` Kinney, Michael D

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E92EE9817A31E24EB0585FDF735412F57D177C2D@ORSMSX113.amr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox