[Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Michael Kinney]

https://bugzilla.tianocore.org/show_bug.cgi?id=573

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.

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

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

[Andrew Fish]

Mike,

Do the other compilers promote (or is that demote) to static? Would not making these lib globals, and private functions static solve this class of issue?

Thanks,

Andrew Fish

> On May 23, 2017, at 4:21 PM, Michael Kinney <michael.d.kinney@intel.com> wrote:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=573
> 
> 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.
> 
> 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>
> ---
> .../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
> 

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

[Kinney, Michael D]

Andrew,

I agree in this specific case, making the global variable static
should also resolve this issue.

In general, we do not make module global variables static, so the 
module global can be shared across multiple source files in the 
module implementation.

Not sure why this issue has not been seen with other tool chains.

Mike

> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Tuesday, May 23, 2017 4:26 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org; Fan, Jeff <jeff.fan@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
> 
> Mike,
> 
> Do the other compilers promote (or is that demote) to static? Would not making these
> lib globals, and private functions static solve this class of issue?
> 
> Thanks,
> 
> Andrew Fish
> 
> > On May 23, 2017, at 4:21 PM, Michael Kinney <michael.d.kinney@intel.com> wrote:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=573
> >
> > 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.
> >
> > 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>
> > ---
> > .../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
> >

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

[Laszlo Ersek]

CC Ard

On 05/24/17 02:27, Kinney, Michael D wrote:
> Andrew,
>
> I agree in this specific case, making the global variable static
> should also resolve this issue.
>
> In general, we do not make module global variables static, so the
> module global can be shared across multiple source files in the
> module implementation.

I think the default should be the reverse: give objects with static
storage duration ("global variables") internal linkage ("STATIC") by
default, and turn the linkage into external only if multiple source
files of the same module actually use the same object together. (In this
case the object will have to be declared in a module-internal header
file anyway.)

I grepped the tree for "mMemoryDiscoveredNotifyList", and there are more
instances, all exhibiting the same issue:

(1) MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
(2) QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c
(3) SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
(4) Vlv2TbltDevicePkg/PlatformPei/Platform.c

In each of these source files, the "mMemoryDiscoveredNotifyList"
variable
- has an initializer,
- is declared in file scope,
- has external linkage,
- has static storage duration,

thus the declaration qualifies as an "external definition" (of which
there may be at most one, for any given object, in the final linking).

In each of the four modules listed above, the
"mMemoryDiscoveredNotifyList" variable is only used in the same source
file that declares / defines the variable. Thus, the variable should be
made "STATIC" in every one of them.

> Not sure why this issue has not been seen with other tool chains.

I think it is either a gcc or a BaseTools (toolchain config) bug.

Namely, we faced a similar issue before: please refer to commit
214a3b79417f ("BaseTools GCC: avoid the use of COMMON symbols",
2015-12-08). In that commit, we made sure that gcc wouldn't silently
merge multiple external definitions (because that violated ISO C and
caused actual runtime bugs). As a result, uninitialized globals were no
longer placed in the COMMON section, but in the data section, and
multiple external definitions triggered a link editing error.

However, in this case we have initialized global variables, which are
*already* placed in the data section. I just built OVMF with
SOURCE_DEBUG_ENABLE, and verified the following:

(a)

> $ nm Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib \
>   | grep mMemoryDiscoveredNotifyList
> 0000000000000000 D mMemoryDiscoveredNotifyList
>
> $ nm Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib \
>   | grep mMemoryDiscoveredNotifyList
> 0000000000000000 D mMemoryDiscoveredNotifyList

The "D" mark means:
- "D" / "d": The symbol is in the initialized data section.
- uppercase: the symbol is global (external)

In other words, linking these two object archives together should fail.

(b)

> $ egrep 'SecPeiDebugAgentLib\.lib|DxeIpl\.lib' \
>   Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst
> .../Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib
> .../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib

This means that the build process will link them together. Indeed I can
find the following *successful* command in the build log (see the
reference to the above "static_library_files.lst" object list file):

> "gcc" \
>   -o \
>   Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll \
>   -nostdlib \
>   -Wl,-n,-q,--gc-sections \
>   -z common-page-size=0x20 \
>   -Wl,--entry,_ModuleEntryPoint \
>   -u _ModuleEntryPoint \
>   -Wl,-Map,Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.map \
>   -Wl,-melf_x86_64,--oformat=elf64-x86-64 \
>   -Wl,--start-group,@Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst,--end-group \
>   -g \
>   -fshort-wchar \
>   -fno-builtin \
>   -fno-strict-aliasing \
>   -Wall \
>   -Werror \
>   -Wno-array-bounds \
>   -ffunction-sections \
>   -fdata-sections \
>   -include AutoGen.h \
>   -fno-common \
>   -DSTRING_ARRAY_NAME=DxeIplStrings \
>   -m64 \
>   -fno-stack-protector \
>   "-DEFIAPI=__attribute__((ms_abi))" \
>   -maccumulate-outgoing-args \
>   -mno-red-zone \
>   -Wno-address \
>   -mcmodel=small \
>   -fpie \
>   -fno-asynchronous-unwind-tables \
>   -Wno-address \
>   -Os \
>   -mno-mmx \
>   -mno-sse \
>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>   -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \
>   -Wl,--script=BaseTools/Scripts/GccBase.lds

(c) Re-running the command manually succeeds.

