* [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
@ 2017-08-24 6:28 Liming Gao
2017-08-25 10:28 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Liming Gao @ 2017-08-24 6:28 UTC (permalink / raw)
To: edk2-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-24 6:28 [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option Liming Gao
@ 2017-08-25 10:28 ` Laszlo Ersek
2017-08-25 10:30 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2017-08-25 10:28 UTC (permalink / raw)
To: Liming Gao; +Cc: edk2-devel, Ard Biesheuvel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-25 10:28 ` Laszlo Ersek
@ 2017-08-25 10:30 ` Ard Biesheuvel
2017-08-25 11:48 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2017-08-25 10:30 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Liming Gao, edk2-devel@lists.01.org
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-25 10:30 ` Ard Biesheuvel
@ 2017-08-25 11:48 ` Laszlo Ersek
2017-08-28 3:27 ` Gao, Liming
0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2017-08-25 11:48 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Liming Gao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-25 11:48 ` Laszlo Ersek
@ 2017-08-28 3:27 ` Gao, Liming
2017-08-28 16:59 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Gao, Liming @ 2017-08-28 3:27 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-28 3:27 ` Gao, Liming
@ 2017-08-28 16:59 ` Ard Biesheuvel
2017-08-29 7:41 ` Gao, Liming
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2017-08-28 16:59 UTC (permalink / raw)
To: Gao, Liming; +Cc: Laszlo Ersek, edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-28 16:59 ` Ard Biesheuvel
@ 2017-08-29 7:41 ` Gao, Liming
2017-08-29 13:33 ` Ard Biesheuvel
2017-08-29 15:47 ` Kinney, Michael D
0 siblings, 2 replies; 10+ messages in thread
From: Gao, Liming @ 2017-08-29 7:41 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Laszlo Ersek, edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-29 7:41 ` Gao, Liming
@ 2017-08-29 13:33 ` Ard Biesheuvel
2017-08-29 15:47 ` Kinney, Michael D
1 sibling, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2017-08-29 13:33 UTC (permalink / raw)
To: Gao, Liming; +Cc: Laszlo Ersek, edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-29 7:41 ` Gao, Liming
2017-08-29 13:33 ` Ard Biesheuvel
@ 2017-08-29 15:47 ` Kinney, Michael D
2017-08-29 16:37 ` Ard Biesheuvel
1 sibling, 1 reply; 10+ messages in thread
From: Kinney, Michael D @ 2017-08-29 15:47 UTC (permalink / raw)
To: Gao, Liming, Ard Biesheuvel, Kinney, Michael D
Cc: edk2-devel@lists.01.org, Laszlo Ersek
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option
2017-08-29 15:47 ` Kinney, Michael D
@ 2017-08-29 16:37 ` Ard Biesheuvel
0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2017-08-29 16:37 UTC (permalink / raw)
To: Kinney, Michael D; +Cc: Gao, Liming, edk2-devel@lists.01.org, Laszlo Ersek
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-08-29 16:34 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-24 6:28 [Patch] BaseTools: Enable --whole-archive in GCC tool chain as the default option Liming Gao
2017-08-25 10:28 ` Laszlo Ersek
2017-08-25 10:30 ` Ard Biesheuvel
2017-08-25 11:48 ` Laszlo Ersek
2017-08-28 3:27 ` Gao, Liming
2017-08-28 16:59 ` Ard Biesheuvel
2017-08-29 7:41 ` Gao, Liming
2017-08-29 13:33 ` Ard Biesheuvel
2017-08-29 15:47 ` Kinney, Michael D
2017-08-29 16:37 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox