Re: [PATCH] BaseTools/GenFw: Add X64 GOTPCREL Support to GenFw

[Zenith432 <zenith432@users.sourceforge.net>]

This issue only applies to GCC/ELF target.

When using -fpie ("small pie" model which is GCC extension to SYSV ABI) - eliminates nearly all emission of GOTPCREL in object code (= loading addresses from the GOT.)

However, when optimizing for size (-Os), sometimes emitted code only needs to compute address of external symbol, not access data under it.  For example, when doing pointer arithmetic or passing address as an argument - the code generator concludes that loading the address from GOT is beneficial to reduced code size (since address can be loaded from GOT using push or add instructions, not just mov instruction.)  It then occasionally emits GOTPCREL.

The emission of GOTPCREL is completely suppressed by putting __attribute__((visibility("hidden")) on declaration of external symbol, or using

#pragma GCC push visibility("hidden")

to achieve this on all exernal symbols.

This is included in MdePkg/Include/X64/ProcessorBind.h - however only turned on for non-LTO builds.

When using "-flto -DUSING_LTO" - GCC can sometimes emit GOTPCREL for pointer arithmetic on external symbols.  I have instance of this which is not part of EDK2 (the GOTPCREL is emitted when "-flto -DUSING_LTO" is enabled.)

There is also an instance of emitted PCREL occuring when building OvmfX64.dsc if using "-fno-lto -DUSING_LTO"  (i.e. no LTO, but suppress the #pragma).

Adding GOTPCREL support to GenFw can handle a few occasional cases if this relocation is emitted by GCC.

I no longer ask that "#pragma GCC push visibility("hidden")" be removed for non-LTO builds, although it's a sweeping solution because if affects all symbols.  It is enough to put __attribute__((visibility("hidden")) on affected symbols.  Additionally, maybe scope be reduced with "#if defined(__GNUC__) && !defined(__clang__) && defined(__ELF__)" instead of "#if defined(__GNUC__)" so pragma affects only GCC/ELF.

I don't have any standalone samples that are not part of a large build.  If you want standalone test cases - I only have time next week to construct some.

As for the code itself

- I did test of course on the cases that occur in the build.

- Patch is constructed so that if no GOTPCREL are found in ELF binary - there's no effect on today's function of GenFw other then testing for NULL in a couple of places in function WriteLocation64.  In this sense the patch is safe - does not affect function of GenFw on previously suppoted ELF binaries that already works.  For ELF binaries with GOTPCREL today GenFw breaks with an error.

- ELF with multiple text or data sections is supported.  However, the order of events in the added code the patch makes to WriteSection64 depends on text sections being processed first and GOT being emitted in one of the text sections.  This is the case today (ld script emits GOT in .text).  I don't believe this is likely to change, however, if it's important to elminate this minor order dependence - I can do so.

- I hope someone reviews the code carefully :)

- I see other patches to Elf64Convert.c were posted so this patch probably needs merging before commit.

Regards

--------------------------------------------
On Thu, 6/7/18, Gao, Liming <liming.gao@intel.com> wrote:

 Subject: RE: [PATCH] BaseTools/GenFw: Add X64 GOTPCREL Support to GenFw
 To: "Zenith432" <zenith432@users.sourceforge.net>, "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
 Cc: "Shi, Steven" <steven.shi@intel.com>, "Zhu, Yonghong" <yonghong.zhu@intel.com>
 Date: Thursday, June 7, 2018, 4:32 AM
 
 What's purpose to support GOTPCREL in GenFw?
 Could you introduce your usage model?