Page MenuHomePhabricator

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

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

Details

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

Event Timeline

MaskRay created this revision.Nov 25 2020, 10:13 AM
MaskRay requested review of this revision.Nov 25 2020, 10:13 AM
grimar added a comment.EditedNov 25 2020, 11:51 PM

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)

MaskRay updated this revision to Diff 307897.Nov 26 2020, 9:20 AM
MaskRay marked 4 inline comments as done.

Address comments

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
401–402

The old comment was about "movq loads". I moved it below.

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.

MaskRay updated this revision to Diff 308080.Nov 27 2020, 9:42 AM
MaskRay marked an inline comment as done.

Add a comment

jhenderson accepted this revision.Nov 30 2020, 12:15 AM

Looks good.

This revision is now accepted and ready to land.Nov 30 2020, 12:15 AM
MaskRay edited the summary of this revision. (Show Details)Nov 30 2020, 8:25 AM