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:[email protected]] > Sent: Thursday, December 7, 2017 7:41 PM > To: Laszlo Ersek <[email protected]> > Cc: Gao, Liming <[email protected]>; [email protected]; Kinney, > Michael D <[email protected]>; Wu, Hao A > <[email protected]>; Andrew Fish <[email protected]>; Jeff Fan > <[email protected]> > Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix > duplicate symbol > > On 7 December 2017 at 11:18, Laszlo Ersek <[email protected]> wrote: > > On 12/07/17 09:46, Ard Biesheuvel wrote: > >> On 7 December 2017 at 07:48, Liming Gao <[email protected]> wrote: > >>> From: Michael Kinney <[email protected]> > >>> > >>> 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 <[email protected]> > > > > (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 <[email protected]> > >>> Cc: Jeff Fan <[email protected]> > >>> Cc: Hao Wu <[email protected]> > >>> Cc: Laszlo Ersek <[email protected]> > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Michael D Kinney <[email protected]> > >>> Reviewed-by: Jeff Fan <[email protected]> > >>> --- > >>> .../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 > >>> [email protected] > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >>> GitPatchExtractor 1.1 > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> [email protected] > >>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
