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

["Shi, Steven" <steven.shi@intel.com>]

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.






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)





Steven Shi

Intel\SSG\STO\UEFI Firmware



Tel: +86 021-61166522

iNet: 821-6522



> -----Original Message-----

> From: Laszlo Ersek [mailto:lersek@redhat.com]

> Sent: Tuesday, August 15, 2017 11:46 PM

> To: Shi, Steven <steven.shi@intel.com>

> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Alex Williamson

> <alex.williamson@redhat.com>; Ard Biesheuvel

> <ard.biesheuvel@linaro.org>; Justen, Jordan L <jordan.l.justen@intel.com>;

> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>

> Subject: Re: [edk2] [PATCH 1/1] BaseTools/tools_def.template: revert to

> large code model for X64/GCC5/LTO

>

> On 08/12/17 05:05, Shi, Steven wrote:

> > OK. I can reproduce the failure with -smp 4 and -m 5120 in my side.

> >

> > It looks a linker bug about assemble function support in PIC/PIE

> > code.  You know, if we only have C code, the compiler/linker will

> > generate all the machine code and guarantee all the address reference

> > are position independent under PIC/PIE build. But if we mix manually

> > written assemble code in the C code,  the linker cannot really control

> > the address reference in the assemble code, and  might got confused.

>

> This is an incorrect description of the situation.

>

> The address reference is *not* in assembly code. (It used to be in

> assembly code, but Mike changed that earlier, for XCODE5 compatibility.)

>

> At this moment the address reference is in *C code*. The C code takes

> the address of an external function, and assigns it to a field in a data

> structure. The assembly code calls the function through this field. The

> assembly code makes no reference to the called function by name. See

> Mike's commit 3b2928b46987:

>

> -    mov        rax, ASM_PFX(InitializeFloatingPointUnits)

> +    mov        rax, qword [esi + InitializeFloatingPointUnitsAddress]

>      sub        rsp, 20h

>      call       rax               ; Call assembly function to initialize FPU per UEFI spec

>

> And, indeed, it is *not* the assembly code that's being miscompiled. It

> is the C-language assignment below that is miscompiled:

>

> +  ExchangeInfo->InitializeFloatingPointUnitsAddress =

> (UINTN)InitializeFloatingPointUnits;

>

> > So, it is not seldom we could see the compiler/linker generate wrong

> > code for mixed code,  especially with very high level optimization,

> > e.g. LTO.

> >

> > Globally change memory model from small to large will bring not

> > trivial impact (+15%) to code size, espcial for the uncomperssed

> > option rom dirver. Below is some data of OvmfPkgX64.dsc platform.

> >

> >               Dxecore.efi     CpuDxe.efi      CpuMpPei        PeiCore.efi

> > Small+PIE:    139520          47360           30144           46720

> > Large:        165696          55360           34496           53504

>

> My argument is that the current "-mcmodel=small" option actually *lies*

> to the compiler about our binaries. According to the GCC documentation,

> "-mcmodel=small" implies that a binary built like this will never be

> executed from above 2GB in the address space. This is why gcc-7 believes

> it is allowed to generate a MOV instruction that sign-extends a 32-bit

> address to 64-bit -- because we promise GCC that the sign bit will

> always be clear to begin with.

>

> IOW, we make a promise, gcc-7 generates code accordingly, and then we

> break the promise, by executing the binary (the assignment in the C code

> of MpInitLib) from above 2GB.

>

>

> In particular, an X64 UEFI_DRIVER module, shipped as an option ROM on a

> physical PCI(E) card, could be loaded anywhere at all in the 64-bit

> address space (given sufficient memory in the computer). Building such a

> driver with "-mcmodel=small" is wrong therefore; we cannot guarantee

> that the driver will be executed from under 2GB.

>

> Perhaps we should use "-mcmodel=large", but *keep* "-fpie". ... I've now

> tried that, but it doesn't work. With "-mcmodel=large -fpie", the

> compiler emits R_X86_64_GOTOFF64 relocations (type 25 decimal), and I

> get errors like:

>

> GenFw: ERROR 3000: Invalid

>

> Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/ReportStatus

> CodeRouter/Pei/ReportStatusCodeRouterPei/DEBUG/ReportStatusCodeRou

> terPei.dll

>   unsupported ELF EM_X86_64 relocation 0x19.

>

> I believe Ard's commit 28ade7b802e0 ("MdePkg: move to 'hidden'

> visibility for all symbols under GCC/X64", 2016-08-01) was meant to

> prevent this, but apparently it's not enough with gcc-7.1.

>

> > A simpler workaround could be to add a C function wrapper around the

> > assemble lib function as below. This simple workaround works in my

> > side.  But it is necessary to find this issue's root cause and fix it

> > in the compiler/linker. I will try to raise this issue to

> > compiler/linker guys.

> >

> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c

> b/UefiCpuPkg/Library/MpInitLib/MpLib.c

> > old mode 100644

> > new mode 100755

> > index a3eea29..7afe434

> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c

> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c

> > @@ -738,6 +738,15 @@ WaitApWakeup (

> >    }

> >  }

> >

> > +VOID

> > +EFIAPI

> > +InitializeFloatingPointUnitsWrapper (

> > +  VOID

> > +  )

> > +{

> > +  InitializeFloatingPointUnits();

> > +}

> > +

> >  /**

> >    This function will fill the exchange info structure.

> >

> > @@ -771,7 +780,7 @@ FillExchangeInfoData (

> >

> >    ExchangeInfo->EnableExecuteDisable = IsBspExecuteDisableEnabled ();

> >

> > -  ExchangeInfo->InitializeFloatingPointUnitsAddress =

> (UINTN)InitializeFloatingPointUnits;

> > +  ExchangeInfo->InitializeFloatingPointUnitsAddress =

> (UINTN)InitializeFloatingPointUnitsWrapper;

> >

> >    //

> >    // Get the BSP's data of GDT and IDT

>

> I'm not convinced that this is the right fix, until we know exactly why

> and how it changes the behavior of gcc-7. So I've now filed

> <https://bugzilla.tianocore.org/show_bug.cgi?id=671> to track this

> problem. (I also captured your suggestion in the BZ.)

>

> We do know the exact symptoms and consequences of the miscompilation,

> and I want to suppress those symptoms at least, as soon as possible. I

> will post a patch for OvmfPkg's "build.sh" to use the GCC49 toolchain

> settings with gcc-7.* (no LTO). I'll also ask Gerd to update the

> toolchain selection in his SPEC file accordingly.

>

> Thanks

> Laszlo