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

[Laszlo Ersek <lersek@redhat.com>]

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