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