(d) Just to see if "-fdata-sections" made any difference ("Place each
data item into its own section in the output file"), I removed it. Even
that way, the command succeeded.

I think this is either a gcc / GNU linker bug, or else our command line
(or linker script, "GccBase.lds") is buggy. This link command should not
succeed.

Anyway, regarding the patch, I think that all four declarations of
"mMemoryDiscoveredNotifyList" should be made STATIC instead.

Thanks,
Laszlo

>> -----Original Message-----
>> From: afish@apple.com [mailto:afish@apple.com]
>> Sent: Tuesday, May 23, 2017 4:26 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: edk2-devel@lists.01.org; Fan, Jeff <jeff.fan@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
>>
>> Mike,
>>
>> Do the other compilers promote (or is that demote) to static? Would not making these
>> lib globals, and private functions static solve this class of issue?
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>> On May 23, 2017, at 4:21 PM, Michael Kinney <michael.d.kinney@intel.com> wrote:
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=573
>>>
>>> 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.
>>>
>>> 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>
>>> ---
>>> .../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
>>>
>

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

[Ard Biesheuvel]

On 24 May 2017 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote:
> CC Ard
>
> On 05/24/17 02:27, Kinney, Michael D wrote:
>> Andrew,
>>
>> I agree in this specific case, making the global variable static
>> should also resolve this issue.
>>
>> In general, we do not make module global variables static, so the
>> module global can be shared across multiple source files in the
>> module implementation.
>
> I think the default should be the reverse: give objects with static
> storage duration ("global variables") internal linkage ("STATIC") by
> default, and turn the linkage into external only if multiple source
> files of the same module actually use the same object together. (In this
> case the object will have to be declared in a module-internal header
> file anyway.)
>

I strongly agree with Laszlo here. Omitting static defeats any kind of
optimization the compiler can perform when it knows it can see all
references to a variable, such as constant folding or emitting the
variable into .rodata if it does not observe any modifications to it.
In theory, this could have security implications as well as
performance implications (e.g., a variable which only gets set in
DEBUG builds)

> I grepped the tree for "mMemoryDiscoveredNotifyList", and there are more
> instances, all exhibiting the same issue:
>
> (1) MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> (2) QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c
> (3) SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> (4) Vlv2TbltDevicePkg/PlatformPei/Platform.c
>
> In each of these source files, the "mMemoryDiscoveredNotifyList"
> variable
> - has an initializer,
> - is declared in file scope,
> - has external linkage,
> - has static storage duration,
>
> thus the declaration qualifies as an "external definition" (of which
> there may be at most one, for any given object, in the final linking).
>
> In each of the four modules listed above, the
> "mMemoryDiscoveredNotifyList" variable is only used in the same source
> file that declares / defines the variable. Thus, the variable should be
> made "STATIC" in every one of them.
>
>> Not sure why this issue has not been seen with other tool chains.
>
> I think it is either a gcc or a BaseTools (toolchain config) bug.
>
> Namely, we faced a similar issue before: please refer to commit
> 214a3b79417f ("BaseTools GCC: avoid the use of COMMON symbols",
> 2015-12-08). In that commit, we made sure that gcc wouldn't silently
> merge multiple external definitions (because that violated ISO C and
> caused actual runtime bugs). As a result, uninitialized globals were no
> longer placed in the COMMON section, but in the data section, and
> multiple external definitions triggered a link editing error.
>
> However, in this case we have initialized global variables, which are
> *already* placed in the data section. I just built OVMF with
> SOURCE_DEBUG_ENABLE, and verified the following:
>
> (a)
>
>> $ nm Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib \
>>   | grep mMemoryDiscoveredNotifyList
>> 0000000000000000 D mMemoryDiscoveredNotifyList
>>
>> $ nm Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib \
>>   | grep mMemoryDiscoveredNotifyList
>> 0000000000000000 D mMemoryDiscoveredNotifyList
>
> The "D" mark means:
> - "D" / "d": The symbol is in the initialized data section.
> - uppercase: the symbol is global (external)
>
> In other words, linking these two object archives together should fail.
>

Yes, but given that they are part of a static library, objects are
only pulled in on-demand, and so if all references already happen to
be satisfied, the 'offending' object may never be loaded.

> (b)
>
>> $ egrep 'SecPeiDebugAgentLib\.lib|DxeIpl\.lib' \
>>   Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst
>> .../Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib/OUTPUT/SecPeiDebugAgentLib.lib
>> .../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib
>
> This means that the build process will link them together. Indeed I can
> find the following *successful* command in the build log (see the
> reference to the above "static_library_files.lst" object list file):
>
>> "gcc" \
>>   -o \
>>   Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll \
>>   -nostdlib \
>>   -Wl,-n,-q,--gc-sections \
>>   -z common-page-size=0x20 \
>>   -Wl,--entry,_ModuleEntryPoint \
>>   -u _ModuleEntryPoint \
>>   -Wl,-Map,Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.map \
>>   -Wl,-melf_x86_64,--oformat=elf64-x86-64 \
>>   -Wl,--start-group,@Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_library_files.lst,--end-group \
>>   -g \
>>   -fshort-wchar \
>>   -fno-builtin \
>>   -fno-strict-aliasing \
>>   -Wall \
>>   -Werror \
>>   -Wno-array-bounds \
>>   -ffunction-sections \
>>   -fdata-sections \
>>   -include AutoGen.h \
>>   -fno-common \
>>   -DSTRING_ARRAY_NAME=DxeIplStrings \
>>   -m64 \
>>   -fno-stack-protector \
>>   "-DEFIAPI=__attribute__((ms_abi))" \
>>   -maccumulate-outgoing-args \
>>   -mno-red-zone \
>>   -Wno-address \
>>   -mcmodel=small \
>>   -fpie \
>>   -fno-asynchronous-unwind-tables \
>>   -Wno-address \
>>   -Os \
>>   -mno-mmx \
>>   -mno-sse \
>>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
>>   -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \
>>   -Wl,--script=BaseTools/Scripts/GccBase.lds
>
> (c) Re-running the command manually succeeds.
>
> (d) Just to see if "-fdata-sections" made any difference ("Place each
> data item into its own section in the output file"), I removed it. Even
> that way, the command succeeded.
>
> I think this is either a gcc / GNU linker bug, or else our command line
> (or linker script, "GccBase.lds") is buggy. This link command should not
> succeed.
>

Depending on link order, this may succeed given the reasoning above.

> Anyway, regarding the patch, I think that all four declarations of
> "mMemoryDiscoveredNotifyList" should be made STATIC instead.
>

Yes, please. Especially when it comes to static libraries (due to the
flexible way we allow them to be specified in EDK2), I think it is
really poor hygiene to expose library internals to the library user. I
know we cannot always avoid it, but we should if we can imo.

-- 
Ard.


>>> -----Original Message-----
>>> From: afish@apple.com [mailto:afish@apple.com]
>>> Sent: Tuesday, May 23, 2017 4:26 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: edk2-devel@lists.01.org; Fan, Jeff <jeff.fan@intel.com>; Wu, Hao A
>>> <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>> Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
>>>
>>> Mike,
>>>
>>> Do the other compilers promote (or is that demote) to static? Would not making these
>>> lib globals, and private functions static solve this class of issue?
>>>
>>> Thanks,
>>>
>>> Andrew Fish
>>>
>>>> On May 23, 2017, at 4:21 PM, Michael Kinney <michael.d.kinney@intel.com> wrote:
>>>>
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=573
>>>>
>>>> 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.
>>>>
>>>> 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>
>>>> ---
>>>> .../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
>>>>
>>

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

[Kinney, Michael D]

Laszlo,

I agree with the request to add 'static' to the variable declaration
in the SecPeiDebugAgentLib.  The variable name change will be retained
because the same symbol name can still be confusing when debugging.

The part that is more concerning is why this duplicate symbol reference
was not triggered by MSFT or GCC tool chain families, so I did some
more investigation from the MSFT side this morning.

My first thought was that the optimizer in the compiler/linker was 
optimizing away the duplicate symbol before the final link stage,
so I tried -b NOOPT builds.  That also did not trigger a duplicate 
symbol error and the MAP file contains only the single reference to
_mMemoryDiscoveredNotifyList from the DxeIpl.

  0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj

I then evaluated what functions in the DebugAgentLib the DxeIpl
is using.  It turns out that it is only using a single function
SaveAndSetDebugTimerInterrupt() that is implemented in 

  SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c

The mMemoryDiscoveredNotifyList duplicate symbol is implemented in

  SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c

Even with a MSFT -b NOOPT build, when I look at the MAP file, the
linker only pulls in symbols from the obj in the DebugAgentLib that
contains the one SaveAndSetDebugTimerInterrupt() function.  The symbols
from the other objs in the DebugAgentLib are not included in the final
DxeIpl PE/COFF image.

  0001:000081b0       _InitializeDebugTimer      00008410 f   SecPeiDebugAgentLib:DebugTimer.obj
  0001:00008330       _IsDebugTimerTimeout       00008590 f   SecPeiDebugAgentLib:DebugTimer.obj
  0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f   SecPeiDebugAgentLib:DebugTimer.obj

So it appears that even with -b NOOPT builds, the linker is 
filtering out unreferenced objs which explains why the duplicate
symbol error is not triggered.

The XCODE5 tool chain appears to be evaluating symbols
across all objs in a lib and detects a duplicate symbol.

The question is what is the required linker behavior for EDK II
builds?  Require library filtering at obj level, or require no
duplicates across all objs in all libs?

Best regards,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Wednesday, May 24, 2017 4:53 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> edk2-devel@lists.01.org; afish@apple.com; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> On 24 May 2017 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote:
> > CC Ard
> >
> > On 05/24/17 02:27, Kinney, Michael D wrote:
> >> Andrew,
> >>
> >> I agree in this specific case, making the global variable static
> >> should also resolve this issue.
> >>
> >> In general, we do not make module global variables static, so the
> >> module global can be shared across multiple source files in the
> >> module implementation.
> >
> > I think the default should be the reverse: give objects with static
> > storage duration ("global variables") internal linkage ("STATIC") by
> > default, and turn the linkage into external only if multiple source
> > files of the same module actually use the same object together. (In this
> > case the object will have to be declared in a module-internal header
> > file anyway.)
> >
> 
> I strongly agree with Laszlo here. Omitting static defeats any kind of
> optimization the compiler can perform when it knows it can see all
> references to a variable, such as constant folding or emitting the
> variable into .rodata if it does not observe any modifications to it.
> In theory, this could have security implications as well as
> performance implications (e.g., a variable which only gets set in
> DEBUG builds)
> 
> > I grepped the tree for "mMemoryDiscoveredNotifyList", and there are more
> > instances, all exhibiting the same issue:
> >
> > (1) MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > (2) QuarkPlatformPkg/Platform/Pei/PlatformInit/PlatformEarlyInit.c
> > (3) SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c
> > (4) Vlv2TbltDevicePkg/PlatformPei/Platform.c
> >
> > In each of these source files, the "mMemoryDiscoveredNotifyList"
> > variable
> > - has an initializer,
> > - is declared in file scope,
> > - has external linkage,
> > - has static storage duration,
> >
> > thus the declaration qualifies as an "external definition" (of which
> > there may be at most one, for any given object, in the final linking).
> >
> > In each of the four modules listed above, the
> > "mMemoryDiscoveredNotifyList" variable is only used in the same source
> > file that declares / defines the variable. Thus, the variable should be
> > made "STATIC" in every one of them.
> >
> >> Not sure why this issue has not been seen with other tool chains.
> >
> > I think it is either a gcc or a BaseTools (toolchain config) bug.
> >
> > Namely, we faced a similar issue before: please refer to commit
> > 214a3b79417f ("BaseTools GCC: avoid the use of COMMON symbols",
> > 2015-12-08). In that commit, we made sure that gcc wouldn't silently
> > merge multiple external definitions (because that violated ISO C and
> > caused actual runtime bugs). As a result, uninitialized globals were no
> > longer placed in the COMMON section, but in the data section, and
> > multiple external definitions triggered a link editing error.
> >
> > However, in this case we have initialized global variables, which are
> > *already* placed in the data section. I just built OVMF with
> > SOURCE_DEBUG_ENABLE, and verified the following:
> >
> > (a)
> >
> >> $ nm
> Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentL
> ib/OUTPUT/SecPeiDebugAgentLib.lib \
> >>   | grep mMemoryDiscoveredNotifyList
> >> 0000000000000000 D mMemoryDiscoveredNotifyList
> >>
> >> $ nm
> Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.lib \
> >>   | grep mMemoryDiscoveredNotifyList
> >> 0000000000000000 D mMemoryDiscoveredNotifyList
> >
> > The "D" mark means:
> > - "D" / "d": The symbol is in the initialized data section.
> > - uppercase: the symbol is global (external)
> >
> > In other words, linking these two object archives together should fail.
> >
> 
> Yes, but given that they are part of a static library, objects are
> only pulled in on-demand, and so if all references already happen to
> be satisfied, the 'offending' object may never be loaded.
> 
> > (b)
> >
> >> $ egrep 'SecPeiDebugAgentLib\.lib|DxeIpl\.lib' \
> >>
> Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static_librar
> y_files.lst
> >>
> .../Build/OvmfX64/DEBUG_GCC48/X64/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAg
> entLib/OUTPUT/SecPeiDebugAgentLib.lib
> >>
> .../Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/DxeIpl.li
> b
> >
> > This means that the build process will link them together. Indeed I can
> > find the following *successful* command in the build log (see the
> > reference to the above "static_library_files.lst" object list file):
> >
> >> "gcc" \
> >>   -o \
> >>
> Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.dll \
> >>   -nostdlib \
> >>   -Wl,-n,-q,--gc-sections \
> >>   -z common-page-size=0x20 \
> >>   -Wl,--entry,_ModuleEntryPoint \
> >>   -u _ModuleEntryPoint \
> >>   -Wl,-
> Map,Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/DEBUG/DxeIpl.map
> \
> >>   -Wl,-melf_x86_64,--oformat=elf64-x86-64 \
> >>   -Wl,--start-
> group,@Build/OvmfX64/DEBUG_GCC48/X64/MdeModulePkg/Core/DxeIplPeim/DxeIpl/OUTPUT/static
> _library_files.lst,--end-group \
> >>   -g \
> >>   -fshort-wchar \
> >>   -fno-builtin \
> >>   -fno-strict-aliasing \
> >>   -Wall \
> >>   -Werror \
> >>   -Wno-array-bounds \
> >>   -ffunction-sections \
> >>   -fdata-sections \
> >>   -include AutoGen.h \
> >>   -fno-common \
> >>   -DSTRING_ARRAY_NAME=DxeIplStrings \
> >>   -m64 \
> >>   -fno-stack-protector \
> >>   "-DEFIAPI=__attribute__((ms_abi))" \
> >>   -maccumulate-outgoing-args \
> >>   -mno-red-zone \
> >>   -Wno-address \
> >>   -mcmodel=small \
> >>   -fpie \
> >>   -fno-asynchronous-unwind-tables \
> >>   -Wno-address \
> >>   -Os \
> >>   -mno-mmx \
> >>   -mno-sse \
> >>   -D DISABLE_NEW_DEPRECATED_INTERFACES \
> >>   -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \
> >>   -Wl,--script=BaseTools/Scripts/GccBase.lds
> >
> > (c) Re-running the command manually succeeds.
> >
> > (d) Just to see if "-fdata-sections" made any difference ("Place each
> > data item into its own section in the output file"), I removed it. Even
> > that way, the command succeeded.
> >
> > I think this is either a gcc / GNU linker bug, or else our command line
> > (or linker script, "GccBase.lds") is buggy. This link command should not
> > succeed.
> >
> 
> Depending on link order, this may succeed given the reasoning above.
> 
> > Anyway, regarding the patch, I think that all four declarations of
> > "mMemoryDiscoveredNotifyList" should be made STATIC instead.
> >
> 
> Yes, please. Especially when it comes to static libraries (due to the
> flexible way we allow them to be specified in EDK2), I think it is
> really poor hygiene to expose library internals to the library user. I
> know we cannot always avoid it, but we should if we can imo.
> 
> --
> Ard.
> 
> 
> >>> -----Original Message-----
> >>> From: afish@apple.com [mailto:afish@apple.com]
> >>> Sent: Tuesday, May 23, 2017 4:26 PM
> >>> To: Kinney, Michael D <michael.d.kinney@intel.com>
> >>> Cc: edk2-devel@lists.01.org; Fan, Jeff <jeff.fan@intel.com>; Wu, Hao A
> >>> <hao.a.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >>> Subject: Re: [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol
> >>>
> >>> Mike,
> >>>
> >>> Do the other compilers promote (or is that demote) to static? Would not making
> these
> >>> lib globals, and private functions static solve this class of issue?
> >>>
> >>> Thanks,
> >>>
> >>> Andrew Fish
> >>>
> >>>> On May 23, 2017, at 4:21 PM, Michael Kinney <michael.d.kinney@intel.com> wrote:
> >>>>
> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=573
> >>>>
> >>>> 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.
> >>>>
> >>>> 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>
> >>>> ---
> >>>> .../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

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

[Ard Biesheuvel]

On 24 May 2017 at 13:18, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Laszlo,
>
> I agree with the request to add 'static' to the variable declaration
> in the SecPeiDebugAgentLib.  The variable name change will be retained
> because the same symbol name can still be confusing when debugging.
>
> The part that is more concerning is why this duplicate symbol reference
> was not triggered by MSFT or GCC tool chain families, so I did some
> more investigation from the MSFT side this morning.
>
> My first thought was that the optimizer in the compiler/linker was
> optimizing away the duplicate symbol before the final link stage,
> so I tried -b NOOPT builds.  That also did not trigger a duplicate
> symbol error and the MAP file contains only the single reference to
> _mMemoryDiscoveredNotifyList from the DxeIpl.
>
>   0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj
>
> I then evaluated what functions in the DebugAgentLib the DxeIpl
> is using.  It turns out that it is only using a single function
> SaveAndSetDebugTimerInterrupt() that is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
>
> The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
>
> Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> linker only pulls in symbols from the obj in the DebugAgentLib that
> contains the one SaveAndSetDebugTimerInterrupt() function.  The symbols
> from the other objs in the DebugAgentLib are not included in the final
> DxeIpl PE/COFF image.
>
>   0001:000081b0       _InitializeDebugTimer      00008410 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:00008330       _IsDebugTimerTimeout       00008590 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f   SecPeiDebugAgentLib:DebugTimer.obj
>
> So it appears that even with -b NOOPT builds, the linker is
> filtering out unreferenced objs which explains why the duplicate
> symbol error is not triggered.
>

It does not need to filter out what it never considered for inclusion
in the first place. That is why it is called a library: only objects
that satisfy some as yet unresolved symbol reference will get pulled
in.

> The XCODE5 tool chain appears to be evaluating symbols
> across all objs in a lib and detects a duplicate symbol.
>
> The question is what is the required linker behavior for EDK II
> builds?  Require library filtering at obj level, or require no
> duplicates across all objs in all libs?
>

Require no *externally visible* duplicates at all. That is really the
only scaleable solution. Libraries declare their externally visible
symbols in their .h file, and ideally, nothing else should be exposed.
I know this is infeasible in the general case, due to the fact that
libraries typically  consist of several objects, but it should be the
goal not to export anything if we can avoid it. Note that this applies
to functions as well as variables

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

[Kinney, Michael D]

Ard,

I agree that it would be good practice for a library instance to 
only have the public interfaces(functions/data) that are declared
in the library class .h file.

I would be useful to have a report that showed the extra interfaces.

One additional constraint I just found with the MSFT tool chains 
involves the GLOBAL_REMOVE_IF_UNREFERENCED.  This macro can not be
combined with static.  So the recommendation in the thread to add
static to mMemoryDiscoveredNotifyList in SecPeiDebugAgentLib
breaks the build.

d:\work\github\tianocore\edk2\SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c(35): error C2496: 'mDebugAgentMemoryDiscoveredNotifyList': 'selectany' can only be applied to data items with external linkage

The MSFT tool chains depend on the following #define in order for
The optimizer to remove global variables that are not used.  This
is a valuable size optimization for libraries that may be sparsely 
used by modules.

  #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany)

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, May 24, 2017 2:44 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-
> devel@lists.01.org; afish@apple.com; Fan, Jeff <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> On 24 May 2017 at 13:18, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> > Laszlo,
> >
> > I agree with the request to add 'static' to the variable declaration
> > in the SecPeiDebugAgentLib.  The variable name change will be retained
> > because the same symbol name can still be confusing when debugging.
> >
> > The part that is more concerning is why this duplicate symbol reference
> > was not triggered by MSFT or GCC tool chain families, so I did some
> > more investigation from the MSFT side this morning.
> >
> > My first thought was that the optimizer in the compiler/linker was
> > optimizing away the duplicate symbol before the final link stage,
> > so I tried -b NOOPT builds.  That also did not trigger a duplicate
> > symbol error and the MAP file contains only the single reference to
> > _mMemoryDiscoveredNotifyList from the DxeIpl.
> >
> >   0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj
> >
> > I then evaluated what functions in the DebugAgentLib the DxeIpl
> > is using.  It turns out that it is only using a single function
> > SaveAndSetDebugTimerInterrupt() that is implemented in
> >
> >   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
> >
> > The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
> >
> >   SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
> >
> > Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> > linker only pulls in symbols from the obj in the DebugAgentLib that
> > contains the one SaveAndSetDebugTimerInterrupt() function.  The symbols
> > from the other objs in the DebugAgentLib are not included in the final
> > DxeIpl PE/COFF image.
> >
> >   0001:000081b0       _InitializeDebugTimer      00008410 f
> SecPeiDebugAgentLib:DebugTimer.obj
> >   0001:00008330       _IsDebugTimerTimeout       00008590 f
> SecPeiDebugAgentLib:DebugTimer.obj
> >   0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f
> SecPeiDebugAgentLib:DebugTimer.obj
> >
> > So it appears that even with -b NOOPT builds, the linker is
> > filtering out unreferenced objs which explains why the duplicate
> > symbol error is not triggered.
> >
> 
> It does not need to filter out what it never considered for inclusion
> in the first place. That is why it is called a library: only objects
> that satisfy some as yet unresolved symbol reference will get pulled
> in.
> 
> > The XCODE5 tool chain appears to be evaluating symbols
> > across all objs in a lib and detects a duplicate symbol.
> >
> > The question is what is the required linker behavior for EDK II
> > builds?  Require library filtering at obj level, or require no
> > duplicates across all objs in all libs?
> >
> 
> Require no *externally visible* duplicates at all. That is really the
> only scaleable solution. Libraries declare their externally visible
> symbols in their .h file, and ideally, nothing else should be exposed.
> I know this is infeasible in the general case, due to the fact that
> libraries typically  consist of several objects, but it should be the
> goal not to export anything if we can avoid it. Note that this applies
> to functions as well as variables

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

[Kinney, Michael D]

Andrew,

I think I have found an alternate fix for this XCODE5 specific
build failure.  Since there appears to be a difference in the 
linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
the 'ld' command line options used in XCODE5 tool chain in
tools_def.txt.

There is a flag set call '-all_load'.  The description of this
flag is 'Loads all members of static archive libraries.'.

I tried removing this flag from the XCODE5 specific SLINK_FLAGS
and DLINK_FLAGS statements in tools_def.txt, and the duplicate
symbol build failure is no longer present.  I am able to build
and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.

This seems to make XCODE5 linker behavior match the MSFT and GCC
linker behavior.

Do you know why '-all_load' is used in XCODE5 and what impacts
there may be from removing it?

Best regards,

Mike


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, May 24, 2017 5:38 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-
> devel@lists.01.org; afish@apple.com; Fan, Jeff <jeff.fan@intel.com>
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> Ard,
> 
> I agree that it would be good practice for a library instance to
> only have the public interfaces(functions/data) that are declared
> in the library class .h file.
> 
> I would be useful to have a report that showed the extra interfaces.
> 
> One additional constraint I just found with the MSFT tool chains
> involves the GLOBAL_REMOVE_IF_UNREFERENCED.  This macro can not be
> combined with static.  So the recommendation in the thread to add
> static to mMemoryDiscoveredNotifyList in SecPeiDebugAgentLib
> breaks the build.
> 
> d:\work\github\tianocore\edk2\SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\
> SecPeiDebugAgentLib.c(35): error C2496: 'mDebugAgentMemoryDiscoveredNotifyList':
> 'selectany' can only be applied to data items with external linkage
> 
> The MSFT tool chains depend on the following #define in order for
> The optimizer to remove global variables that are not used.  This
> is a valuable size optimization for libraries that may be sparsely
> used by modules.
> 
>   #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany)
> 
> Mike
> 
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Wednesday, May 24, 2017 2:44 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-
> > devel@lists.01.org; afish@apple.com; Fan, Jeff <jeff.fan@intel.com>
> > Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> > symbol
> >
> > On 24 May 2017 at 13:18, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> > > Laszlo,
> > >
> > > I agree with the request to add 'static' to the variable declaration
> > > in the SecPeiDebugAgentLib.  The variable name change will be retained
> > > because the same symbol name can still be confusing when debugging.
> > >
> > > The part that is more concerning is why this duplicate symbol reference
> > > was not triggered by MSFT or GCC tool chain families, so I did some
> > > more investigation from the MSFT side this morning.
> > >
> > > My first thought was that the optimizer in the compiler/linker was
> > > optimizing away the duplicate symbol before the final link stage,
> > > so I tried -b NOOPT builds.  That also did not trigger a duplicate
> > > symbol error and the MAP file contains only the single reference to
> > > _mMemoryDiscoveredNotifyList from the DxeIpl.
> > >
> > >   0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj
> > >
> > > I then evaluated what functions in the DebugAgentLib the DxeIpl
> > > is using.  It turns out that it is only using a single function
> > > SaveAndSetDebugTimerInterrupt() that is implemented in
> > >
> > >   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
> > >
> > > The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
> > >
> > >   SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
> > >
> > > Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> > > linker only pulls in symbols from the obj in the DebugAgentLib that
> > > contains the one SaveAndSetDebugTimerInterrupt() function.  The symbols
> > > from the other objs in the DebugAgentLib are not included in the final
> > > DxeIpl PE/COFF image.
> > >
> > >   0001:000081b0       _InitializeDebugTimer      00008410 f
> > SecPeiDebugAgentLib:DebugTimer.obj
> > >   0001:00008330       _IsDebugTimerTimeout       00008590 f
> > SecPeiDebugAgentLib:DebugTimer.obj
> > >   0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f
> > SecPeiDebugAgentLib:DebugTimer.obj
> > >
> > > So it appears that even with -b NOOPT builds, the linker is
> > > filtering out unreferenced objs which explains why the duplicate
> > > symbol error is not triggered.
> > >
> >
> > It does not need to filter out what it never considered for inclusion
> > in the first place. That is why it is called a library: only objects
> > that satisfy some as yet unresolved symbol reference will get pulled
> > in.
> >
> > > The XCODE5 tool chain appears to be evaluating symbols
> > > across all objs in a lib and detects a duplicate symbol.
> > >
> > > The question is what is the required linker behavior for EDK II
> > > builds?  Require library filtering at obj level, or require no
> > > duplicates across all objs in all libs?
> > >
> >
> > Require no *externally visible* duplicates at all. That is really the
> > only scaleable solution. Libraries declare their externally visible
> > symbols in their .h file, and ideally, nothing else should be exposed.
> > I know this is infeasible in the general case, due to the fact that
> > libraries typically  consist of several objects, but it should be the
> > goal not to export anything if we can avoid it. Note that this applies
> > to functions as well as variables

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

[Laszlo Ersek]

On 05/25/17 03:47, Kinney, Michael D wrote:
> Andrew,
> 
> I think I have found an alternate fix for this XCODE5 specific
> build failure.  Since there appears to be a difference in the 
> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
> the 'ld' command line options used in XCODE5 tool chain in
> tools_def.txt.
> 
> There is a flag set call '-all_load'.  The description of this
> flag is 'Loads all members of static archive libraries.'.
> 
> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
> symbol build failure is no longer present.  I am able to build
> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
> 
> This seems to make XCODE5 linker behavior match the MSFT and GCC
> linker behavior.
> 
> Do you know why '-all_load' is used in XCODE5 and what impacts
> there may be from removing it?

Please don't remove -all_load from there; instead we should figure out
if the same can be brought to MSFT and GCC.

The error message that XCODE5 emitted caught a real bug (undefined
behavior according to ISO C, see my previous email), and so we should
keep that detection enabled (we should even extend it to other
toolchains, if that's possible).

As for docs, I found this:

http://www.manpages.info/macosx/ld.1.html

> -all_load
>     Loads all members of static archive libraries. This option does
>     not apply to dynamic shared libraries.


Thanks
Laszlo

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

[Andrew Fish]

> On May 25, 2017, at 9:08 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 05/25/17 03:47, Kinney, Michael D wrote:
>> Andrew,
>> 
>> I think I have found an alternate fix for this XCODE5 specific
>> build failure.  Since there appears to be a difference in the 
>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed 
>> the 'ld' command line options used in XCODE5 tool chain in
>> tools_def.txt.
>> 
>> There is a flag set call '-all_load'.  The description of this
>> flag is 'Loads all members of static archive libraries.'.
>> 
>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
>> symbol build failure is no longer present.  I am able to build
>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
>> 
>> This seems to make XCODE5 linker behavior match the MSFT and GCC
>> linker behavior.
>> 
>> Do you know why '-all_load' is used in XCODE5 and what impacts
>> there may be from removing it?
> 
> Please don't remove -all_load from there; instead we should figure out
> if the same can be brought to MSFT and GCC.
> 

Not to mention I was told by the Xcode linker developer that -all_load was required to ensure in all cases the Mach-O produced would be compatible with conversion to PE/COFF. 

Thanks,

Andrew Fish

> The error message that XCODE5 emitted caught a real bug (undefined
> behavior according to ISO C, see my previous email), and so we should
> keep that detection enabled (we should even extend it to other
> toolchains, if that's possible).
> 
> As for docs, I found this:
> 
> http://www.manpages.info/macosx/ld.1.html <http://www.manpages.info/macosx/ld.1.html>
> 
>> -all_load
>>    Loads all members of static archive libraries. This option does
>>    not apply to dynamic shared libraries.
> 
> 
> Thanks
> Laszlo

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

[Kinney, Michael D]

Laszlo,

I think the equivalent flag for GCC builds is --whole-archive.

I tried adding that flag to DLINK_FLAGS in GCC5, and I get the 
following error building OVMF from edk2/master with 
-D SOURCE_DEBUG_ENABLE set.

DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
(.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
collect2: error: ld returned 1 exit status

Visual Studio 2015 Update 2 has also added a new linker flag called
/WHOLEARCHIVE.  I am working on evaluating that flag to see if it 
catches the same issue.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, May 25, 2017 9:09 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> On 05/25/17 03:47, Kinney, Michael D wrote:
> > Andrew,
> >
> > I think I have found an alternate fix for this XCODE5 specific
> > build failure.  Since there appears to be a difference in the
> > linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
> > the 'ld' command line options used in XCODE5 tool chain in
> > tools_def.txt.
> >
> > There is a flag set call '-all_load'.  The description of this
> > flag is 'Loads all members of static archive libraries.'.
> >
> > I tried removing this flag from the XCODE5 specific SLINK_FLAGS
> > and DLINK_FLAGS statements in tools_def.txt, and the duplicate
> > symbol build failure is no longer present.  I am able to build
> > and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
> >
> > This seems to make XCODE5 linker behavior match the MSFT and GCC
> > linker behavior.
> >
> > Do you know why '-all_load' is used in XCODE5 and what impacts
> > there may be from removing it?
> 
> Please don't remove -all_load from there; instead we should figure out
> if the same can be brought to MSFT and GCC.
> 
> The error message that XCODE5 emitted caught a real bug (undefined
> behavior according to ISO C, see my previous email), and so we should
> keep that detection enabled (we should even extend it to other
> toolchains, if that's possible).
> 
> As for docs, I found this:
> 
> http://www.manpages.info/macosx/ld.1.html
> 
> > -all_load
> >     Loads all members of static archive libraries. This option does
> >     not apply to dynamic shared libraries.
> 
> 
> Thanks
> Laszlo

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

[Laszlo Ersek]

On 05/25/17 19:38, Kinney, Michael D wrote:
> Laszlo,
> 
> I think the equivalent flag for GCC builds is --whole-archive.
> 
> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the 
> following error building OVMF from edk2/master with 
> -D SOURCE_DEBUG_ENABLE set.
> 
> DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
> collect2: error: ld returned 1 exit status

Great find! That's the error message we should get.

Unfortunately, after reading the "ld" manual on "--whole-archive", it
seems that the complete object files will actually be copied into the
resultant binary, even if several of their symbols will remain unused. I
think that's quite sub-optimal. (I haven't verified this though.) What
we'd like to get is (a) the full verification at link time, and (b)
inclusion of *only* those symbols that are actually necessary.

In your testing, when you build OVMF with and without "--whole-archive",
do you see a difference in, say, the DXEFV footprint, when the build
completes?

(If so, then I wonder if we should add "--whole-archive" only to the
NOOPT build... Not sure.)

> Visual Studio 2015 Update 2 has also added a new linker flag called
> /WHOLEARCHIVE.  I am working on evaluating that flag to see if it 
> catches the same issue.

Thanks!
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, May 25, 2017 9:09 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>> <jeff.fan@intel.com>
>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
>> symbol
>>
>> On 05/25/17 03:47, Kinney, Michael D wrote:
>>> Andrew,
>>>
>>> I think I have found an alternate fix for this XCODE5 specific
>>> build failure.  Since there appears to be a difference in the
>>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
>>> the 'ld' command line options used in XCODE5 tool chain in
>>> tools_def.txt.
>>>
>>> There is a flag set call '-all_load'.  The description of this
>>> flag is 'Loads all members of static archive libraries.'.
>>>
>>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
>>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
>>> symbol build failure is no longer present.  I am able to build
>>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
>>>
>>> This seems to make XCODE5 linker behavior match the MSFT and GCC
>>> linker behavior.
>>>
>>> Do you know why '-all_load' is used in XCODE5 and what impacts
>>> there may be from removing it?
>>
>> Please don't remove -all_load from there; instead we should figure out
>> if the same can be brought to MSFT and GCC.
>>
>> The error message that XCODE5 emitted caught a real bug (undefined
>> behavior according to ISO C, see my previous email), and so we should
>> keep that detection enabled (we should even extend it to other
>> toolchains, if that's possible).
>>
>> As for docs, I found this:
>>
>> http://www.manpages.info/macosx/ld.1.html
>>
>>> -all_load
>>>     Loads all members of static archive libraries. This option does
>>>     not apply to dynamic shared libraries.
>>
>>
>> Thanks
>> Laszlo

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

[Ard Biesheuvel]

On 25 May 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/25/17 19:38, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I think the equivalent flag for GCC builds is --whole-archive.
>>
>> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
>> following error building OVMF from edk2/master with
>> -D SOURCE_DEBUG_ENABLE set.
>>
>> DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
>> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
>> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
>> collect2: error: ld returned 1 exit status
>
> Great find! That's the error message we should get.
>
> Unfortunately, after reading the "ld" manual on "--whole-archive", it
> seems that the complete object files will actually be copied into the
> resultant binary, even if several of their symbols will remain unused. I
> think that's quite sub-optimal. (I haven't verified this though.) What
> we'd like to get is (a) the full verification at link time, and (b)
> inclusion of *only* those symbols that are actually necessary.
>
> In your testing, when you build OVMF with and without "--whole-archive",
> do you see a difference in, say, the DXEFV footprint, when the build
> completes?
>
> (If so, then I wonder if we should add "--whole-archive" only to the
> NOOPT build... Not sure.)
>

I haven't tried, but I would expect --gc-sections to deal with the
unused objects.

>> Visual Studio 2015 Update 2 has also added a new linker flag called
>> /WHOLEARCHIVE.  I am working on evaluating that flag to see if it
>> catches the same issue.
>
> Thanks!
> Laszlo
>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Thursday, May 25, 2017 9:09 AM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>>> <jeff.fan@intel.com>
>>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
>>> symbol
>>>
>>> On 05/25/17 03:47, Kinney, Michael D wrote:
>>>> Andrew,
>>>>
>>>> I think I have found an alternate fix for this XCODE5 specific
>>>> build failure.  Since there appears to be a difference in the
>>>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
>>>> the 'ld' command line options used in XCODE5 tool chain in
>>>> tools_def.txt.
>>>>
>>>> There is a flag set call '-all_load'.  The description of this
>>>> flag is 'Loads all members of static archive libraries.'.
>>>>
>>>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
>>>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
>>>> symbol build failure is no longer present.  I am able to build
>>>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
>>>>
>>>> This seems to make XCODE5 linker behavior match the MSFT and GCC
>>>> linker behavior.
>>>>
>>>> Do you know why '-all_load' is used in XCODE5 and what impacts
>>>> there may be from removing it?
>>>
>>> Please don't remove -all_load from there; instead we should figure out
>>> if the same can be brought to MSFT and GCC.
>>>
>>> The error message that XCODE5 emitted caught a real bug (undefined
>>> behavior according to ISO C, see my previous email), and so we should
>>> keep that detection enabled (we should even extend it to other
>>> toolchains, if that's possible).
>>>
>>> As for docs, I found this:
>>>
>>> http://www.manpages.info/macosx/ld.1.html
>>>
>>>> -all_load
>>>>     Loads all members of static archive libraries. This option does
>>>>     not apply to dynamic shared libraries.
>>>
>>>
>>> Thanks
>>> Laszlo
>

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

[Laszlo Ersek]

On 05/25/17 21:55, Ard Biesheuvel wrote:
> On 25 May 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/25/17 19:38, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> I think the equivalent flag for GCC builds is --whole-archive.
>>>
>>> I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
>>> following error building OVMF from edk2/master with
>>> -D SOURCE_DEBUG_ENABLE set.
>>>
>>> DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
>>> (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
>>> SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
>>> collect2: error: ld returned 1 exit status
>>
>> Great find! That's the error message we should get.
>>
>> Unfortunately, after reading the "ld" manual on "--whole-archive", it
>> seems that the complete object files will actually be copied into the
>> resultant binary, even if several of their symbols will remain unused. I
>> think that's quite sub-optimal. (I haven't verified this though.) What
>> we'd like to get is (a) the full verification at link time, and (b)
>> inclusion of *only* those symbols that are actually necessary.
>>
>> In your testing, when you build OVMF with and without "--whole-archive",
>> do you see a difference in, say, the DXEFV footprint, when the build
>> completes?
>>
>> (If so, then I wonder if we should add "--whole-archive" only to the
>> NOOPT build... Not sure.)
>>
> 
> I haven't tried, but I would expect --gc-sections to deal with the
> unused objects.

Good point! (... After I read this option's description too, in the "ld"
manual.) This ELF and PE/COFF stuff is way too complex for my taste. :/

Laszlo

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

[Kinney, Michael D]

Laszlo,

I have the same concern on final image sizes.  I have done some
evaluation:

GCC5 OVMF X64 DEBUG without -whole-archive 
==========================================
FV Space Information
SECFV [19%Full] 212992 total, 42000 used, 170992 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
PEIFV [19%Full] 917504 total, 180648 used, 736856 free
Total used = 5409432

GCC5 OVMF X64 DEBUG with -whole-archive 
=======================================
FV Space Information
SECFV [19%Full] 212992 total, 41936 used, 171056 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
PEIFV [19%Full] 917504 total, 181352 used, 736152 free
Total used = 5411248

Total used difference = 1816 bytes larger with -whole-archive

I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
and it also catches the same duplicate symbol error now.

error C2220: warning treated as error - no 'executable' file generated
warning C4744: 'mMemoryDiscoveredNotifyList' has different type in 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebugagent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList already defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Core\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more multiply defined symbols found

VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE 
===========================================
FV Space Information
SECFV [22%Full] 212992 total, 48560 used, 164432 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
PEIFV [22%Full] 917504 total, 204840 used, 712664 free
Total used = 5564752


VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE 
===========================================
FV Space Information
SECFV [23%Full] 212992 total, 50384 used, 162608 free
FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
PEIFV [27%Full] 917504 total, 255528 used, 661976 free
Total used = 5875338

Total used difference = 310586 bytes larger with /WHOLEARCHIVE

For tool chains that do have size impacts, one option is to
have a "test" build that enables the linker flags to detect
duplicate symbols.  For example the following could be added
to a DSC file.  May want to disable GenFds stage when doing
this type of build.

[BuildOptions]
!ifdef $(DETECT_DUPLICATE_SYMBOLS)
  MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
!endif

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, May 25, 2017 11:07 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> On 05/25/17 19:38, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I think the equivalent flag for GCC builds is --whole-archive.
> >
> > I tried adding that flag to DLINK_FLAGS in GCC5, and I get the
> > following error building OVMF from edk2/master with
> > -D SOURCE_DEBUG_ENABLE set.
> >
> > DxeLoad.obj (symbol from plugin): In function `InstallIplPermanentMemoryPpis':
> > (.text+0x0): multiple definition of `mMemoryDiscoveredNotifyList'
> > SecPeiDebugAgentLib.obj (symbol from plugin):(.text+0x0): first defined here
> > collect2: error: ld returned 1 exit status
> 
> Great find! That's the error message we should get.
> 
> Unfortunately, after reading the "ld" manual on "--whole-archive", it
> seems that the complete object files will actually be copied into the
> resultant binary, even if several of their symbols will remain unused. I
> think that's quite sub-optimal. (I haven't verified this though.) What
> we'd like to get is (a) the full verification at link time, and (b)
> inclusion of *only* those symbols that are actually necessary.
> 
> In your testing, when you build OVMF with and without "--whole-archive",
> do you see a difference in, say, the DXEFV footprint, when the build
> completes?
> 
> (If so, then I wonder if we should add "--whole-archive" only to the
> NOOPT build... Not sure.)
> 
> > Visual Studio 2015 Update 2 has also added a new linker flag called
> > /WHOLEARCHIVE.  I am working on evaluating that flag to see if it
> > catches the same issue.
> 
> Thanks!
> Laszlo
> 
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Thursday, May 25, 2017 9:09 AM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> >> <jeff.fan@intel.com>
> >> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> >> symbol
> >>
> >> On 05/25/17 03:47, Kinney, Michael D wrote:
> >>> Andrew,
> >>>
> >>> I think I have found an alternate fix for this XCODE5 specific
> >>> build failure.  Since there appears to be a difference in the
> >>> linker behavior between MSFT/GCC/XCODE tool chains, I reviewed
> >>> the 'ld' command line options used in XCODE5 tool chain in
> >>> tools_def.txt.
> >>>
> >>> There is a flag set call '-all_load'.  The description of this
> >>> flag is 'Loads all members of static archive libraries.'.
> >>>
> >>> I tried removing this flag from the XCODE5 specific SLINK_FLAGS
> >>> and DLINK_FLAGS statements in tools_def.txt, and the duplicate
> >>> symbol build failure is no longer present.  I am able to build
> >>> and boot OVMF with XCODE5 with -D SOURCE_DEBUG_ENABLE flag set.
> >>>
> >>> This seems to make XCODE5 linker behavior match the MSFT and GCC
> >>> linker behavior.
> >>>
> >>> Do you know why '-all_load' is used in XCODE5 and what impacts
> >>> there may be from removing it?
> >>
> >> Please don't remove -all_load from there; instead we should figure out
> >> if the same can be brought to MSFT and GCC.
> >>
> >> The error message that XCODE5 emitted caught a real bug (undefined
> >> behavior according to ISO C, see my previous email), and so we should
> >> keep that detection enabled (we should even extend it to other
> >> toolchains, if that's possible).
> >>
> >> As for docs, I found this:
> >>
> >> http://www.manpages.info/macosx/ld.1.html
> >>
> >>> -all_load
> >>>     Loads all members of static archive libraries. This option does
> >>>     not apply to dynamic shared libraries.
> >>
> >>
> >> Thanks
> >> Laszlo

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

[Laszlo Ersek]

On 05/25/17 21:57, Kinney, Michael D wrote:
> Laszlo,
> 
> I have the same concern on final image sizes.  I have done some
> evaluation:
> 
> GCC5 OVMF X64 DEBUG without -whole-archive 
> ==========================================
> FV Space Information
> SECFV [19%Full] 212992 total, 42000 used, 170992 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
> DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
> PEIFV [19%Full] 917504 total, 180648 used, 736856 free
> Total used = 5409432
> 
> GCC5 OVMF X64 DEBUG with -whole-archive 
> =======================================
> FV Space Information
> SECFV [19%Full] 212992 total, 41936 used, 171056 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
> DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
> PEIFV [19%Full] 917504 total, 181352 used, 736152 free
> Total used = 5411248
> 
> Total used difference = 1816 bytes larger with -whole-archive
> 
> I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
> and it also catches the same duplicate symbol error now.
> 
> error C2220: warning treated as error - no 'executable' file generated
> warning C4744: 'mMemoryDiscoveredNotifyList' has different type in 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebugagent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
> DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList already defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
> d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Core\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more multiply defined symbols found
> 
> VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE 
> ===========================================
> FV Space Information
> SECFV [22%Full] 212992 total, 48560 used, 164432 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
> DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
> PEIFV [22%Full] 917504 total, 204840 used, 712664 free
> Total used = 5564752
> 
> 
> VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE 
> ===========================================
> FV Space Information
> SECFV [23%Full] 212992 total, 50384 used, 162608 free
> FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
> DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
> PEIFV [27%Full] 917504 total, 255528 used, 661976 free
> Total used = 5875338
> 
> Total used difference = 310586 bytes larger with /WHOLEARCHIVE
> 
> For tool chains that do have size impacts, one option is to
> have a "test" build that enables the linker flags to detect
> duplicate symbols.  For example the following could be added
> to a DSC file.  May want to disable GenFds stage when doing
> this type of build.
> 
> [BuildOptions]
> !ifdef $(DETECT_DUPLICATE_SYMBOLS)
>   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
> !endif

Thank you (again) for the research! Looks like the gcc size impact is
friendly enough to keep --whole-archive enabled at all times (possibly
due to the --gc-sections flag mentioned by Ard, which we already have
enabled).

The VS2015 impact is really large however.

I was hoping we could add these flags to
"BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
flag is non-default, and/or it's platform-dependent, then it will almost
never be used, most likely.) But the VS2015 size increase really
precludes /WHOLEARCHIVE (for the MSFT family) from "tools_def.template".

Would it be acceptable to add --whole-archive to "tools_def.template"
only for GCC? After all, at the moment only XCODE*/XCLANG have "-all_load".

Thanks,
Laszlo

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

[Kinney, Michael D]

Laszlo,

The other idea I have is for MSFT tool chains to do the DLINK step twice.  Once
with /WHOLEARCHIVE set to a .dll that is not used in later steps, but the doing 
the DLINK action detects duplicate symbols.

If the first DLINK step passes, then so a second DLINK step to a .dll without 
/WHOLEARCHIVE set and use this .dll to produce the .efi file that goes into the 
FW image.

This 2 step link process would have the side effect of potentially increasing 
build times, but could be done for specific tool chain families in build_rules.txt.

The first DLINK step could also disable a lot of the optimizations that take 
longer since the goal of this step is only to detect a duplicate symbol.

Best regards,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, May 25, 2017 1:11 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com) <afish@apple.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> On 05/25/17 21:57, Kinney, Michael D wrote:
> > Laszlo,
> >
> > I have the same concern on final image sizes.  I have done some
> > evaluation:
> >
> > GCC5 OVMF X64 DEBUG without -whole-archive
> > ==========================================
> > FV Space Information
> > SECFV [19%Full] 212992 total, 42000 used, 170992 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
> > DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
> > PEIFV [19%Full] 917504 total, 180648 used, 736856 free
> > Total used = 5409432
> >
> > GCC5 OVMF X64 DEBUG with -whole-archive
> > =======================================
> > FV Space Information
> > SECFV [19%Full] 212992 total, 41936 used, 171056 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
> > DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
> > PEIFV [19%Full] 917504 total, 181352 used, 736152 free
> > Total used = 5411248
> >
> > Total used difference = 1816 bytes larger with -whole-archive
> >
> > I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
> > and it also catches the same duplicate symbol error now.
> >
> > error C2220: warning treated as error - no 'executable' file generated
> > warning C4744: 'mMemoryDiscoveredNotifyList' has different type in
> 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c' and
> 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\secpeidebug
> agent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
> > DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList already
> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
> >
> d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\MdeModulePkg\Co
> re\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more multiply
> defined symbols found
> >
> > VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE
> > ===========================================
> > FV Space Information
> > SECFV [22%Full] 212992 total, 48560 used, 164432 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
> > DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
> > PEIFV [22%Full] 917504 total, 204840 used, 712664 free
> > Total used = 5564752
> >
> >
> > VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE
> > ===========================================
> > FV Space Information
> > SECFV [23%Full] 212992 total, 50384 used, 162608 free
> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
> > DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
> > PEIFV [27%Full] 917504 total, 255528 used, 661976 free
> > Total used = 5875338
> >
> > Total used difference = 310586 bytes larger with /WHOLEARCHIVE
> >
> > For tool chains that do have size impacts, one option is to
> > have a "test" build that enables the linker flags to detect
> > duplicate symbols.  For example the following could be added
> > to a DSC file.  May want to disable GenFds stage when doing
> > this type of build.
> >
> > [BuildOptions]
> > !ifdef $(DETECT_DUPLICATE_SYMBOLS)
> >   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
> > !endif
> 
> Thank you (again) for the research! Looks like the gcc size impact is
> friendly enough to keep --whole-archive enabled at all times (possibly
> due to the --gc-sections flag mentioned by Ard, which we already have
> enabled).
> 
> The VS2015 impact is really large however.
> 
> I was hoping we could add these flags to
> "BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
> flag is non-default, and/or it's platform-dependent, then it will almost
> never be used, most likely.) But the VS2015 size increase really
> precludes /WHOLEARCHIVE (for the MSFT family) from "tools_def.template".
> 
> Would it be acceptable to add --whole-archive to "tools_def.template"
> only for GCC? After all, at the moment only XCODE*/XCLANG have "-all_load".
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Gao, Liming]

Mike:
  I think build performance is also a key point. I prefer to add this option in NOOPT target. After add this option, we can build edk2 packages to detect those duplicated issues.

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Kinney, Michael D
>Sent: Friday, May 26, 2017 6:48 AM
>To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com)
><afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
><jeff.fan@intel.com>
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>Laszlo,
>
>The other idea I have is for MSFT tool chains to do the DLINK step twice.  Once
>with /WHOLEARCHIVE set to a .dll that is not used in later steps, but the doing
>the DLINK action detects duplicate symbols.
>
>If the first DLINK step passes, then so a second DLINK step to a .dll without
>/WHOLEARCHIVE set and use this .dll to produce the .efi file that goes into the
>FW image.
>
>This 2 step link process would have the side effect of potentially increasing
>build times, but could be done for specific tool chain families in build_rules.txt.
>
>The first DLINK step could also disable a lot of the optimizations that take
>longer since the goal of this step is only to detect a duplicate symbol.
>
>Best regards,
>
>Mike
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo
>> Ersek
>> Sent: Thursday, May 25, 2017 1:11 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com)
><afish@apple.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>> <jeff.fan@intel.com>
>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> duplicate symbol
>>
>> On 05/25/17 21:57, Kinney, Michael D wrote:
>> > Laszlo,
>> >
>> > I have the same concern on final image sizes.  I have done some
>> > evaluation:
>> >
>> > GCC5 OVMF X64 DEBUG without -whole-archive
>> > ==========================================
>> > FV Space Information
>> > SECFV [19%Full] 212992 total, 42000 used, 170992 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
>> > DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
>> > PEIFV [19%Full] 917504 total, 180648 used, 736856 free
>> > Total used = 5409432
>> >
>> > GCC5 OVMF X64 DEBUG with -whole-archive
>> > =======================================
>> > FV Space Information
>> > SECFV [19%Full] 212992 total, 41936 used, 171056 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
>> > DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
>> > PEIFV [19%Full] 917504 total, 181352 used, 736152 free
>> > Total used = 5411248
>> >
>> > Total used difference = 1816 bytes larger with -whole-archive
>> >
>> > I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
>> > and it also catches the same duplicate symbol error now.
>> >
>> > error C2220: warning treated as error - no 'executable' file generated
>> > warning C4744: 'mMemoryDiscoveredNotifyList' has different type in
>>
>'d:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c'
>and
>>
>'d:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\s
>ecpeidebug
>> agent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
>> > DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList
>already
>> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
>> >
>>
>d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\M
>deModulePkg\Co
>> re\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more
>multiply
>> defined symbols found
>> >
>> > VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE
>> > ===========================================
>> > FV Space Information
>> > SECFV [22%Full] 212992 total, 48560 used, 164432 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
>> > DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
>> > PEIFV [22%Full] 917504 total, 204840 used, 712664 free
>> > Total used = 5564752
>> >
>> >
>> > VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE
>> > ===========================================
>> > FV Space Information
>> > SECFV [23%Full] 212992 total, 50384 used, 162608 free
>> > FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
>> > DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
>> > PEIFV [27%Full] 917504 total, 255528 used, 661976 free
>> > Total used = 5875338
>> >
>> > Total used difference = 310586 bytes larger with /WHOLEARCHIVE
>> >
>> > For tool chains that do have size impacts, one option is to
>> > have a "test" build that enables the linker flags to detect
>> > duplicate symbols.  For example the following could be added
>> > to a DSC file.  May want to disable GenFds stage when doing
>> > this type of build.
>> >
>> > [BuildOptions]
>> > !ifdef $(DETECT_DUPLICATE_SYMBOLS)
>> >   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
>> > !endif
>>
>> Thank you (again) for the research! Looks like the gcc size impact is
>> friendly enough to keep --whole-archive enabled at all times (possibly
>> due to the --gc-sections flag mentioned by Ard, which we already have
>> enabled).
>>
>> The VS2015 impact is really large however.
>>
>> I was hoping we could add these flags to
>> "BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
>> flag is non-default, and/or it's platform-dependent, then it will almost
>> never be used, most likely.) But the VS2015 size increase really
>> precludes /WHOLEARCHIVE (for the MSFT family) from
>"tools_def.template".
>>
>> Would it be acceptable to add --whole-archive to "tools_def.template"
>> only for GCC? After all, at the moment only XCODE*/XCLANG have "-
>all_load".
>>
>> Thanks,
>> Laszlo
>> _______________________________________________
>> 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

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

[Laszlo Ersek]

On 05/26/17 07:29, Gao, Liming wrote:
> Mike:
> I think build performance is also a key point. I prefer to add this
> option in NOOPT target. After add this option, we can build edk2
> packages to detect those duplicated issues.
OK, how about this:

- MSFT toolchains: do the two step linking that Mike described, but only
  for NOOPT (assuming that is possible). The user would have to pay for
  the collision detection with CPU time, but not with an exorbitant size
  increase. I think that's a better trade-off than finishing quickly but
  potentially not fitting into the firmware image. (You generally pick
  NOOPT for debugging, so you certainly wish to *run* the image.) DEBUG
  and RELEASE targets wouldn't be changed.

- GCC toolchains: I think I'd like --whole-archive to become the default
  (regardless of build target), since there don't seem to be any
  relevant size costs to it.

Thanks,
Laszlo

> 
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Kinney, Michael D
>> Sent: Friday, May 26, 2017 6:48 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com)
>> <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>> <jeff.fan@intel.com>
>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>> duplicate symbol
>>
>> Laszlo,
>>
>> The other idea I have is for MSFT tool chains to do the DLINK step twice.  Once
>> with /WHOLEARCHIVE set to a .dll that is not used in later steps, but the doing
>> the DLINK action detects duplicate symbols.
>>
>> If the first DLINK step passes, then so a second DLINK step to a .dll without
>> /WHOLEARCHIVE set and use this .dll to produce the .efi file that goes into the
>> FW image.
>>
>> This 2 step link process would have the side effect of potentially increasing
>> build times, but could be done for specific tool chain families in build_rules.txt.
>>
>> The first DLINK step could also disable a lot of the optimizations that take
>> longer since the goal of this step is only to detect a duplicate symbol.
>>
>> Best regards,
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo
>>> Ersek
>>> Sent: Thursday, May 25, 2017 1:11 PM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Andrew Fish (afish@apple.com)
>> <afish@apple.com>
>>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>>> <jeff.fan@intel.com>
>>> Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>> Fix
>>> duplicate symbol
>>>
>>> On 05/25/17 21:57, Kinney, Michael D wrote:
>>>> Laszlo,
>>>>
>>>> I have the same concern on final image sizes.  I have done some
>>>> evaluation:
>>>>
>>>> GCC5 OVMF X64 DEBUG without -whole-archive
>>>> ==========================================
>>>> FV Space Information
>>>> SECFV [19%Full] 212992 total, 42000 used, 170992 free
>>>> FVMAIN_COMPACT [33%Full] 3440640 total, 1162760 used, 2277880 free
>>>> DXEFV [38%Full] 10485760 total, 4024024 used, 6461736 free
>>>> PEIFV [19%Full] 917504 total, 180648 used, 736856 free
>>>> Total used = 5409432
>>>>
>>>> GCC5 OVMF X64 DEBUG with -whole-archive
>>>> =======================================
>>>> FV Space Information
>>>> SECFV [19%Full] 212992 total, 41936 used, 171056 free
>>>> FVMAIN_COMPACT [33%Full] 3440640 total, 1158304 used, 2282336 free
>>>> DXEFV [38%Full] 10485760 total, 4029656 used, 6456104 free
>>>> PEIFV [19%Full] 917504 total, 181352 used, 736152 free
>>>> Total used = 5411248
>>>>
>>>> Total used difference = 1816 bytes larger with -whole-archive
>>>>
>>>> I was also able to do a MSFT VS2015 build with /WHOLEARCHIVE set
>>>> and it also catches the same duplicate symbol error now.
>>>>
>>>> error C2220: warning treated as error - no 'executable' file generated
>>>> warning C4744: 'mMemoryDiscoveredNotifyList' has different type in
>>>
>> 'd:\work\github\tianocore\edk2\mdemodulepkg\core\dxeiplpeim\dxeload.c'
>> and
>>>
>> 'd:\work\github\tianocore\edk2\sourceleveldebugpkg\library\debugagent\s
>> ecpeidebug
>>> agent\secpeidebugagentlib.c': 'struct (12 bytes)' and 'array (12 bytes)'
>>>> DxeIpl.lib(DxeLoad.obj) : error LNK2005: _mMemoryDiscoveredNotifyList
>> already
>>> defined in SecPeiDebugAgentLib.lib(SecPeiDebugAgentLib.obj)
>>>>
>>>
>> d:\work\github\tianocore\edk2\Build\OvmfIa32\DEBUG_VS2015x86\IA32\M
>> deModulePkg\Co
>>> re\DxeIplPeim\DxeIpl\DEBUG\DxeIpl.dll : fatal error LNK1169: one or more
>> multiply
>>> defined symbols found
>>>>
>>>> VS2015 OVMF X64 DEBUG without /WHOLEARCHIVE
>>>> ===========================================
>>>> FV Space Information
>>>> SECFV [22%Full] 212992 total, 48560 used, 164432 free
>>>> FVMAIN_COMPACT [33%Full] 3440640 total, 1147464 used, 2293176 free
>>>> DXEFV [39%Full] 10485760 total, 4163888 used, 6321872 free
>>>> PEIFV [22%Full] 917504 total, 204840 used, 712664 free
>>>> Total used = 5564752
>>>>
>>>>
>>>> VS2015 OVMF X64 DEBUG with /WHOLEARCHIVE
>>>> ===========================================
>>>> FV Space Information
>>>> SECFV [23%Full] 212992 total, 50384 used, 162608 free
>>>> FVMAIN_COMPACT [33%Full] 3440640 total, 1147424 used, 2293216 free
>>>> DXEFV [42%Full] 10485760 total, 4422992 used, 6062768 free
>>>> PEIFV [27%Full] 917504 total, 255528 used, 661976 free
>>>> Total used = 5875338
>>>>
>>>> Total used difference = 310586 bytes larger with /WHOLEARCHIVE
>>>>
>>>> For tool chains that do have size impacts, one option is to
>>>> have a "test" build that enables the linker flags to detect
>>>> duplicate symbols.  For example the following could be added
>>>> to a DSC file.  May want to disable GenFds stage when doing
>>>> this type of build.
>>>>
>>>> [BuildOptions]
>>>> !ifdef $(DETECT_DUPLICATE_SYMBOLS)
>>>>   MSFT:*_VS2015_*_DLINK_FLAGS = /WHOLEARCHIVE
>>>> !endif
>>>
>>> Thank you (again) for the research! Looks like the gcc size impact is
>>> friendly enough to keep --whole-archive enabled at all times (possibly
>>> due to the --gc-sections flag mentioned by Ard, which we already have
>>> enabled).
>>>
>>> The VS2015 impact is really large however.
>>>
>>> I was hoping we could add these flags to
>>> "BaseTools/Conf/tools_def.template", regardless of platform DSC. (If the
>>> flag is non-default, and/or it's platform-dependent, then it will almost
>>> never be used, most likely.) But the VS2015 size increase really
>>> precludes /WHOLEARCHIVE (for the MSFT family) from
>> "tools_def.template".
>>>
>>> Would it be acceptable to add --whole-archive to "tools_def.template"
>>> only for GCC? After all, at the moment only XCODE*/XCLANG have "-
>> all_load".
>>>
>>> Thanks,
>>> Laszlo
>>> _______________________________________________
>>> 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

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

