.../Library/DebugAgent/SecPeiDebugAgent/SecPeiDebugAgentLib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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
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
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
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>>>>
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
> 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>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.