[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]

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
+# COMMON-NEXT:     R_X86_64_GOTPCREL mov 0xFFFFFFFFFFFFFFFC
+# COMMON-NEXT:   }
+
----------------
(not strictly needed, but good for completeness, I think)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92114/new/

https://reviews.llvm.org/D92114