[Laszlo Ersek]

On 05/24/17 22:18, Kinney, Michael D wrote:
> Laszlo,
>
> I agree with the request to add 'static' to the variable declaration
> in the SecPeiDebugAgentLib.  The variable name change will be retained
> because the same symbol name can still be confusing when debugging.
>
> The part that is more concerning is why this duplicate symbol
> reference was not triggered by MSFT or GCC tool chain families, so I
> did some more investigation from the MSFT side this morning.
>
> My first thought was that the optimizer in the compiler/linker was
> optimizing away the duplicate symbol before the final link stage,
> so I tried -b NOOPT builds.  That also did not trigger a duplicate
> symbol error and the MAP file contains only the single reference to
> _mMemoryDiscoveredNotifyList from the DxeIpl.
>
>   0002:000001b8       _mMemoryDiscoveredNotifyList 000164f8     DxeIpl:DxeLoad.obj
>
> I then evaluated what functions in the DebugAgentLib the DxeIpl
> is using.  It turns out that it is only using a single function
> SaveAndSetDebugTimerInterrupt() that is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\DebugAgentCommon\DebugAgent.c
>
> The mMemoryDiscoveredNotifyList duplicate symbol is implemented in
>
>   SourceLevelDebugPkg\Library\DebugAgent\SecPeiDebugAgent\SecPeiDebugAgentLib.c
>
> Even with a MSFT -b NOOPT build, when I look at the MAP file, the
> linker only pulls in symbols from the obj in the DebugAgentLib that
> contains the one SaveAndSetDebugTimerInterrupt() function.  The
> symbols from the other objs in the DebugAgentLib are not included in
> the final DxeIpl PE/COFF image.
>
>   0001:000081b0       _InitializeDebugTimer      00008410 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:00008330       _IsDebugTimerTimeout       00008590 f   SecPeiDebugAgentLib:DebugTimer.obj
>   0001:000083d0       _SaveAndSetDebugTimerInterrupt 00008630 f   SecPeiDebugAgentLib:DebugTimer.obj
>
> So it appears that even with -b NOOPT builds, the linker is
> filtering out unreferenced objs which explains why the duplicate
> symbol error is not triggered.
>
> The XCODE5 tool chain appears to be evaluating symbols
> across all objs in a lib and detects a duplicate symbol.

