[Bug 970] #pragma visibility hidden in MdePkg/Include/X64/ProcessorBind.h is misconceived #pragma

bugzilla-daemon at bugzilla.tianocore.org...


--- Comment #3 from Ard Biesheuvel <ard.biesheuvel(a)linaro.org> ---
(In reply to zenith432 from comment #2)
... but here, I have to disagree. binutils only gained support for these
relocations in version 2.26 [0], and older versions of GCC do indeed emit a
different type of access depending on the visibility, even if this access lacks
any kind of ELF annotation (and I don't know why it should, but since you
brought it up ...)
You're conflating two separate things.
Earlier versions of GCC would generate GOT loads with R_X86_64_GOTPCREL
relocations for references to extern data, and that's what ld would handle
by transformating to lea instructions.
You are saying binutils before v2.26 will apply the R_X86_64_GOTPCRELX
transformation to R_X86_64_GOTPCREL relocations as well? If that is the case,
what is the difference between those relocations? What was the point of
introducing the new one that triggers the GNU ld optimization? Is it subject to
some ld command line option we may have been omitting?

Notice with H.J. Liu says...

I am proposing to add 2 new relocations, R_X86_64_GOTPCRELX and
R_X86_64_REX_GOTPCRELX, to x86-64 psABI. Instead of generating
R_X86_64_GOTPCREL relocation agasint foo for foo(a)GOTPCREL(%rip),
we generate R_X86_64_GOTPCRELX or R_X86_64_REX_GOTPCRELX if there
is a REX prefix. Linker can treat them the same as R_X86_64_GOTPCREL
or it can perform the transformations listed above.
What he suggested (and was indeed done) was that for x86_64 instructions
with a REX prefix, instead of generating GOT load with R_X86_64_GOTPCREL
relocation, GCC generates GOT load with R_X86_64_REX_GOTPCRELX relocation,
and ld was extended to recognize that. Except for the formal distinction
and the presence of the REX prefix byte, the transformation is exactly the
same. Check the opcodes in the Intel processor manual if you don't believe
me. The REX prefix is needed for many instructions with a 64-bit data
width. Prior to this change it was simply making the same transformation
ignoring the REX prefix byte.
This is not how I read that, I read it is (emphasis added by me)

we generate R_X86_64_GOTPCRELX *(* or R_X86_64_REX_GOTPCRELX if there
is a REX prefix *)*.
i.e., only R_X86_64_GOTPCRELX (and R_X86_64_REX_GOTPCRELX) are subject to the
transformation you describe (but only by GNU ld v2.26 or later), while
R_X86_64_GOTPCREL is not transformed by the linker at all.

MACHO faces the exact same issues and does not make a REX distriction to
this day. Here's recent defs used in kernel of macOS 10.13.3

What the documentation in MdePkg/Include/X64/ProcessorBind.h claims

// Mark all symbol declarations and references as hidden, meaning they will
// not be subject to symbol preemption. This allows the compiler to refer to
// symbols directly using relative references rather than via the GOT, which
// contains absolute symbol addresses that are subject to runtime relocation.

is something no version of GCC or clang has ever done. If you think I'm
wrong, please name a version of GCC that did this, I'll build it and check.
When I remove this piece of code, building OvmfX64.dsc using the GCC49 profile
(which disables LTO) results in

unsupported ELF EM_X86_64 relocation 0x9

caused by


3a8: ff 35 00 00 00 00 pushq 0x0(%rip) # 3ae
3aa: R_X86_64_GOTPCREL DhcpDummyExtFree-0x4

When I add the code back again, I get

3a1: 48 8d 05 00 00 00 00 lea 0x0(%rip),%rax # 3a8
3a4: R_X86_64_PC32 DhcpDummyExtFree-0x4

$ x86_64-linux-gnu-gcc -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../../gcc/configure --target=x86_64-linux-gnu
Thread model: posix
gcc version 8.0.1 20180122 (experimental) (GCC)

So in my view, this proves that
a) the visibility pragma does affect GCC's behavior when emitting external
b) the linker does *not* fix up R_X86_64_GOTPCREL relocations but only
R_X86_64_GOTPCRELX relocations.

But the reality is simply that removing this 'misconceived' code breaks the
build, so we cannot just remove it, we will have to work around the problem in
another way instead.

The SYSV x86_64 ABI spec, section 3.5.4 says "Position-independent code
cannot contain absolute address. To access a global symbol the address of
the symbol *has* to be loaded from the Global Offset Table. The address of
the entry in the GOT can be obtained with a %rip-relative instruction in the
small model."
While I can't find any formal definition what they mean by "global symbol",
section 3.2 states "The standard calling sequence requirements apply only to
global functions. Local functions that are not reachable from other
compilation units may use different conventions." So extrapolating what
they mean by "global function" to "global data" - any data not reachable
from other compilation units (= C source file in ISO C parlance) is local -
the rest is global.
So the SYSV ABI spec appears to explicitly forbid the kind of optimisation
that the documentation in MdePkg/Include/X64/ProcessorBind.h says the
compiler makes.

... which is precisely why we needed the 'hidden' visibility override, to
prevent [older versions of] GCC from emitting such relocations.
But GCC emits such relocations to this day! It's the linker that transforms
them. GenFw's success depends on the linker transforming them, not on GCC
abstaining from emitting them. It never abstains.

Maybe the problem was that updating to a recent version of GCC caused it to
emit R_X86_64_REX_GOTPCRELX, which an older verion of ld would then not
recognize, since it only recognized R_X86_64_GOTPCREL. But the solution to
that is to use a version of binutils compatible with the version of GCC
being used - as there can be many other such incompatibilities. For
example, there were version compatilibity problems with LTO plugin provided
by GCC and binutils that has to use it. At any rate, if this was added to
address an incompatibility between version of GCC and version of binutils
used - it should be phased out after this incompatibility gets old.
Well, we have a poor track record of phasing out support for older toolchains,
and BaseTools/Conf/tools_def.template is testament to that.

You are receiving this mail because:
You are on the CC list for the bug.