On 7 December 2017 at 14:19, Gao, Liming <[email protected]> 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:[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