Thank you for the thorough investigation. I agree that keeping the name
change *and* adding STATIC is the best combination.

> The question is what is the required linker behavior for EDK II
> builds?  Require library filtering at obj level, or require no
> duplicates across all objs in all libs?

Consider "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

> CONST EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList = {
>   (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>   &gEfiPeiMemoryDiscoveredPpiGuid,
>   InstallIplPermanentMemoryPpis
> };

(a) This variable has external linkage. From ISO C99, "6.2.2 Linkages of
identifiers":

> 5 [...] If the declaration of an identifier for an object has file
>   scope and no storage-class specifier, its linkage is external.

(b) This variable has static storage duration. From ISO C99, "6.2.4
Storage durations of objects":

> 3 An object whose identifier is declared with external or internal
>   linkage, or with the storage-class specifier static has static
>   storage duration. Its lifetime is the entire execution of the
>   program and its stored value is initialized only once, prior to
>   program startup.

(c) The quoted declaration is an external definition of
"mMemoryDiscoveredNotifyList". From ISO C99, "6.9.2 External object
definitions":

> 1 If the declaration of an identifier for an object has file scope and
>   an initializer, the declaration is an external definition for the
>   identifier.

The exact same statements can be made about
"SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c":

> GLOBAL_REMOVE_IF_UNREFERENCED EFI_PEI_NOTIFY_DESCRIPTOR mMemoryDiscoveredNotifyList[1] = {
>   {
>     (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
>     &gEfiPeiMemoryDiscoveredPpiGuid,
>     DebugAgentCallbackMemoryDiscoveredPpi
>   }
> };

(I see "GLOBAL_REMOVE_IF_UNREFERENCED", but with GCC, that expands to
nothing.)

So, we have two external definitions for "mMemoryDiscoveredNotifyList".
(It's especially funny because even their types don't match.)

This is what ISO C99, "6.9 External definitions" says:

> Syntax
>
> [...]
>
> Constraints
>
> [...]
>
> Semantics
>
> [...]
>
> 5 [...] If an identifier declared with external linkage is used in an
>   expression (other than as part of the operand of a sizeof operator
>   whose result is an integer constant), somewhere in the entire
>   program there shall be exactly one external definition for the
>   identifier [...]

Given that we use "mMemoryDiscoveredNotifyList" in two expressions,
namely:

* "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c":

>     Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList);

* "SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c"

>     Status = PeiServicesNotifyPpi (&mMemoryDiscoveredNotifyList[0]);

the requirement definitely applies, and we're breaking it.


Now, the second question is, why don't some compilers complain about it?
The reason is that they don't have to (I missed this in my previous
email). This is what ISO C99, "5.1.1.3 Diagnostics" says:

