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

[Laszlo Ersek <lersek@redhat.com>]

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