On 7 December 2017 at 11:18, Laszlo Ersek <ler...@redhat.com> wrote:
> On 12/07/17 09:46, Ard Biesheuvel wrote:
>> On 7 December 2017 at 07:48, Liming Gao <liming....@intel.com> wrote:
>>> From: Michael Kinney <michael.d.kin...@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 <ler...@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 <af...@apple.com>
>>> Cc: Jeff Fan <jeff....@intel.com>
>>> Cc: Hao Wu <hao.a...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Michael D Kinney <michael.d.kin...@intel.com>
>>> Reviewed-by: Jeff Fan <jeff....@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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