> 1 A conforming implementation shall produce at least one diagnostic
>   message (identified in an implementation-defined manner) if a
>   preprocessing translation unit or translation unit contains a
>   violation of any syntax rule or constraint, even if the behavior is
>   also explicitly specified as undefined or implementation-defined.
>   Diagnostic messages need not be produced in other circumstances.
>   [...]

Note that when we break the requirement of "exactly one", we break
*Semantics*, not *Syntax* or *Constraints*. So the bug "only" causes
undefined behavior, and the compiler is not required to produce a
diagnostic message.

It's a good thing that the XCODE5 toolchain helped us catch this bug.

Thanks
Laszlo

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

[Fan, Jeff]

Reviewed-by: Jeff Fan <jeff.fan@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Michael Kinney
Sent: Wednesday, May 24, 2017 7:21 AM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A; Kinney, Michael D; Laszlo Ersek; Andrew Fish; Fan, Jeff
Subject: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

https://bugzilla.tianocore.org/show_bug.cgi?id=573

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.

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>
---
 .../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/SecPeiDebu
+++ gAgentLib.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

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

[Kinney, Michael D]

Laszlo and Andrew,

With the information that has been collected on this thread, I
still think this patch in its original form is a good change
to resolve the this one specific duplicate symbol issue for all
tool chains.  'static' can not be mixed with
GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
the global variable is the easiest way to remove the duplicate.

I will continue to work on ways to detect duplicate symbols for
all tool chains and will enter a Bugzilla issue to for that
feature.

In addition, the idea of detecting if a library is exporting more
than the library class defines is another good feature to consider
and I will enter a Bugzilla issue for that one as well.

If we can find ways to both restrict the symbols exported by a
library and strip all symbols that are unused, then we can have
additional Bugzilla issues to perform that clean up on each 
library instance that is exporting more than the library class.

Thanks,

Mike

> -----Original Message-----
> From: Fan, Jeff
> Sent: Tuesday, May 23, 2017 7:48 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew Fish
> <afish@apple.com>
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> Reviewed-by: Jeff Fan <jeff.fan@intel.com>
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Michael
> Kinney
> Sent: Wednesday, May 24, 2017 7:21 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Kinney, Michael D; Laszlo Ersek; Andrew Fish; Fan, Jeff
> Subject: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=573
> 
> 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.
> 
> 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>
> ---
>  .../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/SecPeiDebu
> +++ gAgentLib.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

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

[Ard Biesheuvel]

On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Laszlo and Andrew,
>
> With the information that has been collected on this thread, I
> still think this patch in its original form is a good change
> to resolve the this one specific duplicate symbol issue for all
> tool chains.  'static' can not be mixed with
> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
> the global variable is the easiest way to remove the duplicate.
>

GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
was Felix who reported on this recently?

STATIC is really the only sensible way to deal with this for symbols
that are only referenced by a single compilation unit.

> I will continue to work on ways to detect duplicate symbols for
> all tool chains and will enter a Bugzilla issue to for that
> feature.
>
> In addition, the idea of detecting if a library is exporting more
> than the library class defines is another good feature to consider
> and I will enter a Bugzilla issue for that one as well.
>
> If we can find ways to both restrict the symbols exported by a
> library and strip all symbols that are unused, then we can have
> additional Bugzilla issues to perform that clean up on each
> library instance that is exporting more than the library class.
>

A static library is nothing more than an archive containing a
collection of object files. Sadly, that implies that we cannot
distinguish between symbols that may only be referenced by other
objects in the same static library and symbols that are exported to
the library client.

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

[Laszlo Ersek]

On 05/25/17 22:11, Ard Biesheuvel wrote:
> On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>> Laszlo and Andrew,
>>
>> With the information that has been collected on this thread, I
>> still think this patch in its original form is a good change
>> to resolve the this one specific duplicate symbol issue for all
>> tool chains.  'static' can not be mixed with
>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>> the global variable is the easiest way to remove the duplicate.
>>
> 
> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
> was Felix who reported on this recently?
> 
> STATIC is really the only sensible way to deal with this for symbols
> that are only referenced by a single compilation unit.
> 
>> I will continue to work on ways to detect duplicate symbols for
>> all tool chains and will enter a Bugzilla issue to for that
>> feature.
>>
>> In addition, the idea of detecting if a library is exporting more
>> than the library class defines is another good feature to consider
>> and I will enter a Bugzilla issue for that one as well.
>>
>> If we can find ways to both restrict the symbols exported by a
>> library and strip all symbols that are unused, then we can have
>> additional Bugzilla issues to perform that clean up on each
>> library instance that is exporting more than the library class.
>>
> 
> A static library is nothing more than an archive containing a
> collection of object files. Sadly, that implies that we cannot
> distinguish between symbols that may only be referenced by other
> objects in the same static library and symbols that are exported to
> the library client.

Do we know for a fact that, with /OPT:REF, VS does not strip unused
*static* variables and functions?

I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
with STATIC in this case would lead to a size increase?

If that's the case, then I'm fine if we go ahead with this patch, I'd
just like to request that Mike please file some of those BZs, and please
reference them from the commit message (as the longer term solution),
before committing the patch.

Thanks
Laszlo

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

[Andrew Fish]

> On May 25, 2017, at 1:28 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 05/25/17 22:11, Ard Biesheuvel wrote:
>> On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>>> Laszlo and Andrew,
>>> 
>>> With the information that has been collected on this thread, I
>>> still think this patch in its original form is a good change
>>> to resolve the this one specific duplicate symbol issue for all
>>> tool chains.  'static' can not be mixed with
>>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>>> the global variable is the easiest way to remove the duplicate.
>>> 
>> 
>> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>> was Felix who reported on this recently?
>> 
>> STATIC is really the only sensible way to deal with this for symbols
>> that are only referenced by a single compilation unit.
>> 
>>> I will continue to work on ways to detect duplicate symbols for
>>> all tool chains and will enter a Bugzilla issue to for that
>>> feature.
>>> 
>>> In addition, the idea of detecting if a library is exporting more
>>> than the library class defines is another good feature to consider
>>> and I will enter a Bugzilla issue for that one as well.
>>> 
>>> If we can find ways to both restrict the symbols exported by a
>>> library and strip all symbols that are unused, then we can have
>>> additional Bugzilla issues to perform that clean up on each
>>> library instance that is exporting more than the library class.
>>> 
>> 
>> A static library is nothing more than an archive containing a
>> collection of object files. Sadly, that implies that we cannot
>> distinguish between symbols that may only be referenced by other
>> objects in the same static library and symbols that are exported to
>> the library client.
> 
> Do we know for a fact that, with /OPT:REF, VS does not strip unused
> *static* variables and functions?
> 
> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
> with STATIC in this case would lead to a size increase?
> 
> If that's the case, then I'm fine if we go ahead with this patch, I'd
> just like to request that Mike please file some of those BZs, and please
> reference them from the commit message (as the longer term solution),
> before committing the patch.
> 

Clang will warn if you have unused static variables when warnings are cranked up.

~/work/Compiler>cat static.c
static unsigned char gTest[] = { 42 };

static int test ()
{
  return 1;
}

int main ()
{
  return 0;
}
~/work/Compiler>clang -Os static.c -Wall
static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
static unsigned char gTest[] = { 42 };
                     ^
static.c:3:12: warning: unused function 'test' [-Wunused-function]
static int test ()
           ^
2 warnings generated.


Thanks,

Andrew Fish

> Thanks
> Laszlo

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

[Laszlo Ersek]

On 05/25/17 22:37, Andrew Fish wrote:
> 
>> On May 25, 2017, at 1:28 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 05/25/17 22:11, Ard Biesheuvel wrote:
>>> On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>>>> Laszlo and Andrew,
>>>>
>>>> With the information that has been collected on this thread, I
>>>> still think this patch in its original form is a good change
>>>> to resolve the this one specific duplicate symbol issue for all
>>>> tool chains.  'static' can not be mixed with
>>>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>>>> the global variable is the easiest way to remove the duplicate.
>>>>
>>>
>>> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>>> was Felix who reported on this recently?
>>>
>>> STATIC is really the only sensible way to deal with this for symbols
>>> that are only referenced by a single compilation unit.
>>>
>>>> I will continue to work on ways to detect duplicate symbols for
>>>> all tool chains and will enter a Bugzilla issue to for that
>>>> feature.
>>>>
>>>> In addition, the idea of detecting if a library is exporting more
>>>> than the library class defines is another good feature to consider
>>>> and I will enter a Bugzilla issue for that one as well.
>>>>
>>>> If we can find ways to both restrict the symbols exported by a
>>>> library and strip all symbols that are unused, then we can have
>>>> additional Bugzilla issues to perform that clean up on each
>>>> library instance that is exporting more than the library class.
>>>>
>>>
>>> A static library is nothing more than an archive containing a
>>> collection of object files. Sadly, that implies that we cannot
>>> distinguish between symbols that may only be referenced by other
>>> objects in the same static library and symbols that are exported to
>>> the library client.
>>
>> Do we know for a fact that, with /OPT:REF, VS does not strip unused
>> *static* variables and functions?
>>
>> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>> with STATIC in this case would lead to a size increase?
>>
>> If that's the case, then I'm fine if we go ahead with this patch, I'd
>> just like to request that Mike please file some of those BZs, and please
>> reference them from the commit message (as the longer term solution),
>> before committing the patch.
>>
> 
> Clang will warn if you have unused static variables when warnings are cranked up.
> 
> ~/work/Compiler>cat static.c
> static unsigned char gTest[] = { 42 };
> 
> static int test ()
> {
>   return 1;
> }
> 
> int main ()
> {
>   return 0;
> }
> ~/work/Compiler>clang -Os static.c -Wall
> static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
> static unsigned char gTest[] = { 42 };
>                      ^
> static.c:3:12: warning: unused function 'test' [-Wunused-function]
> static int test ()
>            ^
> 2 warnings generated.

Sorry, my question was imprecise.

Assume there is a public library function ("external linkage") that
calls a static function in the same library instance and uses a static
variable in the same library instance. Then this library instance is
linked into a driver, but the driver never actually calls the extern
function -- so the static variable and the static function too become
useless.

In this case, will /OPT:REF remove the static variable and the static
function too?

