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

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

On 7 December 2017 at 14:19, Gao, Liming <liming.gao@intel.com> wrote:
> 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?
>

Yes that is fine.

-- 
Ard.

>> -----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
>> >