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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 26 01:31:57 PST 2020

jhenderson added a comment.

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.

Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:401-402
+    unsigned FixupKind = [&]() {
+      // Enable relaxed relocation only for a MCSymbolRefExpr.  We cannot
+      // enabled relaxed relocation if an offset is present (e.g. x at GOTPCREL+4).
+      if (!(Disp.isExpr() && isa<MCSymbolRefExpr>(Disp.getExpr())))
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.

Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:421
+        // movq loads is a subset of reloc_riprel_4byte_relax_rex. It is a
+        // special case because Mach-O and Windows don't support ELF's more
+        // flexible relaxation.
Seems like this is a file format limitation, not an OS one, right?

Comment at: llvm/test/MC/X86/gotpcrelx.s:6-7
 # CHECK:      Relocations [
 # CHECK-NEXT:   Section ({{.*}}) .rela.text {
 # CHECK-NEXT:     R_X86_64_GOTPCRELX mov
Then you can delete the two matching lines for the NORELAX case.

Comment at: llvm/test/MC/X86/gotpcrelx.s:54
(not strictly needed, but good for completeness, I think)

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list