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

[Laszlo Ersek <lersek@redhat.com>]

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

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!

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