1. Patchew
  2. EDK2
  3. View series

[edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option

Liming Gao posted 1 patch 7 years, 4 months ago
Failed in applying to current master (apply log)
BaseTools/Conf/tools_def.template | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Liming Gao 7 years, 4 months ago 
https://bugzilla.tianocore.org/show_bug.cgi?id=581

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <liming.gao@intel.com>
Cc: Yonghong Zhu <yonghong.zhu@intel.com>
---
 BaseTools/Conf/tools_def.template | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 24956e4..a85afd5 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -4377,7 +4377,7 @@ DEFINE GCC44_IA32_CC_FLAGS           = DEF(GCC44_ALL_CC_FLAGS) -m32 -march=i586
 DEFINE GCC44_X64_CC_FLAGS            = DEF(GCC44_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables
 DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
 DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
-DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
+DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
 DEFINE GCC44_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
 DEFINE GCC44_X64_DLINK_FLAGS         = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
 DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
@@ -4457,7 +4457,7 @@ DEFINE GCC49_IA32_CC_FLAGS           = DEF(GCC48_IA32_CC_FLAGS)
 DEFINE GCC49_X64_CC_FLAGS            = DEF(GCC48_X64_CC_FLAGS)
 DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x40
 DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
-DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
+DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
 DEFINE GCC49_IA32_DLINK2_FLAGS       = DEF(GCC48_IA32_DLINK2_FLAGS)
 DEFINE GCC49_X64_DLINK_FLAGS         = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
 DEFINE GCC49_X64_DLINK2_FLAGS        = DEF(GCC48_X64_DLINK2_FLAGS)
-- 
2.8.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Laszlo Ersek 7 years, 4 months ago 
On 08/24/17 08:28, Liming Gao wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=581

(1) I suggest adding one sentence (before you push the patch):

"The --whole-archive linker option helps us catch multiply defined symbols."

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> ---
>  BaseTools/Conf/tools_def.template | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index 24956e4..a85afd5 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4377,7 +4377,7 @@ DEFINE GCC44_IA32_CC_FLAGS           = DEF(GCC44_ALL_CC_FLAGS) -m32 -march=i586
>  DEFINE GCC44_X64_CC_FLAGS            = DEF(GCC44_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables
>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
> -DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
> +DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>  DEFINE GCC44_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
>  DEFINE GCC44_X64_DLINK_FLAGS         = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>  DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
> @@ -4457,7 +4457,7 @@ DEFINE GCC49_IA32_CC_FLAGS           = DEF(GCC48_IA32_CC_FLAGS)
>  DEFINE GCC49_X64_CC_FLAGS            = DEF(GCC48_X64_CC_FLAGS)
>  DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x40
>  DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
> -DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
> +DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>  DEFINE GCC49_IA32_DLINK2_FLAGS       = DEF(GCC48_IA32_DLINK2_FLAGS)
>  DEFINE GCC49_X64_DLINK_FLAGS         = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>  DEFINE GCC49_X64_DLINK2_FLAGS        = DEF(GCC48_X64_DLINK2_FLAGS)
> 

This patch doesn't apply directly on current master, due to context
differences introduced by 2f7f1e73c10f ("BaseTools: Add the missing -pie
link option in GCC tool chain", 2017-08-23).

However, it does apply to the parent of 2f7f1e73c10f, and from there it
can be rebased without manual conflict resolution.

I did just that, and tested the patch with GCC48, IA32, IA32X64, and X64
OVMF. It works for me:

Tested-by: Laszlo Ersek <lersek@redhat.com>

(2) If Ard agrees, can you please post a similar patch for ARM/AARCH64,
GCC? (I think that should be a separate patch, so that I don't have to
re-test the IA32/X64 change.)

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Ard Biesheuvel 7 years, 4 months ago 
On 25 August 2017 at 11:28, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/24/17 08:28, Liming Gao wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=581
>
> (1) I suggest adding one sentence (before you push the patch):
>
> "The --whole-archive linker option helps us catch multiply defined symbols."
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>> ---
>>  BaseTools/Conf/tools_def.template | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>> index 24956e4..a85afd5 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -4377,7 +4377,7 @@ DEFINE GCC44_IA32_CC_FLAGS           = DEF(GCC44_ALL_CC_FLAGS) -m32 -march=i586
>>  DEFINE GCC44_X64_CC_FLAGS            = DEF(GCC44_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables
>>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
>>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
>> -DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>> +DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>  DEFINE GCC44_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
>>  DEFINE GCC44_X64_DLINK_FLAGS         = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>>  DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
>> @@ -4457,7 +4457,7 @@ DEFINE GCC49_IA32_CC_FLAGS           = DEF(GCC48_IA32_CC_FLAGS)
>>  DEFINE GCC49_X64_CC_FLAGS            = DEF(GCC48_X64_CC_FLAGS)
>>  DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x40
>>  DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
>> -DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>> +DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>  DEFINE GCC49_IA32_DLINK2_FLAGS       = DEF(GCC48_IA32_DLINK2_FLAGS)
>>  DEFINE GCC49_X64_DLINK_FLAGS         = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>>  DEFINE GCC49_X64_DLINK2_FLAGS        = DEF(GCC48_X64_DLINK2_FLAGS)
>>
>
> This patch doesn't apply directly on current master, due to context
> differences introduced by 2f7f1e73c10f ("BaseTools: Add the missing -pie
> link option in GCC tool chain", 2017-08-23).
>
> However, it does apply to the parent of 2f7f1e73c10f, and from there it
> can be rebased without manual conflict resolution.
>
> I did just that, and tested the patch with GCC48, IA32, IA32X64, and X64
> OVMF. It works for me:
>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
>
> (2) If Ard agrees, can you please post a similar patch for ARM/AARCH64,
> GCC? (I think that should be a separate patch, so that I don't have to
> re-test the IA32/X64 change.)
>

Didn't we decide this should be DEBUG/NOOPT only, due to code size increase?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Laszlo Ersek 7 years, 4 months ago 
On 08/25/17 12:30, Ard Biesheuvel wrote:
> On 25 August 2017 at 11:28, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 08/24/17 08:28, Liming Gao wrote:
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=581
>>
>> (1) I suggest adding one sentence (before you push the patch):
>>
>> "The --whole-archive linker option helps us catch multiply defined symbols."
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>>> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>>> ---
>>>  BaseTools/Conf/tools_def.template | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>>> index 24956e4..a85afd5 100755
>>> --- a/BaseTools/Conf/tools_def.template
>>> +++ b/BaseTools/Conf/tools_def.template
>>> @@ -4377,7 +4377,7 @@ DEFINE GCC44_IA32_CC_FLAGS           = DEF(GCC44_ALL_CC_FLAGS) -m32 -march=i586
>>>  DEFINE GCC44_X64_CC_FLAGS            = DEF(GCC44_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables
>>>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
>>>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
>>> -DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>> +DEFINE GCC44_IA32_X64_DLINK_FLAGS    = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>>  DEFINE GCC44_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
>>>  DEFINE GCC44_X64_DLINK_FLAGS         = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>>>  DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
>>> @@ -4457,7 +4457,7 @@ DEFINE GCC49_IA32_CC_FLAGS           = DEF(GCC48_IA32_CC_FLAGS)
>>>  DEFINE GCC49_X64_CC_FLAGS            = DEF(GCC48_X64_CC_FLAGS)
>>>  DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x40
>>>  DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
>>> -DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>> +DEFINE GCC49_IA32_X64_DLINK_FLAGS    = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>>  DEFINE GCC49_IA32_DLINK2_FLAGS       = DEF(GCC48_IA32_DLINK2_FLAGS)
>>>  DEFINE GCC49_X64_DLINK_FLAGS         = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
>>>  DEFINE GCC49_X64_DLINK2_FLAGS        = DEF(GCC48_X64_DLINK2_FLAGS)
>>>
>>
>> This patch doesn't apply directly on current master, due to context
>> differences introduced by 2f7f1e73c10f ("BaseTools: Add the missing -pie
>> link option in GCC tool chain", 2017-08-23).
>>
>> However, it does apply to the parent of 2f7f1e73c10f, and from there it
>> can be rebased without manual conflict resolution.
>>
>> I did just that, and tested the patch with GCC48, IA32, IA32X64, and X64
>> OVMF. It works for me:
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> (2) If Ard agrees, can you please post a similar patch for ARM/AARCH64,
>> GCC? (I think that should be a separate patch, so that I don't have to
>> re-test the IA32/X64 change.)
>>
> 
> Didn't we decide this should be DEBUG/NOOPT only, due to code size increase?

I didn't remember off-hand, but I found this in my mailbox:

http://mid.mail-archive.com/df25cbe5-a6a0-79d5-ccd7-f0ad53c2ed03@redhat.com

On 05/26/17 11:05, Laszlo Ersek wrote:
> - 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.

Up-thread, Mike wrote "Total used difference = 1816 bytes larger with
-whole-archive".

Nonetheless, if we want to be super cautious, I don't mind if RELEASE
doesn't get --whole-archive.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Gao, Liming 7 years, 3 months ago 
Laszlo:
  I will update the patch with your comments. 

Ard: 
  We collect the size impact in Ovmf platform. Its impact is small. So, my patch enable this option for all targets. Below is the data collected on OvmfIa32X64.dsc with GCC5 tool chain. Raw image is a little bigger. But, the compressed size is a little smaller. 

PEIFV   178472  --> 179176   +704   (Bytes)
DXEFV  4062512 --> 4075056  +12544 (Bytes)
FVMAIN_COMPACT 1190896 --> 1184920  -5976  (Bytes)

Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Friday, August 25, 2017 7:48 PM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
>Subject: Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool
>chain as the default option
>
>On 08/25/17 12:30, Ard Biesheuvel wrote:
>> On 25 August 2017 at 11:28, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 08/24/17 08:28, Liming Gao wrote:
>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=581
>>>
>>> (1) I suggest adding one sentence (before you push the patch):
>>>
>>> "The --whole-archive linker option helps us catch multiply defined
>symbols."
>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>>>> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>>>> ---
>>>>  BaseTools/Conf/tools_def.template | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/BaseTools/Conf/tools_def.template
>b/BaseTools/Conf/tools_def.template
>>>> index 24956e4..a85afd5 100755
>>>> --- a/BaseTools/Conf/tools_def.template
>>>> +++ b/BaseTools/Conf/tools_def.template
>>>> @@ -4377,7 +4377,7 @@ DEFINE GCC44_IA32_CC_FLAGS           =
>DEF(GCC44_ALL_CC_FLAGS) -m32 -march=i586
>>>>  DEFINE GCC44_X64_CC_FLAGS            = DEF(GCC44_ALL_CC_FLAGS) -m64
>-fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-
>outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-
>asynchronous-unwind-tables
>>>>  DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-
>sections -z common-page-size=0x20
>>>>  DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS =
>DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u
>ReferenceAcpiTable
>>>> -DEFINE GCC44_IA32_X64_DLINK_FLAGS    =
>DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--
>entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
>Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>>> +DEFINE GCC44_IA32_X64_DLINK_FLAGS    =
>DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--
>entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
>Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>>>  DEFINE GCC44_IA32_DLINK2_FLAGS       = -Wl,--
>defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
>>>>  DEFINE GCC44_X64_DLINK_FLAGS         =
>DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-
>64
>>>>  DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--
>defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
>>>> @@ -4457,7 +4457,7 @@ DEFINE GCC49_IA32_CC_FLAGS           =
>DEF(GCC48_IA32_CC_FLAGS)
>>>>  DEFINE GCC49_X64_CC_FLAGS            = DEF(GCC48_X64_CC_FLAGS)
>>>>  DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-
>sections -z common-page-size=0x40
>>>>  DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS =
>DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u
>ReferenceAcpiTable
>>>> -DEFINE GCC49_IA32_X64_DLINK_FLAGS    =
>DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--
>entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
>Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map
>>>> +DEFINE GCC49_IA32_X64_DLINK_FLAGS    =
>DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--
>entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-
>Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
>>>>  DEFINE GCC49_IA32_DLINK2_FLAGS       =
>DEF(GCC48_IA32_DLINK2_FLAGS)
>>>>  DEFINE GCC49_X64_DLINK_FLAGS         =
>DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-
>64
>>>>  DEFINE GCC49_X64_DLINK2_FLAGS        =
>DEF(GCC48_X64_DLINK2_FLAGS)
>>>>
>>>
>>> This patch doesn't apply directly on current master, due to context
>>> differences introduced by 2f7f1e73c10f ("BaseTools: Add the missing -pie
>>> link option in GCC tool chain", 2017-08-23).
>>>
>>> However, it does apply to the parent of 2f7f1e73c10f, and from there it
>>> can be rebased without manual conflict resolution.
>>>
>>> I did just that, and tested the patch with GCC48, IA32, IA32X64, and X64
>>> OVMF. It works for me:
>>>
>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> (2) If Ard agrees, can you please post a similar patch for ARM/AARCH64,
>>> GCC? (I think that should be a separate patch, so that I don't have to
>>> re-test the IA32/X64 change.)
>>>
>>
>> Didn't we decide this should be DEBUG/NOOPT only, due to code size
>increase?
>
>I didn't remember off-hand, but I found this in my mailbox:
>
>http://mid.mail-archive.com/df25cbe5-a6a0-79d5-ccd7-
>f0ad53c2ed03@redhat.com
>
>On 05/26/17 11:05, Laszlo Ersek wrote:
>> - 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.
>
>Up-thread, Mike wrote "Total used difference = 1816 bytes larger with
>-whole-archive".
>
>Nonetheless, if we want to be super cautious, I don't mind if RELEASE
>doesn't get --whole-archive.
>
>Thanks,
>Laszlo
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Ard Biesheuvel 7 years, 3 months ago 
On 28 August 2017 at 04:27, Gao, Liming <liming.gao@intel.com> wrote:
> Laszlo:
>   I will update the patch with your comments.
>
> Ard:
>   We collect the size impact in Ovmf platform. Its impact is small. So, my patch enable this option for all targets. Below is the data collected on OvmfIa32X64.dsc with GCC5 tool chain. Raw image is a little bigger. But, the compressed size is a little smaller.
>
> PEIFV   178472  --> 179176   +704   (Bytes)
> DXEFV  4062512 --> 4075056  +12544 (Bytes)
> FVMAIN_COMPACT 1190896 --> 1184920  -5976  (Bytes)
>

I don't care deeply, but given that --whole-archive is used as a debug
feature (we don't actually need the whole archive, but we want to
force a linker error if duplicate symbols exist), I don't think it
belongs in the RELEASE configuration.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Gao, Liming 7 years, 3 months ago 
Ard:
  This is the compiler check option, not debug option. I suggest to add it for all configuration.

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Tuesday, August 29, 2017 12:59 AM
>To: Gao, Liming <liming.gao@intel.com>
>Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
>Subject: Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool
>chain as the default option
>
>On 28 August 2017 at 04:27, Gao, Liming <liming.gao@intel.com> wrote:
>> Laszlo:
>>   I will update the patch with your comments.
>>
>> Ard:
>>   We collect the size impact in Ovmf platform. Its impact is small. So, my
>patch enable this option for all targets. Below is the data collected on
>OvmfIa32X64.dsc with GCC5 tool chain. Raw image is a little bigger. But, the
>compressed size is a little smaller.
>>
>> PEIFV   178472  --> 179176   +704   (Bytes)
>> DXEFV  4062512 --> 4075056  +12544 (Bytes)
>> FVMAIN_COMPACT 1190896 --> 1184920  -5976  (Bytes)
>>
>
>I don't care deeply, but given that --whole-archive is used as a debug
>feature (we don't actually need the whole archive, but we want to
>force a linker error if duplicate symbols exist), I don't think it
>belongs in the RELEASE configuration.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Ard Biesheuvel 7 years, 3 months ago 
On 29 August 2017 at 08:41, Gao, Liming <liming.gao@intel.com> wrote:
> Ard:
>   This is the compiler check option, not debug option. I suggest to add it for all configuration.
>

OK, fair enough.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Kinney, Michael D 7 years, 3 months ago 
Ard,

If there is a concern about the size impact, we can add an extra
Link step that uses --whole-archive to check for duplicate symbols,
but the link step used to generate final image would not use
--whole-archive.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Gao, Liming
> Sent: Tuesday, August 29, 2017 12:41 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [Patch] BaseTools: Enable --whole-archive
> in GCC tool chain as the default option
> 
> Ard:
>   This is the compiler check option, not debug option. I
> suggest to add it for all configuration.
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >Sent: Tuesday, August 29, 2017 12:59 AM
> >To: Gao, Liming <liming.gao@intel.com>
> >Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> >Subject: Re: [edk2] [Patch] BaseTools: Enable --whole-archive
> in GCC tool
> >chain as the default option
> >
> >On 28 August 2017 at 04:27, Gao, Liming <liming.gao@intel.com>
> wrote:
> >> Laszlo:
> >>   I will update the patch with your comments.
> >>
> >> Ard:
> >>   We collect the size impact in Ovmf platform. Its impact is
> small. So, my
> >patch enable this option for all targets. Below is the data
> collected on
> >OvmfIa32X64.dsc with GCC5 tool chain. Raw image is a little
> bigger. But, the
> >compressed size is a little smaller.
> >>
> >> PEIFV   178472  --> 179176   +704   (Bytes)
> >> DXEFV  4062512 --> 4075056  +12544 (Bytes)
> >> FVMAIN_COMPACT 1190896 --> 1184920  -5976  (Bytes)
> >>
> >
> >I don't care deeply, but given that --whole-archive is used as
> a debug
> >feature (we don't actually need the whole archive, but we want
> to
> >force a linker error if duplicate symbols exist), I don't
> think it
> >belongs in the RELEASE configuration.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
Posted by Ard Biesheuvel 7 years, 3 months ago 
On 29 August 2017 at 16:47, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
> Ard,
>
> If there is a concern about the size impact, we can add an extra
> Link step that uses --whole-archive to check for duplicate symbols,
> but the link step used to generate final image would not use
> --whole-archive.
>

If the size delta is in the noise, it's fine with me. I just triggered
on the fact that this now applies to all builds rather than
DEBUG/NOOPT ones only.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.