Re: [PATCH 1/1] BaseTools/tools_def.template: revert to large code model for X64/GCC5/LTO

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  I will take BZ#671 and create the formal patch for review. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, August 22, 2017 7:59 PM
> To: Shi, Steven <steven.shi@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Alex Williamson <alex.williamson@redhat.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [edk2] [PATCH 1/1] BaseTools/tools_def.template: revert to large code model for X64/GCC5/LTO
> 
> On 08/22/17 10:00, Shi, Steven wrote:
> > It is a link flag misuse in our GCC build toolchains, not
> > compiler/linker's problem. Below patch can fix the wrong assembly
> > function relocation type in PIE binary. With below patch, all the
> > GCC5, GCC49 and GCC48 can build correctly images of OvmfPkgIa32X64 and
> > OvmfPkgX64 platforms without my previous simple work around in my
> > side. Please try it in your side.
> >
> > Since we are using GCC as linker command, we MUST pass -pie to ld with
> > "-Wl,-pie", not just "--pie"  or "-fpie".
> >
> > So, this means we never correctly build small+PIE 64bits code with GCC
> > toolchains before (CLANG38 is correct). If you failed to enable
> > PIE/LTO build before, it is worthy to revisit those failures with
> > "-Wl,-pie". FYI.
> 
> The first question of Paolo (CC'd) was, when he saw this issue in my
> last status report, whether we added "-fpie" to the link command line as
> well. And, I confirmed, we did. This is how I responded to him:
> 
> > - in "BaseTools/Conf/build_rule.template", there's
> >
> > > [Static-Library-File]
> > >     <InputFile>
> > >         *.lib
> > >
> > >     <ExtraDependency>
> > >         $(MAKE_FILE)
> > >
> > >     <OutputFile>
> > >         $(DEBUG_DIR)(+)$(MODULE_NAME).dll
> > >
> > >     [...]
> > >
> > >     <Command.GCC>
> > >         "$(DLINK)" -o ${dst} $(DLINK_FLAGS) -Wl,--start-group,@$(STATIC_LIBRARY_FILES_LIST),--end-group $(CC_FLAGS)
> $(DLINK2_FLAGS)
> > >         "$(OBJCOPY)" $(OBJCOPY_FLAGS) ${dst}
> >
> > (see "$(CC_FLAGS)")
> >
> > - and in the build log, I see
> >
> > > "gcc" \
> > >   -o Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/DEBUG/CpuMpPei.dll \
> > >   -nostdlib \
> > >   -Wl,-n,-q,--gc-sections \
> > >   -z common-page-size=0x40 \
> > >   -Wl,--entry,_ModuleEntryPoint \
> > >   -u _ModuleEntryPoint \
> > >   -Wl,-Map,Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/DEBUG/CpuMpPei.map \
> > >   -Wl,-melf_x86_64,--oformat=elf64-x86-64 \
> > >   -flto \
> > >   -Os \
> > >
> -Wl,--start-group,@Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/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=CpuMpPeiStrings \
> > >   -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 \
> > >   -flto \
> > >   -DUSING_LTO \
> > >   -Os \
> > >   -mno-mmx \
> > >   -mno-sse \
> > >   -D DISABLE_NEW_DEPRECATED_INTERFACES \
> > >   -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \
> > >   -Wl,--script=BaseTools/Scripts/GccBase.lds \
> > >   -Wno-error
> 
> I don't understand why we need "-Wl,-pie" separately. The "gcc" binary
> should pass it through to "ld" as necessary, IMO. This is what the gcc
> documentation says:
> 
> > 3.13 Options for Linking
> > ========================
> > '-pie'
> >      Produce a position independent executable on targets that support
> >      it.  For predictable results, you must also specify the same set
> >      of options used for compilation ('-fpie', '-fPIE', or model
> >      suboptions) when you specify this linker option.
> >
> > 3.18 Options for Code Generation Conventions
> > ============================================
> > '-fpie'
> > '-fPIE'
> >      These options are similar to '-fpic' and '-fPIC', but generated
> >      position independent code can be only linked into executables.
> >      Usually these options are used when '-pie' GCC option is used
> >      during linking.
> >
> >      '-fpie' and '-fPIE' both define the macros '__pie__' and
> >      '__PIE__'. The macros have the value 1 for '-fpie' and 2 for
> >      '-fPIE'.
> 
> This seems to suggest that "-pie" is the *master* switch (used only when
> linking), and "-fpie" is a *prerequisite* for it (to be used both when
> linking and compiling). Is this right?
> 
> If so, then I think this is a gcc usability bug. We don't generally
> start our thinking from the linker side. The above implies that the
> simple (hosted) command line:
> 
> $ gcc -o example -fpie source1.c source2.c
> 
> could also result in miscompilation, because "-pie" is not given, only
> "-fpie".
> 
> On 08/22/17 10:00, Shi, Steven wrote:
> 
> > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > index 1fa3ca3..9e46d65 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -4375,7 +4375,7 @@ DEFINE GCC44_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z comm
> > 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_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_DLINK_FLAGS         = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
> -Wl,-pie
> > DEFINE GCC44_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON)
> > DEFINE GCC44_ASM_FLAGS               = DEF(GCC_ASM_FLAGS)
> >
> > @@ -4455,7 +4455,7 @@ DEFINE GCC49_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z comm
> > 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_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_DLINK_FLAGS         = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64
> -Wl,-pie
> > DEFINE GCC49_X64_DLINK2_FLAGS        = DEF(GCC48_X64_DLINK2_FLAGS)
> > DEFINE GCC49_ASM_FLAGS               = DEF(GCC48_ASM_FLAGS)
> > DEFINE GCC49_ARM_ASM_FLAGS           = DEF(GCC48_ARM_ASM_FLAGS)
> 
> Do we think that this was an omission in commit a1b8baccc30b ("BaseTools
> GCC: use 'gcc' as the linker command for GCC44 and later", 2016-07-23)?
> 
> Honestly, I'm quite a bit annoyed by this parameter forwarding mess
> between "gcc" and "ld". If someone can submit a formal patch to fix
> this, please do so (I also suggest to reassign BZ#671 to BaseTools
> then). If there's an actual patch I can fetch or apply with git, I'll be
> happy to test it.
> 
> Thanks,
> Laszlo