From: "Gao, Liming" <liming.gao@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Wu, Hao A" <hao.a.wu@intel.com>, Andrew Fish <afish@apple.com>
Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
Date: Thu, 7 Dec 2017 14:19:20 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E18C020@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <CAKv+Gu-UdXF7uTRiwX9mbsC9LQJkdg9biDk6gagBBTeYH-LEhA@mail.gmail.com>
Ard and Ersek:
On VS, static may make debug become hard. And, STATIC + GLOBAL_REMOVE_IF_UNREFERENCED may failure on old VS before VS supports /Gw option. So, I don't add static here.
I would like to separate static usage topic. We could discuss more and summary the rule on how to use static in code. But for this build failure issue, I prefer to fix it with this solution first. Is it OK to you?
Thanks
Liming
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, December 7, 2017 7:41 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Andrew Fish <afish@apple.com>; Jeff Fan <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
>
> On 7 December 2017 at 11:18, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 12/07/17 09:46, Ard Biesheuvel wrote:
> >> On 7 December 2017 at 07:48, Liming Gao <liming.gao@intel.com> wrote:
> >>> From: Michael Kinney <michael.d.kinney@intel.com>
> >>>
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=573
> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=796
> >>>
> >>> The same issue is reported again by GCC. Resend this patch again.
> >>> This patch renames the duplicated function name to fix it.
> >>>
> >>> The SecPeiDebugAgentLib uses the global variable
> >>> mMemoryDiscoveredNotifyList for a PPI notification on
> >>> the Memory Discovered PPI. This same variable name is
> >>> used in the DxeIplPeim for the same PPI notification.
> >>>
> >>> The XCODE5 tool chain detects this duplicate symbol
> >>> when the OVMF platform is built with the flag
> >>> -D SOURCE_DEBUG_ENABLE.
> >>>
> >>> The fix is to rename this global variable in the
> >>> SecPeiDebugAgentLib library.
> >>>
> >>
> >> No, the fix is to make it STATIC unless it *needs* to be a global.
> >> Is that the case here?
> >
> > I agree with you (of course), but Mike explained earlier (if I recall
> > correctly -- and perhaps you remember too) that giving internal linkage
> > to global variables (i.e., making them STATIC) messes either with
> > debuggability under VS, or else defeats "GLOBAL_REMOVE_IF_UNREFERENCED".
> > (I'm not sure which one of the two.)
> >
>
> That doesn't quite ring a bell, but if that is the case, it deserves a mention.
>
> Note that STATIC variables are also removed when unreferenced (but may
> require a warning override like we have for GCC if it is only used
> from DEBUG () code). In any case, polluting the global namespace in a
> heterogeneous project like EDK2 is something that should only be done
> with good reason IMO.
>
> > So, I've settled on considering "extern by default" just another
> > peculiarity of edk2. *shrug* I'm just glad -fno-common catches bugs like
> > this!
> >
>
> Well, the thing is, external linkage defeats optimizations in the
> compiler, and also prevents it from emitting the variable into a
> read-only section even if it would otherwise be able to infer from the
> usage that the variable is never modified.
>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > (Obviously I'm not trying to dismiss your objection at all! Just stating
> > my view. If the patch is changed to STATIC, I'll R-b that version *more
> > happily* than this one.)
> >
> > Thanks!
> > Laszlo
> >
> >>
> >>
> >>> Cc: Andrew Fish <afish@apple.com>
> >>> Cc: Jeff Fan <jeff.fan@intel.com>
> >>> Cc: Hao Wu <hao.a.wu@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> >>> Reviewed-by: Jeff Fan <jeff.fan@intel.com>
> >>> ---
> >>> .../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> >>> index b717e33..9f5223a 100644
> >>> --- a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> >>> +++ b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> >>> @@ -32,7 +32,7 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_PPI_DESCRIPTOR mVectorHandoffInf
> >>> }
> >>> };
> >>>
> >>> -GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList[1] = {
> >>> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mDebugAgentMemoryDiscoveredNotifyList[1] = {
> >>> {
> >>> (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> >>> &gEfiPeiMemoryDiscoveredPpiGuid,
> >>> @@ -554,7 +554,7 @@ InitializeDebugAgent (
> >>> // Register for a callback once memory has been initialized.
> >>> // If memery has been ready, the callback funtion will be invoked immediately
> >>> //
> >>> - Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0]);
> >>> + Status = PeiServicesNotifyPpi (&mDebugAgentMemoryDiscoveredNotifyList[0]);
> >>> if (EFI_ERROR (Status)) {
> >>> DEBUG ((EFI_D_ERROR, "DebugAgent: Failed to register memory discovered callback function!\n"));
> >>> CpuDeadLoop ();
> >>> --
> >>> 2.6.3.windows.1
> >>>
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>> GitPatchExtractor 1.1
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >
next prev parent reply other threads:[~2017-12-07 14:14 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-07 7:48 [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol 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 [this message]
2017-12-07 14:21 ` Ard Biesheuvel
2017-12-07 16:52 ` Kinney, Michael D
-- strict thread matches above, loose matches on Subject: below --
2017-05-23 23:21 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
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
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=4A89E2EF3DFEDB4C8BFDE51014F606A14E18C020@SHSMSX104.ccr.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