[X86] Don't emit R_X86_64_[REX_]GOTPCRELX for a GOT load with an offset
ClosedPublic
Actions

Authored by MaskRay on Nov 25 2020, 10:13 AM.

Details

Reviewers

craig.topper
grimar
jhenderson
sberg
• espindola

Commits

rG25c8fbb3d92f: [X86] Don't emit R_X86_64_[REX_]GOTPCRELX for a GOT load with an offset

Summary

clang may produce movl x@GOTPCREL+4(%rip), %eax when loading the high
32 bits of the address of a global variable in -fpic/-fpie mode.

If assembled by GNU as, the fixup emits R_X86_64_GOTPCRELX
with an addend != -4. The instruction loads from the GOT entry with an offset
and thus it is incorrect to relax the instruction.

This patch does not emit a relaxable relocation for a GOT load with an offset.
The result is good enough for LLD to work. GNU ld relaxes mov+GOTPCREL
as well, but it suppresses the relaxation if addend != -4.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

MaskRay created this revision.Nov 25 2020, 10:13 AM

Herald added a reviewer: • espindola. · View Herald TranscriptNov 25 2020, 10:13 AM

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: llvm-commits, pengfei, hiraditya, emaste. · View Herald Transcript

MaskRay requested review of this revision.Nov 25 2020, 10:13 AM

MaskRay mentioned this in D91993: [ELF] Don't relax R_X86_64_GOTPCRELX if addend != -4.Nov 25 2020, 10:14 AM

Harbormaster completed remote builds in B80117: Diff 307651.Nov 25 2020, 10:55 AM

I think this makes sense. When we know that a relocation can't be relaxed, we should not emit a potentially relaxable relocation.
This just adds an excessive job to linker and not useful.

Moreover, with the current version of ABI it is just not correct to emit it, as it doesn't yet says anything like "when we have an addend, we can't relax". Such change is going to be added though it seems (https://gitlab.com/x86-psABIs/x86-64-ABI/-/merge_requests/16).

See my comment at the end here: https://reviews.llvm.org/D91993#2417933. I think this is a sensible thing to do, though strictly it's a violation of the psABI as I read it, combined with H J Lu's response to @MaskRay. Ideally, I'd love it if we could get the ABI changed to prohibit the emission of the relax relocation in this case, but I'm not sure if that's really practical for us.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
401–402	I think we should keep the old comment, at least in part, since it may not be obvious to a future developer what a "relaxed relocation form" actually means in practice. We can then just add this new comment after it, saying why we don't emit such a relocation for all situations.
424	Seems like this is a file format limitation, not an OS one, right?
llvm/test/MC/X86/gotpcrelx.s
5–7	Then you can delete the two matching lines for the NORELAX case.
52	(not strictly needed, but good for completeness, I think)

Address comments

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
401–402	The old comment was about "movq loads". I moved it below.

Harbormaster completed remote builds in B80257: Diff 307897.Nov 26 2020, 9:54 AM

My only remaining concern is the comment.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
401–402	The new comment below doesn't say anything about the linker being able "to eliminate some loads for GOT references which end up in the same linkage unit" which I think is useful information and should be kept somewhere.