It seems counter-intuitive to me that an internal-only function or an
internal-only variable has to be declared extern (via
GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
time, if it is never referenced (transitively).

Thanks
Laszlo

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

[Andrew Fish]

> On May 25, 2017, at 2:02 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 05/25/17 22:37, Andrew Fish wrote:
>> 
>>> On May 25, 2017, at 1:28 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 
>>> On 05/25/17 22:11, Ard Biesheuvel wrote:
>>>> On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>>>>> Laszlo and Andrew,
>>>>> 
>>>>> With the information that has been collected on this thread, I
>>>>> still think this patch in its original form is a good change
>>>>> to resolve the this one specific duplicate symbol issue for all
>>>>> tool chains.  'static' can not be mixed with
>>>>> GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>>>>> the global variable is the easiest way to remove the duplicate.
>>>>> 
>>>> 
>>>> GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>>>> was Felix who reported on this recently?
>>>> 
>>>> STATIC is really the only sensible way to deal with this for symbols
>>>> that are only referenced by a single compilation unit.
>>>> 
>>>>> I will continue to work on ways to detect duplicate symbols for
>>>>> all tool chains and will enter a Bugzilla issue to for that
>>>>> feature.
>>>>> 
>>>>> In addition, the idea of detecting if a library is exporting more
>>>>> than the library class defines is another good feature to consider
>>>>> and I will enter a Bugzilla issue for that one as well.
>>>>> 
>>>>> If we can find ways to both restrict the symbols exported by a
>>>>> library and strip all symbols that are unused, then we can have
>>>>> additional Bugzilla issues to perform that clean up on each
>>>>> library instance that is exporting more than the library class.
>>>>> 
>>>> 
>>>> A static library is nothing more than an archive containing a
>>>> collection of object files. Sadly, that implies that we cannot
>>>> distinguish between symbols that may only be referenced by other
>>>> objects in the same static library and symbols that are exported to
>>>> the library client.
>>> 
>>> Do we know for a fact that, with /OPT:REF, VS does not strip unused
>>> *static* variables and functions?
>>> 
>>> I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>>> with STATIC in this case would lead to a size increase?
>>> 
>>> If that's the case, then I'm fine if we go ahead with this patch, I'd
>>> just like to request that Mike please file some of those BZs, and please
>>> reference them from the commit message (as the longer term solution),
>>> before committing the patch.
>>> 
>> 
>> Clang will warn if you have unused static variables when warnings are cranked up.
>> 
>> ~/work/Compiler>cat static.c
>> static unsigned char gTest[] = { 42 };
>> 
>> static int test ()
>> {
>>  return 1;
>> }
>> 
>> int main ()
>> {
>>  return 0;
>> }
>> ~/work/Compiler>clang -Os static.c -Wall
>> static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>> static unsigned char gTest[] = { 42 };
>>                     ^
>> static.c:3:12: warning: unused function 'test' [-Wunused-function]
>> static int test ()
>>           ^
>> 2 warnings generated.
> 
> Sorry, my question was imprecise.
> 
> Assume there is a public library function ("external linkage") that
> calls a static function in the same library instance and uses a static
> variable in the same library instance. Then this library instance is
> linked into a driver, but the driver never actually calls the extern
> function -- so the static variable and the static function too become
> useless.
> 
> In this case, will /OPT:REF remove the static variable and the static
> function too?
> 
> It seems counter-intuitive to me that an internal-only function or an
> internal-only variable has to be declared extern (via
> GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
> time, if it is never referenced (transitively).
> 

Laszlo,

I agree. The LLVM LTO does not have an issue "doing the right thing". Seems like static is also more of a compile time concept vs a link time (global optimization) kind of thing? 

Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec( selectany ) I would guess maybe it has more to due with supporting old non standard header files that can't change without breaking compatibility. 

MSDN on __declspec( selectany ) :
A global data item can normally be initialized only once in an EXE or DLL project. selectany can be used in initializing global data defined by headers, when the same header appears in more than one source file. selectany is available in both the C and C++ compilers.

Thanks,

Andrew Fish


> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

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

[Kinney, Michael D]

Andrew,

The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was added referred to __declspec( selectany ) as putting the symbol into its own comdat, so it was then available to be optimized away with the use of OPT:REF.

I think it is time to re-evaluate the VS optimizers to see if they can optimize away global variables without being decorated with__declspec( selectany ).  If we can remove __declspec( selectany ), then we have a path to use STATIC properly to hide global variables that are not declared as extern in the library class.

I will investigate some more.

Mike

From: afish@apple.com [mailto:afish@apple.com]
Sent: Thursday, May 25, 2017 2:26 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov <Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol


On May 25, 2017, at 2:02 PM, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

On 05/25/17 22:37, Andrew Fish wrote:



On May 25, 2017, at 1:28 PM, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

On 05/25/17 22:11, Ard Biesheuvel wrote:

On 25 May 2017 at 13:06, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

Laszlo and Andrew,

With the information that has been collected on this thread, I
still think this patch in its original form is a good change
to resolve the this one specific duplicate symbol issue for all
tool chains.  'static' can not be mixed with
GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
the global variable is the easiest way to remove the duplicate.

GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
was Felix who reported on this recently?

STATIC is really the only sensible way to deal with this for symbols
that are only referenced by a single compilation unit.


I will continue to work on ways to detect duplicate symbols for
all tool chains and will enter a Bugzilla issue to for that
feature.

In addition, the idea of detecting if a library is exporting more
than the library class defines is another good feature to consider
and I will enter a Bugzilla issue for that one as well.

If we can find ways to both restrict the symbols exported by a
library and strip all symbols that are unused, then we can have
additional Bugzilla issues to perform that clean up on each
library instance that is exporting more than the library class.

A static library is nothing more than an archive containing a
collection of object files. Sadly, that implies that we cannot
distinguish between symbols that may only be referenced by other
objects in the same static library and symbols that are exported to
the library client.

Do we know for a fact that, with /OPT:REF, VS does not strip unused
*static* variables and functions?

I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
with STATIC in this case would lead to a size increase?

If that's the case, then I'm fine if we go ahead with this patch, I'd
just like to request that Mike please file some of those BZs, and please
reference them from the commit message (as the longer term solution),
before committing the patch.

Clang will warn if you have unused static variables when warnings are cranked up.

~/work/Compiler>cat static.c
static unsigned char gTest[] = { 42 };

static int test ()
{
 return 1;
}

int main ()
{
 return 0;
}
~/work/Compiler>clang -Os static.c -Wall
static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
static unsigned char gTest[] = { 42 };
                    ^
static.c:3:12: warning: unused function 'test' [-Wunused-function]
static int test ()
          ^
2 warnings generated.

Sorry, my question was imprecise.

Assume there is a public library function ("external linkage") that
calls a static function in the same library instance and uses a static
variable in the same library instance. Then this library instance is
linked into a driver, but the driver never actually calls the extern
function -- so the static variable and the static function too become
useless.

In this case, will /OPT:REF remove the static variable and the static
function too?

It seems counter-intuitive to me that an internal-only function or an
internal-only variable has to be declared extern (via
GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
time, if it is never referenced (transitively).


Laszlo,

I agree. The LLVM LTO does not have an issue "doing the right thing". Seems like static is also more of a compile time concept vs a link time (global optimization) kind of thing?

Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec( selectany ) I would guess maybe it has more to due with supporting old non standard header files that can't change without breaking compatibility.

MSDN on __declspec( selectany ) :
A global data item can normally be initialized only once in an EXE or DLL project. selectany can be used in initializing global data defined by headers, when the same header appears in more than one source file. selectany is available in both the C and C++ compilers.

Thanks,

Andrew Fish



Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Gao, Liming]

Mike:
  I remember community suggests to use VS /Gw option to remove the global data, and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or static.  

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Kinney, Michael D
>Sent: Friday, May 26, 2017 6:42 AM
>To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D
><michael.d.kinney@intel.com>
>Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>Andrew,
>
>The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was
>added referred to __declspec( selectany ) as putting the symbol into its own
>comdat, so it was then available to be optimized away with the use of OPT:REF.
>
>I think it is time to re-evaluate the VS optimizers to see if they can optimize
>away global variables without being decorated with__declspec( selectany ).  If
>we can remove __declspec( selectany ), then we have a path to use STATIC
>properly to hide global variables that are not declared as extern in the library
>class.
>
>I will investigate some more.
>
>Mike
>
>From: afish@apple.com [mailto:afish@apple.com]
>Sent: Thursday, May 25, 2017 2:26 PM
>To: Laszlo Ersek <lersek@redhat.com>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fan,
>Jeff <jeff.fan@intel.com>
>Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>
>On May 25, 2017, at 2:02 PM, Laszlo Ersek
><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>
>On 05/25/17 22:37, Andrew Fish wrote:
>
>
>
>On May 25, 2017, at 1:28 PM, Laszlo Ersek
><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>
>On 05/25/17 22:11, Ard Biesheuvel wrote:
>
>On 25 May 2017 at 13:06, Kinney, Michael D
><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
>
>Laszlo and Andrew,
>
>With the information that has been collected on this thread, I
>still think this patch in its original form is a good change
>to resolve the this one specific duplicate symbol issue for all
>tool chains.  'static' can not be mixed with
>GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>the global variable is the easiest way to remove the duplicate.
>
>GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>was Felix who reported on this recently?
>
>STATIC is really the only sensible way to deal with this for symbols
>that are only referenced by a single compilation unit.
>
>
>I will continue to work on ways to detect duplicate symbols for
>all tool chains and will enter a Bugzilla issue to for that
>feature.
>
>In addition, the idea of detecting if a library is exporting more
>than the library class defines is another good feature to consider
>and I will enter a Bugzilla issue for that one as well.
>
>If we can find ways to both restrict the symbols exported by a
>library and strip all symbols that are unused, then we can have
>additional Bugzilla issues to perform that clean up on each
>library instance that is exporting more than the library class.
>
>A static library is nothing more than an archive containing a
>collection of object files. Sadly, that implies that we cannot
>distinguish between symbols that may only be referenced by other
>objects in the same static library and symbols that are exported to
>the library client.
>
>Do we know for a fact that, with /OPT:REF, VS does not strip unused
>*static* variables and functions?
>
>I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>with STATIC in this case would lead to a size increase?
>
>If that's the case, then I'm fine if we go ahead with this patch, I'd
>just like to request that Mike please file some of those BZs, and please
>reference them from the commit message (as the longer term solution),
>before committing the patch.
>
>Clang will warn if you have unused static variables when warnings are cranked
>up.
>
>~/work/Compiler>cat static.c
>static unsigned char gTest[] = { 42 };
>
>static int test ()
>{
> return 1;
>}
>
>int main ()
>{
> return 0;
>}
>~/work/Compiler>clang -Os static.c -Wall
>static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>static unsigned char gTest[] = { 42 };
>                    ^
>static.c:3:12: warning: unused function 'test' [-Wunused-function]
>static int test ()
>          ^
>2 warnings generated.
>
>Sorry, my question was imprecise.
>
>Assume there is a public library function ("external linkage") that
>calls a static function in the same library instance and uses a static
>variable in the same library instance. Then this library instance is
>linked into a driver, but the driver never actually calls the extern
>function -- so the static variable and the static function too become
>useless.
>
>In this case, will /OPT:REF remove the static variable and the static
>function too?
>
>It seems counter-intuitive to me that an internal-only function or an
>internal-only variable has to be declared extern (via
>GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
>time, if it is never referenced (transitively).
>
>
>Laszlo,
>
>I agree. The LLVM LTO does not have an issue "doing the right thing". Seems
>like static is also more of a compile time concept vs a link time (global
>optimization) kind of thing?
>
>Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to
>__declspec( selectany ) I would guess maybe it has more to due with
>supporting old non standard header files that can't change without breaking
>compatibility.
>
>MSDN on __declspec( selectany ) :
>A global data item can normally be initialized only once in an EXE or DLL project.
>selectany can be used in initializing global data defined by headers, when the
>same header appears in more than one source file. selectany is available in
>both the C and C++ compilers.
>
>Thanks,
>
>Andrew Fish
>
>
>
>Thanks
>Laszlo
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org<mailto: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

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

[Kinney, Michael D]

Liming,

I agree with /Gw.  That works for newer versions of VS.  We will
need to adjust the behavior of GLOBAL_REMOVE_IF_UNREFERENCED based
on VS version as well.

We can not define GLOBAL_REMOVE_IF_UNREFERENCED to static.  We also
use this macro for globals that are required to be exported from
a library.  So static should be added to the globals that are not
exported.

The challenge is that older versions of VS require 
GLOBAL_REMOVE_IF_UNREFERENCED to be mapped to __declspec(selectany)
and static can not be combined with __declspec(selectany).

Mike

> -----Original Message-----
> From: Gao, Liming
> Sent: Thursday, May 25, 2017 10:21 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> Mike:
>   I remember community suggests to use VS /Gw option to remove the global data,
> and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or static.
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >Kinney, Michael D
> >Sent: Friday, May 26, 2017 6:42 AM
> >To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D
> ><michael.d.kinney@intel.com>
> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> ><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
> ><ard.biesheuvel@linaro.org>
> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> >duplicate symbol
> >
> >Andrew,
> >
> >The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED was
> >added referred to __declspec( selectany ) as putting the symbol into its own
> >comdat, so it was then available to be optimized away with the use of OPT:REF.
> >
> >I think it is time to re-evaluate the VS optimizers to see if they can optimize
> >away global variables without being decorated with__declspec( selectany ).  If
> >we can remove __declspec( selectany ), then we have a path to use STATIC
> >properly to hide global variables that are not declared as extern in the library
> >class.
> >
> >I will investigate some more.
> >
> >Mike
> >
> >From: afish@apple.com [mailto:afish@apple.com]
> >Sent: Thursday, May 25, 2017 2:26 PM
> >To: Laszlo Ersek <lersek@redhat.com>
> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
> ><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
> ><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fan,
> >Jeff <jeff.fan@intel.com>
> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> >duplicate symbol
> >
> >
> >On May 25, 2017, at 2:02 PM, Laszlo Ersek
> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
> >
> >On 05/25/17 22:37, Andrew Fish wrote:
> >
> >
> >
> >On May 25, 2017, at 1:28 PM, Laszlo Ersek
> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
> >
> >On 05/25/17 22:11, Ard Biesheuvel wrote:
> >
> >On 25 May 2017 at 13:06, Kinney, Michael D
> ><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
> >
> >Laszlo and Andrew,
> >
> >With the information that has been collected on this thread, I
> >still think this patch in its original form is a good change
> >to resolve the this one specific duplicate symbol issue for all
> >tool chains.  'static' can not be mixed with
> >GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
> >the global variable is the easiest way to remove the duplicate.
> >
> >GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
> >was Felix who reported on this recently?
> >
> >STATIC is really the only sensible way to deal with this for symbols
> >that are only referenced by a single compilation unit.
> >
> >
> >I will continue to work on ways to detect duplicate symbols for
> >all tool chains and will enter a Bugzilla issue to for that
> >feature.
> >
> >In addition, the idea of detecting if a library is exporting more
> >than the library class defines is another good feature to consider
> >and I will enter a Bugzilla issue for that one as well.
> >
> >If we can find ways to both restrict the symbols exported by a
> >library and strip all symbols that are unused, then we can have
> >additional Bugzilla issues to perform that clean up on each
> >library instance that is exporting more than the library class.
> >
> >A static library is nothing more than an archive containing a
> >collection of object files. Sadly, that implies that we cannot
> >distinguish between symbols that may only be referenced by other
> >objects in the same static library and symbols that are exported to
> >the library client.
> >
> >Do we know for a fact that, with /OPT:REF, VS does not strip unused
> >*static* variables and functions?
> >
> >I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
> >with STATIC in this case would lead to a size increase?
> >
> >If that's the case, then I'm fine if we go ahead with this patch, I'd
> >just like to request that Mike please file some of those BZs, and please
> >reference them from the commit message (as the longer term solution),
> >before committing the patch.
> >
> >Clang will warn if you have unused static variables when warnings are cranked
> >up.
> >
> >~/work/Compiler>cat static.c
> >static unsigned char gTest[] = { 42 };
> >
> >static int test ()
> >{
> > return 1;
> >}
> >
> >int main ()
> >{
> > return 0;
> >}
> >~/work/Compiler>clang -Os static.c -Wall
> >static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
> >static unsigned char gTest[] = { 42 };
> >                    ^
> >static.c:3:12: warning: unused function 'test' [-Wunused-function]
> >static int test ()
> >          ^
> >2 warnings generated.
> >
> >Sorry, my question was imprecise.
> >
> >Assume there is a public library function ("external linkage") that
> >calls a static function in the same library instance and uses a static
> >variable in the same library instance. Then this library instance is
> >linked into a driver, but the driver never actually calls the extern
> >function -- so the static variable and the static function too become
> >useless.
> >
> >In this case, will /OPT:REF remove the static variable and the static
> >function too?
> >
> >It seems counter-intuitive to me that an internal-only function or an
> >internal-only variable has to be declared extern (via
> >GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
> >time, if it is never referenced (transitively).
> >
> >
> >Laszlo,
> >
> >I agree. The LLVM LTO does not have an issue "doing the right thing". Seems
> >like static is also more of a compile time concept vs a link time (global
> >optimization) kind of thing?
> >
> >Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to
> >__declspec( selectany ) I would guess maybe it has more to due with
> >supporting old non standard header files that can't change without breaking
> >compatibility.
> >
> >MSDN on __declspec( selectany ) :
> >A global data item can normally be initialized only once in an EXE or DLL
> project.
> >selectany can be used in initializing global data defined by headers, when the
> >same header appears in more than one source file. selectany is available in
> >both the C and C++ compilers.
> >
> >Thanks,
> >
> >Andrew Fish
> >
> >
> >
> >Thanks
> >Laszlo
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org<mailto: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

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

[Gao, Liming]

Mike:
  Yes. /Gw option is added since VS2013. The older VS version can't use this option. I suggest we always define GLOBAL_REMOVE_IF_UNREFERENCED as empty, and drop this size optimization for the older version MS compiler. I collect its size of OvmfIa32X64 DEBUG tip with VS2015 tool chain on. After define it as empty, DXE Raw size increases ~55K, but PEI raw size and the compressed size doesn't increase big. 

1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Space Information
SECFV [10%Full] 212992 total, 22816 used, 190176 free
FVMAIN_COMPACT [62%Full] 1753088 total, 1099872 used, 653216 free
DXEFV [39%Full] 10485760 total, 4099344 used, 6386416 free
PEIFV [18%Full] 917504 total, 172072 used, 745432 free

2. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty. FV Space Information
SECFV [10%Full] 212992 total, 22912 used, 190080 free
FVMAIN_COMPACT [63%Full] 1753088 total, 1105992 used, 647096 free
DXEFV [39%Full] 10485760 total, 4154480 used, 6331280 free
PEIFV [18%Full] 917504 total, 173448 used, 744056 free

FVMAIN_COMPACT +6120
DXEFV  + 55136
PEIFV    + 1376

3. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty and append /Gw option. FV Space Information.
SECFV [10%Full] 212992 total, 22816 used, 190176 free
FVMAIN_COMPACT [62%Full] 1753088 total, 1099552 used, 653536 free
DXEFV [39%Full] 10485760 total, 4097456 used, 6388304 free
PEIFV [18%Full] 917504 total, 171944 used, 745560 free

FVMAIN_COMPACT -320
DXEFV  -1888
PEIFV    -128

Thanks
Liming
>-----Original Message-----
>From: Kinney, Michael D
>Sent: Friday, May 26, 2017 2:20 PM
>To: Gao, Liming <liming.gao@intel.com>; afish@apple.com; Laszlo Ersek
><lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>duplicate symbol
>
>Liming,
>
>I agree with /Gw.  That works for newer versions of VS.  We will
>need to adjust the behavior of GLOBAL_REMOVE_IF_UNREFERENCED based
>on VS version as well.
>
>We can not define GLOBAL_REMOVE_IF_UNREFERENCED to static.  We also
>use this macro for globals that are required to be exported from
>a library.  So static should be added to the globals that are not
>exported.
>
>The challenge is that older versions of VS require
>GLOBAL_REMOVE_IF_UNREFERENCED to be mapped to __declspec(selectany)
>and static can not be combined with __declspec(selectany).
>
>Mike
>
>> -----Original Message-----
>> From: Gao, Liming
>> Sent: Thursday, May 25, 2017 10:21 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com;
>Laszlo Ersek
>> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>> <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
>> duplicate symbol
>>
>> Mike:
>>   I remember community suggests to use VS /Gw option to remove the
>global data,
>> and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or
>static.
>>
>> Thanks
>> Liming
>> >-----Original Message-----
>> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> >Kinney, Michael D
>> >Sent: Friday, May 26, 2017 6:42 AM
>> >To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael
>D
>> ><michael.d.kinney@intel.com>
>> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
>> ><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
>> ><ard.biesheuvel@linaro.org>
>> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> >duplicate symbol
>> >
>> >Andrew,
>> >
>> >The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED
>was
>> >added referred to __declspec( selectany ) as putting the symbol into its
>own
>> >comdat, so it was then available to be optimized away with the use of
>OPT:REF.
>> >
>> >I think it is time to re-evaluate the VS optimizers to see if they can optimize
>> >away global variables without being decorated with__declspec( selectany ).
>If
>> >we can remove __declspec( selectany ), then we have a path to use
>STATIC
>> >properly to hide global variables that are not declared as extern in the
>library
>> >class.
>> >
>> >I will investigate some more.
>> >
>> >Mike
>> >
>> >From: afish@apple.com [mailto:afish@apple.com]
>> >Sent: Thursday, May 25, 2017 2:26 PM
>> >To: Laszlo Ersek <lersek@redhat.com>
>> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
>> ><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
>> ><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fan,
>> >Jeff <jeff.fan@intel.com>
>> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> >duplicate symbol
>> >
>> >
>> >On May 25, 2017, at 2:02 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:37, Andrew Fish wrote:
>> >
>> >
>> >
>> >On May 25, 2017, at 1:28 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:11, Ard Biesheuvel wrote:
>> >
>> >On 25 May 2017 at 13:06, Kinney, Michael D
>> ><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>wrote:
>> >
>> >Laszlo and Andrew,
>> >
>> >With the information that has been collected on this thread, I
>> >still think this patch in its original form is a good change
>> >to resolve the this one specific duplicate symbol issue for all
>> >tool chains.  'static' can not be mixed with
>> >GLOBAL_REMOVE_IF_UNREFERENCED for MSFT tool chains, so renaming
>> >the global variable is the easiest way to remove the duplicate.
>> >
>> >GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>> >was Felix who reported on this recently?
>> >
>> >STATIC is really the only sensible way to deal with this for symbols
>> >that are only referenced by a single compilation unit.
>> >
>> >
>> >I will continue to work on ways to detect duplicate symbols for
>> >all tool chains and will enter a Bugzilla issue to for that
>> >feature.
>> >
>> >In addition, the idea of detecting if a library is exporting more
>> >than the library class defines is another good feature to consider
>> >and I will enter a Bugzilla issue for that one as well.
>> >
>> >If we can find ways to both restrict the symbols exported by a
>> >library and strip all symbols that are unused, then we can have
>> >additional Bugzilla issues to perform that clean up on each
>> >library instance that is exporting more than the library class.
>> >
>> >A static library is nothing more than an archive containing a
>> >collection of object files. Sadly, that implies that we cannot
>> >distinguish between symbols that may only be referenced by other
>> >objects in the same static library and symbols that are exported to
>> >the library client.
>> >
>> >Do we know for a fact that, with /OPT:REF, VS does not strip unused
>> >*static* variables and functions?
>> >
>> >I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>> >with STATIC in this case would lead to a size increase?
>> >
>> >If that's the case, then I'm fine if we go ahead with this patch, I'd
>> >just like to request that Mike please file some of those BZs, and please
>> >reference them from the commit message (as the longer term solution),
>> >before committing the patch.
>> >
>> >Clang will warn if you have unused static variables when warnings are
>cranked
>> >up.
>> >
>> >~/work/Compiler>cat static.c
>> >static unsigned char gTest[] = { 42 };
>> >
>> >static int test ()
>> >{
>> > return 1;
>> >}
>> >
>> >int main ()
>> >{
>> > return 0;
>> >}
>> >~/work/Compiler>clang -Os static.c -Wall
>> >static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>> >static unsigned char gTest[] = { 42 };
>> >                    ^
>> >static.c:3:12: warning: unused function 'test' [-Wunused-function]
>> >static int test ()
>> >          ^
>> >2 warnings generated.
>> >
>> >Sorry, my question was imprecise.
>> >
>> >Assume there is a public library function ("external linkage") that
>> >calls a static function in the same library instance and uses a static
>> >variable in the same library instance. Then this library instance is
>> >linked into a driver, but the driver never actually calls the extern
>> >function -- so the static variable and the static function too become
>> >useless.
>> >
>> >In this case, will /OPT:REF remove the static variable and the static
>> >function too?
>> >
>> >It seems counter-intuitive to me that an internal-only function or an
>> >internal-only variable has to be declared extern (via
>> >GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
>> >time, if it is never referenced (transitively).
>> >
>> >
>> >Laszlo,
>> >
>> >I agree. The LLVM LTO does not have an issue "doing the right thing".
>Seems
>> >like static is also more of a compile time concept vs a link time (global
>> >optimization) kind of thing?
>> >
>> >Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to
>> >__declspec( selectany ) I would guess maybe it has more to due with
>> >supporting old non standard header files that can't change without
>breaking
>> >compatibility.
>> >
>> >MSDN on __declspec( selectany ) :
>> >A global data item can normally be initialized only once in an EXE or DLL
>> project.
>> >selectany can be used in initializing global data defined by headers, when
>the
>> >same header appears in more than one source file. selectany is available in
>> >both the C and C++ compilers.
>> >
>> >Thanks,
>> >
>> >Andrew Fish
>> >
>> >
>> >
>> >Thanks
>> >Laszlo
>> >_______________________________________________
>> >edk2-devel mailing list
>> >edk2-devel@lists.01.org<mailto: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

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

[Felix Poludov]

Another option to support older VS tool chains is to define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany) only for these tool chains.
One way to do it is to use _MSC_VER macro:
#if _MSC_VER < ....
#define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany)
#endif

Alternatively GLOBAL_REMOVE_IF_UNREFERENCED can be defined for specific tool chains in the tools_def.txt using /D compiler switch.

-----Original Message-----
From: Gao, Liming [mailto:liming.gao@intel.com]
Sent: Friday, May 26, 2017 4:42 AM
To: Kinney, Michael D; afish@apple.com; Laszlo Ersek
Cc: Wu, Hao A; edk2-devel@lists.01.org; Fan, Jeff; Felix Poludov; Ard Biesheuvel
Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

Mike:
  Yes. /Gw option is added since VS2013. The older VS version can't use this option. I suggest we always define GLOBAL_REMOVE_IF_UNREFERENCED as empty, and drop this size optimization for the older version MS compiler. I collect its size of OvmfIa32X64 DEBUG tip with VS2015 tool chain on. After define it as empty, DXE Raw size increases ~55K, but PEI raw size and the compressed size doesn't increase big.

1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Space Information SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%Full] 1753088 total, 1099872 used, 653216 free DXEFV [39%Full] 10485760 total, 4099344 used, 6386416 free PEIFV [18%Full] 917504 total, 172072 used, 745432 free

2. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty. FV Space Information SECFV [10%Full] 212992 total, 22912 used, 190080 free FVMAIN_COMPACT [63%Full] 1753088 total, 1105992 used, 647096 free DXEFV [39%Full] 10485760 total, 4154480 used, 6331280 free PEIFV [18%Full] 917504 total, 173448 used, 744056 free

FVMAIN_COMPACT +6120
DXEFV  + 55136
PEIFV    + 1376

3. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty and append /Gw option. FV Space Information.
SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%Full] 1753088 total, 1099552 used, 653536 free DXEFV [39%Full] 10485760 total, 4097456 used, 6388304 free PEIFV [18%Full] 917504 total, 171944 used, 745560 free

FVMAIN_COMPACT -320
DXEFV  -1888
PEIFV    -128

Thanks
Liming
>-----Original Message-----
>From: Kinney, Michael D
>Sent: Friday, May 26, 2017 2:20 PM
>To: Gao, Liming <liming.gao@intel.com>; afish@apple.com; Laszlo Ersek
><lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix duplicate symbol
>
>Liming,
>
>I agree with /Gw.  That works for newer versions of VS.  We will need
>to adjust the behavior of GLOBAL_REMOVE_IF_UNREFERENCED based on VS
>version as well.
>
>We can not define GLOBAL_REMOVE_IF_UNREFERENCED to static.  We also use
>this macro for globals that are required to be exported from a library.
>So static should be added to the globals that are not exported.
>
>The challenge is that older versions of VS require
>GLOBAL_REMOVE_IF_UNREFERENCED to be mapped to __declspec(selectany) and
>static can not be combined with __declspec(selectany).
>
>Mike
>
>> -----Original Message-----
>> From: Gao, Liming
>> Sent: Thursday, May 25, 2017 10:21 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com;
>Laszlo Ersek
>> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
>> Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>> Fix duplicate symbol
>>
>> Mike:
>>   I remember community suggests to use VS /Gw option to remove the
>global data,
>> and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or
>static.
>>
>> Thanks
>> Liming
>> >-----Original Message-----
>> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> >Of Kinney, Michael D
>> >Sent: Friday, May 26, 2017 6:42 AM
>> >To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; Kinney,
>> >Michael
>D
>> ><michael.d.kinney@intel.com>
>> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
>> >Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
>> >Biesheuvel <ard.biesheuvel@linaro.org>
>> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> >duplicate symbol
>> >
>> >Andrew,
>> >
>> >The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED
>was
>> >added referred to __declspec( selectany ) as putting the symbol into
>> >its
>own
>> >comdat, so it was then available to be optimized away with the use
>> >of
>OPT:REF.
>> >
>> >I think it is time to re-evaluate the VS optimizers to see if they
>> >can optimize away global variables without being decorated with__declspec( selectany ).
>If
>> >we can remove __declspec( selectany ), then we have a path to use
>STATIC
>> >properly to hide global variables that are not declared as extern in
>> >the
>library
>> >class.
>> >
>> >I will investigate some more.
>> >
>> >Mike
>> >
>> >From: afish@apple.com [mailto:afish@apple.com]
>> >Sent: Thursday, May 25, 2017 2:26 PM
>> >To: Laszlo Ersek <lersek@redhat.com>
>> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
>> ><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
>> ><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> >Fan, Jeff <jeff.fan@intel.com>
>> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
>Fix
>> >duplicate symbol
>> >
>> >
>> >On May 25, 2017, at 2:02 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:37, Andrew Fish wrote:
>> >
>> >
>> >
>> >On May 25, 2017, at 1:28 PM, Laszlo Ersek
>> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>> >
>> >On 05/25/17 22:11, Ard Biesheuvel wrote:
>> >
>> >On 25 May 2017 at 13:06, Kinney, Michael D
>> ><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
>wrote:
>> >
>> >Laszlo and Andrew,
>> >
>> >With the information that has been collected on this thread, I still
>> >think this patch in its original form is a good change to resolve
>> >the this one specific duplicate symbol issue for all tool chains.
>> >'static' can not be mixed with GLOBAL_REMOVE_IF_UNREFERENCED for
>> >MSFT tool chains, so renaming the global variable is the easiest way
>> >to remove the duplicate.
>> >
>> >GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
>> >was Felix who reported on this recently?
>> >
>> >STATIC is really the only sensible way to deal with this for symbols
>> >that are only referenced by a single compilation unit.
>> >
>> >
>> >I will continue to work on ways to detect duplicate symbols for all
>> >tool chains and will enter a Bugzilla issue to for that feature.
>> >
>> >In addition, the idea of detecting if a library is exporting more
>> >than the library class defines is another good feature to consider
>> >and I will enter a Bugzilla issue for that one as well.
>> >
>> >If we can find ways to both restrict the symbols exported by a
>> >library and strip all symbols that are unused, then we can have
>> >additional Bugzilla issues to perform that clean up on each library
>> >instance that is exporting more than the library class.
>> >
>> >A static library is nothing more than an archive containing a
>> >collection of object files. Sadly, that implies that we cannot
>> >distinguish between symbols that may only be referenced by other
>> >objects in the same static library and symbols that are exported to
>> >the library client.
>> >
>> >Do we know for a fact that, with /OPT:REF, VS does not strip unused
>> >*static* variables and functions?
>> >
>> >I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
>> >with STATIC in this case would lead to a size increase?
>> >
>> >If that's the case, then I'm fine if we go ahead with this patch,
>> >I'd just like to request that Mike please file some of those BZs,
>> >and please reference them from the commit message (as the longer
>> >term solution), before committing the patch.
>> >
>> >Clang will warn if you have unused static variables when warnings
>> >are
>cranked
>> >up.
>> >
>> >~/work/Compiler>cat static.c
>> >static unsigned char gTest[] = { 42 };
>> >
>> >static int test ()
>> >{
>> > return 1;
>> >}
>> >
>> >int main ()
>> >{
>> > return 0;
>> >}
>> >~/work/Compiler>clang -Os static.c -Wall
>> >static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
>> >static unsigned char gTest[] = { 42 };
>> >                    ^
>> >static.c:3:12: warning: unused function 'test' [-Wunused-function]
>> >static int test ()
>> >          ^
>> >2 warnings generated.
>> >
>> >Sorry, my question was imprecise.
>> >
>> >Assume there is a public library function ("external linkage") that
>> >calls a static function in the same library instance and uses a
>> >static variable in the same library instance. Then this library
>> >instance is linked into a driver, but the driver never actually
>> >calls the extern function -- so the static variable and the static
>> >function too become useless.
>> >
>> >In this case, will /OPT:REF remove the static variable and the
>> >static function too?
>> >
>> >It seems counter-intuitive to me that an internal-only function or
>> >an internal-only variable has to be declared extern (via
>> >GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
>> >time, if it is never referenced (transitively).
>> >
>> >
>> >Laszlo,
>> >
>> >I agree. The LLVM LTO does not have an issue "doing the right thing".
>Seems
>> >like static is also more of a compile time concept vs a link time
>> >(global
>> >optimization) kind of thing?
>> >
>> >Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec(
>> >selectany ) I would guess maybe it has more to due with supporting
>> >old non standard header files that can't change without
>breaking
>> >compatibility.
>> >
>> >MSDN on __declspec( selectany ) :
>> >A global data item can normally be initialized only once in an EXE
>> >or DLL
>> project.
>> >selectany can be used in initializing global data defined by
>> >headers, when
>the
>> >same header appears in more than one source file. selectany is
>> >available in both the C and C++ compilers.
>> >
>> >Thanks,
>> >
>> >Andrew Fish
>> >
>> >
>> >
>> >Thanks
>> >Laszlo
>> >_______________________________________________
>> >edk2-devel mailing list
>> >edk2-devel@lists.01.org<mailto: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

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

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

[Kinney, Michael D]

Felix,

Yes.  I agree.  I will work on a Bugzilla issue for this topic
and I prefer the idea of updating Base.h to check _MSC_VER value.

The one challenge is that 'static' could be added in front of
GLOBAL_REMOVE_IF_UNREFERENCED, and it will build correctly with
VS2012 and newer, but would fail with older VS versions that 
require __declspec(selectany) for size optimization.

Thanks,

Mike

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, May 26, 2017 3:12 PM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com; Laszlo Ersek <lersek@redhat.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> <jeff.fan@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> Another option to support older VS tool chains is to define
> GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany) only for these tool
> chains.
> One way to do it is to use _MSC_VER macro:
> #if _MSC_VER < ....
> #define GLOBAL_REMOVE_IF_UNREFERENCED __declspec(selectany)
> #endif
> 
> Alternatively GLOBAL_REMOVE_IF_UNREFERENCED can be defined for specific tool
> chains in the tools_def.txt using /D compiler switch.
> 
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Friday, May 26, 2017 4:42 AM
> To: Kinney, Michael D; afish@apple.com; Laszlo Ersek
> Cc: Wu, Hao A; edk2-devel@lists.01.org; Fan, Jeff; Felix Poludov; Ard Biesheuvel
> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> Mike:
>   Yes. /Gw option is added since VS2013. The older VS version can't use this
> option. I suggest we always define GLOBAL_REMOVE_IF_UNREFERENCED as empty, and
> drop this size optimization for the older version MS compiler. I collect its size
> of OvmfIa32X64 DEBUG tip with VS2015 tool chain on. After define it as empty, DXE
> Raw size increases ~55K, but PEI raw size and the compressed size doesn't
> increase big.
> 
> 1. Define GLOBAL_REMOVE_IF_UNREFERENCED as __declspec(selectany). FV Space
> Information SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT
> [62%Full] 1753088 total, 1099872 used, 653216 free DXEFV [39%Full] 10485760
> total, 4099344 used, 6386416 free PEIFV [18%Full] 917504 total, 172072 used,
> 745432 free
> 
> 2. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty. FV Space Information SECFV
> [10%Full] 212992 total, 22912 used, 190080 free FVMAIN_COMPACT [63%Full] 1753088
> total, 1105992 used, 647096 free DXEFV [39%Full] 10485760 total, 4154480 used,
> 6331280 free PEIFV [18%Full] 917504 total, 173448 used, 744056 free
> 
> FVMAIN_COMPACT +6120
> DXEFV  + 55136
> PEIFV    + 1376
> 
> 3. Define GLOBAL_REMOVE_IF_UNREFERENCED as empty and append /Gw option. FV Space
> Information.
> SECFV [10%Full] 212992 total, 22816 used, 190176 free FVMAIN_COMPACT [62%Full]
> 1753088 total, 1099552 used, 653536 free DXEFV [39%Full] 10485760 total, 4097456
> used, 6388304 free PEIFV [18%Full] 917504 total, 171944 used, 745560 free
> 
> FVMAIN_COMPACT -320
> DXEFV  -1888
> PEIFV    -128
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Kinney, Michael D
> >Sent: Friday, May 26, 2017 2:20 PM
> >To: Gao, Liming <liming.gao@intel.com>; afish@apple.com; Laszlo Ersek
> ><lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan, Jeff
> ><jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard Biesheuvel
> ><ard.biesheuvel@linaro.org>
> >Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
> >Fix duplicate symbol
> >
> >Liming,
> >
> >I agree with /Gw.  That works for newer versions of VS.  We will need
> >to adjust the behavior of GLOBAL_REMOVE_IF_UNREFERENCED based on VS
> >version as well.
> >
> >We can not define GLOBAL_REMOVE_IF_UNREFERENCED to static.  We also use
> >this macro for globals that are required to be exported from a library.
> >So static should be added to the globals that are not exported.
> >
> >The challenge is that older versions of VS require
> >GLOBAL_REMOVE_IF_UNREFERENCED to be mapped to __declspec(selectany) and
> >static can not be combined with __declspec(selectany).
> >
> >Mike
> >
> >> -----Original Message-----
> >> From: Gao, Liming
> >> Sent: Thursday, May 25, 2017 10:21 PM
> >> To: Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com;
> >Laszlo Ersek
> >> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
> >> Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
> >> Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: RE: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
> >> Fix duplicate symbol
> >>
> >> Mike:
> >>   I remember community suggests to use VS /Gw option to remove the
> >global data,
> >> and then can define GLOBAL_REMOVE_IF_UNREFERENCED as empty or
> >static.
> >>
> >> Thanks
> >> Liming
> >> >-----Original Message-----
> >> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> >Of Kinney, Michael D
> >> >Sent: Friday, May 26, 2017 6:42 AM
> >> >To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>; Kinney,
> >> >Michael
> >D
> >> ><michael.d.kinney@intel.com>
> >> >Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Fan,
> >> >Jeff <jeff.fan@intel.com>; Felix Poludov <Felixp@ami.com>; Ard
> >> >Biesheuvel <ard.biesheuvel@linaro.org>
> >> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
> >Fix
> >> >duplicate symbol
> >> >
> >> >Andrew,
> >> >
> >> >The VS compilers available when GLOBAL_REMOVE_IF_UNREFERENCED
> >was
> >> >added referred to __declspec( selectany ) as putting the symbol into
> >> >its
> >own
> >> >comdat, so it was then available to be optimized away with the use
> >> >of
> >OPT:REF.
> >> >
> >> >I think it is time to re-evaluate the VS optimizers to see if they
> >> >can optimize away global variables without being decorated with__declspec(
> selectany ).
> >If
> >> >we can remove __declspec( selectany ), then we have a path to use
> >STATIC
> >> >properly to hide global variables that are not declared as extern in
> >> >the
> >library
> >> >class.
> >> >
> >> >I will investigate some more.
> >> >
> >> >Mike
> >> >
> >> >From: afish@apple.com [mailto:afish@apple.com]
> >> >Sent: Thursday, May 25, 2017 2:26 PM
> >> >To: Laszlo Ersek <lersek@redhat.com>
> >> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Wu, Hao A
> >> ><hao.a.wu@intel.com>; edk2-devel@lists.01.org; Felix Poludov
> >> ><Felixp@ami.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >> >Fan, Jeff <jeff.fan@intel.com>
> >> >Subject: Re: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib:
> >Fix
> >> >duplicate symbol
> >> >
> >> >
> >> >On May 25, 2017, at 2:02 PM, Laszlo Ersek
> >> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
> >> >
> >> >On 05/25/17 22:37, Andrew Fish wrote:
> >> >
> >> >
> >> >
> >> >On May 25, 2017, at 1:28 PM, Laszlo Ersek
> >> ><lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
> >> >
> >> >On 05/25/17 22:11, Ard Biesheuvel wrote:
> >> >
> >> >On 25 May 2017 at 13:06, Kinney, Michael D
> >> ><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> >wrote:
> >> >
> >> >Laszlo and Andrew,
> >> >
> >> >With the information that has been collected on this thread, I still
> >> >think this patch in its original form is a good change to resolve
> >> >the this one specific duplicate symbol issue for all tool chains.
> >> >'static' can not be mixed with GLOBAL_REMOVE_IF_UNREFERENCED for
> >> >MSFT tool chains, so renaming the global variable is the easiest way
> >> >to remove the duplicate.
> >> >
> >> >GLOBAL_REMOVE_IF_UNREFERENCED itself is problematic imo. I think it
> >> >was Felix who reported on this recently?
> >> >
> >> >STATIC is really the only sensible way to deal with this for symbols
> >> >that are only referenced by a single compilation unit.
> >> >
> >> >
> >> >I will continue to work on ways to detect duplicate symbols for all
> >> >tool chains and will enter a Bugzilla issue to for that feature.
> >> >
> >> >In addition, the idea of detecting if a library is exporting more
> >> >than the library class defines is another good feature to consider
> >> >and I will enter a Bugzilla issue for that one as well.
> >> >
> >> >If we can find ways to both restrict the symbols exported by a
> >> >library and strip all symbols that are unused, then we can have
> >> >additional Bugzilla issues to perform that clean up on each library
> >> >instance that is exporting more than the library class.
> >> >
> >> >A static library is nothing more than an archive containing a
> >> >collection of object files. Sadly, that implies that we cannot
> >> >distinguish between symbols that may only be referenced by other
> >> >objects in the same static library and symbols that are exported to
> >> >the library client.
> >> >
> >> >Do we know for a fact that, with /OPT:REF, VS does not strip unused
> >> >*static* variables and functions?
> >> >
> >> >I mean, is it certain that *replacing* GLOBAL_REMOVE_IF_UNREFERENCED
> >> >with STATIC in this case would lead to a size increase?
> >> >
> >> >If that's the case, then I'm fine if we go ahead with this patch,
> >> >I'd just like to request that Mike please file some of those BZs,
> >> >and please reference them from the commit message (as the longer
> >> >term solution), before committing the patch.
> >> >
> >> >Clang will warn if you have unused static variables when warnings
> >> >are
> >cranked
> >> >up.
> >> >
> >> >~/work/Compiler>cat static.c
> >> >static unsigned char gTest[] = { 42 };
> >> >
> >> >static int test ()
> >> >{
> >> > return 1;
> >> >}
> >> >
> >> >int main ()
> >> >{
> >> > return 0;
> >> >}
> >> >~/work/Compiler>clang -Os static.c -Wall
> >> >static.c:1:22: warning: unused variable 'gTest' [-Wunused-variable]
> >> >static unsigned char gTest[] = { 42 };
> >> >                    ^
> >> >static.c:3:12: warning: unused function 'test' [-Wunused-function]
> >> >static int test ()
> >> >          ^
> >> >2 warnings generated.
> >> >
> >> >Sorry, my question was imprecise.
> >> >
> >> >Assume there is a public library function ("external linkage") that
> >> >calls a static function in the same library instance and uses a
> >> >static variable in the same library instance. Then this library
> >> >instance is linked into a driver, but the driver never actually
> >> >calls the extern function -- so the static variable and the static
> >> >function too become useless.
> >> >
> >> >In this case, will /OPT:REF remove the static variable and the
> >> >static function too?
> >> >
> >> >It seems counter-intuitive to me that an internal-only function or
> >> >an internal-only variable has to be declared extern (via
> >> >GLOBAL_REMOVE_IF_UNREFERENCED) just so it can be eliminated at link
> >> >time, if it is never referenced (transitively).
> >> >
> >> >
> >> >Laszlo,
> >> >
> >> >I agree. The LLVM LTO does not have an issue "doing the right thing".
> >Seems
> >> >like static is also more of a compile time concept vs a link time
> >> >(global
> >> >optimization) kind of thing?
> >> >
> >> >Given on VC++ GLOBAL_REMOVE_IF_UNREFERENCED maps to __declspec(
> >> >selectany ) I would guess maybe it has more to due with supporting
> >> >old non standard header files that can't change without
> >breaking
> >> >compatibility.
> >> >
> >> >MSDN on __declspec( selectany ) :
> >> >A global data item can normally be initialized only once in an EXE
> >> >or DLL
> >> project.
> >> >selectany can be used in initializing global data defined by
> >> >headers, when
> >the
> >> >same header appears in more than one source file. selectany is
> >> >available in both the C and C++ compilers.
> >> >
> >> >Thanks,
> >> >
> >> >Andrew Fish
> >> >
> >> >
> >> >
> >> >Thanks
> >> >Laszlo
> >> >_______________________________________________
> >> >edk2-devel mailing list
> >> >edk2-devel@lists.01.org<mailto: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
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to
> American Megatrends, Inc.  This communication is intended to be read only by the
> individual or entity to whom it is addressed or by their designee. If the reader
> of this message is not the intended recipient, you are on notice that any
> distribution of this message, in any form, is strictly prohibited.  Please
> promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and
> then delete or destroy all copies of the transmission.

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

[Ard Biesheuvel]

On 26 May 2017 at 23:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Felix,
>
> Yes.  I agree.  I will work on a Bugzilla issue for this topic
> and I prefer the idea of updating Base.h to check _MSC_VER value.
>
> The one challenge is that 'static' could be added in front of
> GLOBAL_REMOVE_IF_UNREFERENCED, and it will build correctly with
> VS2012 and newer, but would fail with older VS versions that
> require __declspec(selectany) for size optimization.
>

As has been pointed out already in this thread (by Laszlo, I think),
STATIC may trigger 'unused symbol' warnings that break the build under
our warnings-as-errors policy, since the compiler can infer that no
external references exist. So while STATIC is absolutely the way
forward, #define'ing it in a way that affects existing code is likely
to cause problems.

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

[Laszlo Ersek]

On 05/27/17 14:27, Ard Biesheuvel wrote:
> On 26 May 2017 at 23:06, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
>> Felix,
>>
>> Yes.  I agree.  I will work on a Bugzilla issue for this topic
>> and I prefer the idea of updating Base.h to check _MSC_VER value.
>>
>> The one challenge is that 'static' could be added in front of
>> GLOBAL_REMOVE_IF_UNREFERENCED, and it will build correctly with
>> VS2012 and newer, but would fail with older VS versions that
>> require __declspec(selectany) for size optimization.
>>
> 
> As has been pointed out already in this thread (by Laszlo, I think),

(
It was Andrew's observation:

http://mid.mail-archive.com/56801ADE-446F-43C2-9C99-5500E8EE5881@apple.com
)

> STATIC may trigger 'unused symbol' warnings that break the build under
> our warnings-as-errors policy, since the compiler can infer that no
> external references exist. So while STATIC is absolutely the way
> forward, #define'ing it in a way that affects existing code is likely
> to cause problems.
> 

[Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate symbol

[Liming Gao]

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.

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

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

[Ni, Ruiyu]

On 12/7/2017 3:48 PM, Liming Gao 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.
> 
> 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 ();
> 

It's a good code practice to use library name as prefix for library
global variables.

-- 
Thanks,
Ray

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

[Ni, Ruiyu]

On 12/7/2017 3:48 PM, Liming Gao 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.
> 
> 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 ();
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray

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

[Wu, Hao A]

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Liming Gao
> Sent: Thursday, December 07, 2017 3:48 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D; Wu, Hao A; Laszlo Ersek; Andrew Fish; Jeff Fan
> Subject: [edk2] [Patch] SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix
> duplicate symbol
> 
> 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.
> 
> 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/SecPeiDebug
> AgentLib.c
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebug
> AgentLib.c
> index b717e33..9f5223a 100644
> ---
> a/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebug
> AgentLib.c
> +++
> b/SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgent/SecPeiDebug
> AgentLib.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

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

[Ard Biesheuvel]

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?


> 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

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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
>

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

[Gao, Liming]

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

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

[Ard Biesheuvel]

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

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

[Kinney, Michael D]

Ard,

If the variable was not prepended with GLOBAL_REMOVE_IF_UNREFERENCED, I would agree.

GLOBAL_REMOVE_IF_UNREFERENCED for some compilers is not compatible with static.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Ard Biesheuvel
> Sent: Thursday, December 7, 2017 12:47 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-
> devel@lists.01.org; Andrew Fish <afish@apple.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Jeff Fan <jeff.fan@intel.com>
> Subject: Re: [edk2] [Patch]
> SourceLevelDebugPkg/SecPeiDebugAgentLib: Fix duplicate
> symbol
> 
> 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?
> 
> 
> > 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/SecPeiDebugAgentL
> ib.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