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

[MaskRay]

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.

[grimar]

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).

[jhenderson]

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]

Address comments

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

401–402

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

[jhenderson]

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]

Add a comment

[jhenderson]

Looks good.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

if (BaseReg == X86::RIP ||
       BaseReg == X86::EIP) { // [disp32+rIP] in X86-64 mode
     assert(STI.hasFeature(X86::Mode64Bit) &&
            "Rip-relative addressing requires 64-bit mode");
     assert(IndexReg.getReg() == 0 && !ForceSIB &&
            "Invalid rip-relative address");
     emitByte(modRMByte(0, RegOpcodeField, 5), OS);
 
     unsigned Opcode = MI.getOpcode();
-    // movq loads are handled with a special relocation form which allows the
-    // linker to eliminate some loads for GOT references which end up in the
-    // same linkage unit.
-    unsigned FixupKind = [=]() {
+    unsigned FixupKind = [&]() {
+      // Enable relaxed relocation only for a MCSymbolRefExpr.  We cannot use a
+      // relaxed relocation if an offset is present (e.g. x@GOTPCREL+4).
+      if (!(Disp.isExpr() && isa<MCSymbolRefExpr>(Disp.getExpr())))
+        return X86::reloc_riprel_4byte;
+
+      // Certain loads for GOT references can be relocated against the symbol
+      // directly if the symbol ends up in the same linkage unit.
       switch (Opcode) {
       default:
         return X86::reloc_riprel_4byte;
       case X86::ADC32rm:
       case X86::ADD32rm:
       case X86::AND32rm:
       case X86::CMP32rm:
       case X86::MOV32rm:
       case X86::OR32rm:
       case X86::SBB32rm:
       case X86::SUB32rm:
       case X86::TEST32mr:
       case X86::XOR32rm:
         return X86::reloc_riprel_4byte_relax;
       case X86::MOV64rm:
+        // movq loads is a subset of reloc_riprel_4byte_relax_rex. It is a
+        // special case because COFF and Mach-O don't support ELF's more
+        // flexible R_X86_64_REX_GOTPCRELX relaxation.
         assert(HasREX);
         return X86::reloc_riprel_4byte_movq_load;
       case X86::CALL64m:
       case X86::JMP64m:
       case X86::TAILJMPm64:
       case X86::TEST64mr:
       case X86::ADC64rm:
       case X86::ADD64rm:

llvm/test/MC/ELF/got-relaxed-rex.s

 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o - | llvm-readobj -r - | FileCheck %s
 
 // these should produce R_X86_64_REX_GOTPCRELX
 
         movq mov@GOTPCREL(%rip), %rax
         test %rax, test@GOTPCREL(%rip)
         adc adc@GOTPCREL(%rip), %rax
         add add@GOTPCREL(%rip), %rax
         and and@GOTPCREL(%rip), %rax
         cmp cmp@GOTPCREL(%rip), %rax
         or  or@GOTPCREL(%rip), %rax
         sbb sbb@GOTPCREL(%rip), %rax
         sub sub@GOTPCREL(%rip), %rax
         xor xor@GOTPCREL(%rip), %rax
 
+.section .norelax,"ax"
+## This expression loads the GOT entry with an offset.
+## Don't emit R_X86_64_REX_GOTPCRELX.
+        movq mov@GOTPCREL+1(%rip), %rax
+
 // CHECK:      Relocations [
 // CHECK-NEXT:   Section ({{.*}}) .rela.text {
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX mov
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX test
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX adc
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX add
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX and
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX cmp
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX or
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX sbb
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX sub
 // CHECK-NEXT:     R_X86_64_REX_GOTPCRELX xor
 // CHECK-NEXT:   }
-// CHECK-NEXT: ]
+// CHECK-NEXT:   Section ({{.*}}) .rela.norelax {
+// CHECK-NEXT:     R_X86_64_GOTPCREL mov
+// CHECK-NEXT:   }

llvm/test/MC/X86/gotpcrelx.s

-# RUN: llvm-mc -filetype=obj -triple=x86_64 %s | llvm-readobj -r - | FileCheck %s
-# RUN: llvm-mc -filetype=obj -triple=x86_64 -relax-relocations=false %s | llvm-readobj -r - | FileCheck --check-prefix=NORELAX %s
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: llvm-readobj -r %t.o | FileCheck %s --check-prefixes=CHECK,COMMON
+# RUN: llvm-mc -filetype=obj -triple=x86_64 -relax-relocations=false %s -o %t1.o
+# RUN: llvm-readobj -r %t1.o | FileCheck %s --check-prefixes=NORELAX,COMMON
 
-# CHECK:      Relocations [
-# CHECK-NEXT:   Section ({{.*}}) .rela.text {
+# COMMON:     Relocations [
+# COMMON-NEXT:  Section ({{.*}}) .rela.text {
 # CHECK-NEXT:     R_X86_64_GOTPCRELX mov
 # CHECK-NEXT:     R_X86_64_GOTPCRELX test
 # CHECK-NEXT:     R_X86_64_GOTPCRELX adc
 # CHECK-NEXT:     R_X86_64_GOTPCRELX add
 # CHECK-NEXT:     R_X86_64_GOTPCRELX and
 # CHECK-NEXT:     R_X86_64_GOTPCRELX cmp
 # CHECK-NEXT:     R_X86_64_GOTPCRELX or
 # CHECK-NEXT:     R_X86_64_GOTPCRELX sbb
 # CHECK-NEXT:     R_X86_64_GOTPCRELX sub
 # CHECK-NEXT:     R_X86_64_GOTPCRELX xor
 # CHECK-NEXT:     R_X86_64_GOTPCRELX call
 # CHECK-NEXT:     R_X86_64_GOTPCRELX jmp
 # CHECK-NEXT:   }
-# CHECK-NEXT: ]
 
-# NORELAX:      Relocations [
-# NORELAX-NEXT:   Section ({{.*}}) .rela.text {
 # NORELAX-NEXT:     R_X86_64_GOTPCREL mov
 # NORELAX-NEXT:     R_X86_64_GOTPCREL test
 # NORELAX-NEXT:     R_X86_64_GOTPCREL adc
 # NORELAX-NEXT:     R_X86_64_GOTPCREL add
 # NORELAX-NEXT:     R_X86_64_GOTPCREL and
 # NORELAX-NEXT:     R_X86_64_GOTPCREL cmp
 # NORELAX-NEXT:     R_X86_64_GOTPCREL or
 # NORELAX-NEXT:     R_X86_64_GOTPCREL sbb
 # NORELAX-NEXT:     R_X86_64_GOTPCREL sub
 # NORELAX-NEXT:     R_X86_64_GOTPCREL xor
 # NORELAX-NEXT:     R_X86_64_GOTPCREL call
 # NORELAX-NEXT:     R_X86_64_GOTPCREL jmp
 # NORELAX-NEXT:   }
-# NORELAX-NEXT: ]
 
 movl mov@GOTPCREL(%rip), %eax
 test %eax, test@GOTPCREL(%rip)
 adc adc@GOTPCREL(%rip), %eax
 add add@GOTPCREL(%rip), %eax
 and and@GOTPCREL(%rip), %eax
 cmp cmp@GOTPCREL(%rip), %eax
 or  or@GOTPCREL(%rip), %eax
 sbb sbb@GOTPCREL(%rip), %eax
 sub sub@GOTPCREL(%rip), %eax
 xor xor@GOTPCREL(%rip), %eax
 call *call@GOTPCREL(%rip)
 jmp *jmp@GOTPCREL(%rip)
+
+# COMMON-NEXT:   Section ({{.*}}) .rela.norelax {
+# COMMON-NEXT:     R_X86_64_GOTPCREL mov 0x0
+# COMMON-NEXT:     R_X86_64_GOTPCREL mov 0xFFFFFFFFFFFFFFFC
+# COMMON-NEXT:   }
+# COMMON-NEXT: ]
+
+.section .norelax,"ax",@progbits
+## Clang may emit this expression to load the high 32-bit of the GOT entry.
+## Don't emit R_X86_64_GOTPCRELX.
+movl mov@GOTPCREL+4(%rip), %eax
+## We could emit R_X86_64_GOTPCRELX, but it is probably unnecessary.
+movl mov@GOTPCREL+0(%rip), %eax