[MC] [X86] Teach leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 to use R_X86_64_GOTPC32 instead of R_X86_64_PC32

[MaskRay]

Summary

This is similar to D46319 (ARM). x86-64 psABI p40 gives an example:

leaq _GLOBAL_OFFSET_TABLE(%rip), %r15 # GOTPC32 reloc

GNU as creates such R_X86_64_GOTPC32. However, MC currently emits R_X86_64_PC32.

[MaskRay]

Add test

[MaskRay]

Friendly ping :)

[ruiu]

I'm not very familiar with this code, so please take it with a grain of salt.

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

355–356

Is this the right place to add this code? There's code in this function that also set the same value to FixupKind. Can't you merge them?

[craig.topper]

test/MC/ELF/relocation.s

45

If I just add this line to the test, I get the exact output shown in the check lines without any other changes.

[peter.smith]

Apologies I don't know x86 well enough to be authoritative here. It looks like at least the test case needs to use %rip to see a meaningful difference in the output of mc.

test/MC/ELF/relocation.s

45

Going back to the original example from the LLD review D47098 it seems that llvm-mc will emit a R_X86_64_PC32 relocation when the register is %rip but will use R_X86_64_GOTPC32 if the register is something like %rax. I'm guessing that this is because use of %rip means that the fixups are pc relative, when %rax is chosen the existing use of StartsWithGlobalOffsetTable for FK_DATA4, FK_DATA8 catches it.

For the somewhat arbitrary test:

leaq _GLOBAL_OFFSET_TABLE_(%rip), %rax
leaq _GLOBAL_OFFSET_TABLE_(%rax), %r15

mc gives me:

   0:	48 8d 05 00 00 00 00 	lea    0x0(%rip),%rax        # 0x7
			3: R_X86_64_PC32	_GLOBAL_OFFSET_TABLE_-0x4
   7:	4c 8d b8 00 00 00 00 	lea    0x0(%rax),%r15
			a: R_X86_64_GOTPC32	_GLOBAL_OFFSET_TABLE_+0x3

whereas gas gives me:

   0:	48 8d 05 00 00 00 00 	lea    0x0(%rip),%rax        # 0x7
			3: R_X86_64_GOTPC32	_GLOBAL_OFFSET_TABLE_-0x4
   7:	4c 8d b8 00 00 00 00 	lea    0x0(%rax),%r15
			a: R_X86_64_GOTPC32	_GLOBAL_OFFSET_TABLE_

Apologies I don't know enough about x86 to know which of mc or gas is correct in this case. The code in the ARM gas backend that chooses the got relative relocation did seem a bit loose to me so I'd get an x86 expert to check before assuming that gas is right.

[MaskRay]

Update test

[MaskRay]

The r_addend offsets do not change before/after the patch.

In D47507#1118648, @peter.smith wrote:

Apologies I don't know x86 well enough to be authoritative here. It looks like at least the test case needs to use %rip to see a meaningful difference in the output of mc.

Yes, with _GLOBAL_OFFSET_TABLE_(%rax) (is this a malform?), r_addend is different in GNU as (RIP is relative to the location) and llvm-mc (before or after the patch: relative to the currect instruction):

leaq  _GLOBAL_OFFSET_TABLE_(%rax), %r15

But for:

leaq  _GLOBAL_OFFSET_TABLE_(%rip), %r15

Both GNU as and llvm-mc think it should be relative to the RIP of the next instruction.

[MaskRay]

Added the test`leaq _GLOBAL_OFFSET_TABLE_(%rip), %r15` along with leaq _GLOBAL_OFFSET_TABLE_(%rax), %r15

The behavior of GNU as is weird (it is relative neither the current instruction nor the next, but the relocation position).
https://gmplib.org/repo/gmp/file/tip/mpn/x86/README#l361 notes that instructions like leal _GLOBAL_OFFSET_TABLE_(%edi), %ebx can be assembled incorrectly.

I think the current llvm-mc behavior is fine and it is consistent with another test test/MC/ELF/global-offset.s.

[MaskRay]

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

355–356

When %rip is mentioned, r_addend should be relative to the PC of the next instruction thus the ImmOffset -= 4 here (accounting for a 4-byte PC offset). This is different from the cases above (no PC is involved unless the magic _GLOBAL_OFFSET_TABLE_ appears, where r_addend should be relative to the PC of the currenct instruction ).

I think keeping them separate as is is not bad.

[MaskRay]

enthusiastic ping :) 🐱

[echristo]

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

355–356

Perhaps not right above, but I think the code might be clearer if we just add the change in Fixup right before this block and add the condition to this block.

Thoughts?

[MaskRay]

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

355–356

This block deals with 4-byte pc-relative FixupKind, while the 8-byte branch has been handled above.

_GLOBAL_OFFSET_TABLE_ can only be used in 4-byte or 8-byte FixupKind, the 1-byte and 2-byte branches below do not need this fix. Thus I think the current way organizes the logic well. Did I miss something?

[echristo]

One inline comment then OK.

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

355–356

Could you add a comment sorta like this:

// If this is a rip relative load off of the global offset table symbol:
// leaq _GLOBAL_OFFSET_TABLE(%rip), %r15
// this needs to be a GOTPC32 relocation.

Around the if statement?

Otherwise I played around with this and agree, without hoisting a bunch of the if conditionals into a separate function.

[MaskRay]

Add comment suggested by echristo@

[MaskRay]

.

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

EmitImmediate(const MCOperand &DispOp, SMLoc Loc, unsigned Size,
 
   // If the fixup is pc-relative, we need to bias the value to be relative to
   // the start of the field, not the end of the field.
   if (FixupKind == FK_PCRel_4 ||
       FixupKind == MCFixupKind(X86::reloc_riprel_4byte) ||
       FixupKind == MCFixupKind(X86::reloc_riprel_4byte_movq_load) ||
       FixupKind == MCFixupKind(X86::reloc_riprel_4byte_relax) ||
       FixupKind == MCFixupKind(X86::reloc_riprel_4byte_relax_rex) ||
-      FixupKind == MCFixupKind(X86::reloc_branch_4byte_pcrel))
+      FixupKind == MCFixupKind(X86::reloc_branch_4byte_pcrel)) {
     ImmOffset -= 4;
+    // If this is a pc-relative load off of _GLOBAL_OFFSET_TABLE:
+    // leaq _GLOBAL_OFFSET_TABLE(%rip), %r15
+    // this needs to be a GOTPC32 relocation.
+    if (StartsWithGlobalOffsetTable(Expr) != GOT_None)
+      FixupKind = MCFixupKind(X86::reloc_global_offset_table);
+  }
   if (FixupKind == FK_PCRel_2)
     ImmOffset -= 2;
   if (FixupKind == FK_PCRel_1)
     ImmOffset -= 1;
 
   if (ImmOffset)
     Expr = MCBinaryExpr::createAdd(Expr, MCConstantExpr::create(ImmOffset, Ctx),
                                    Ctx);

test/MC/ELF/relocation.s

.long bar                         # R_X86_64_32
   .byte   foo-bar
 
         # this should probably be an error...
   zed = foo +2
   call zed@PLT
 
         leaq    -1+foo(%rip), %r11
 
+        leaq  _GLOBAL_OFFSET_TABLE_(%rax), %r15
+        leaq  _GLOBAL_OFFSET_TABLE_(%rip), %r15
         movl  $_GLOBAL_OFFSET_TABLE_, %eax
         movabs  $_GLOBAL_OFFSET_TABLE_, %rax
 
         .quad    blah@SIZE                        # R_X86_64_SIZE64
         .quad    blah@SIZE + 32                   # R_X86_64_SIZE64
         .quad    blah@SIZE - 32                   # R_X86_64_SIZE64
          movl    blah@SIZE, %eax                  # R_X86_64_SIZE32
          movl    blah@SIZE + 32, %eax             # R_X86_64_SIZE32
          movl    blah@SIZE - 32, %eax             # R_X86_64_SIZE32
 

 // CHECK-NEXT:       0x70 R_X86_64_PC32 foo 0x70
 // CHECK-NEXT:       0x77 R_X86_64_32S .text 0x0
 // CHECK-NEXT:       0x7B R_X86_64_DTPOFF64 foo 0x0
 // CHECK-NEXT:       0x85 R_X86_64_TPOFF64 baz 0x0
 // CHECK-NEXT:       0x8D R_X86_64_PC16 foo 0x8D
 // CHECK-NEXT:       0x8F R_X86_64_PC8 foo 0x8F
 // CHECK-NEXT:       0x91 R_X86_64_PLT32 zed 0xFFFFFFFFFFFFFFFC
 // CHECK-NEXT:       0x98 R_X86_64_PC32 foo 0xFFFFFFFFFFFFFFFB
-// CHECK-NEXT:       0x9D R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0x1
-// CHECK-NEXT:       0xA3 R_X86_64_GOTPC64 _GLOBAL_OFFSET_TABLE_ 0x2
-// CHECK-NEXT:       0xAB R_X86_64_SIZE64 blah 0x0
-// CHECK-NEXT:       0xB3 R_X86_64_SIZE64 blah 0x20
-// CHECK-NEXT:       0xBB R_X86_64_SIZE64 blah 0xFFFFFFFFFFFFFFE0
-// CHECK-NEXT:       0xC6 R_X86_64_SIZE32 blah 0x0
-// CHECK-NEXT:       0xCD R_X86_64_SIZE32 blah 0x20
-// CHECK-NEXT:       0xD4 R_X86_64_SIZE32 blah 0xFFFFFFFFFFFFFFE0
-// CHECK-NEXT:       0xD8 R_X86_64_GOTPCREL foo 0x0
-// CHECK-NEXT:       0xDC R_X86_64_PLT32 foo 0x0
-// CHECK-NEXT:       0xF0 R_X86_64_32 .text 0xF0
-// CHECK-NEXT:       0xF5 R_X86_64_PC16 pr23771 0xFFFFFFFFFFFFFFFE
-// CHECK-NEXT:       0xF7 R_X86_64_PC32 pr23272 0x0
+// CHECK-NEXT:       0x9F R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0x3
+// CHECK-NEXT:       0xA6 R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0xFFFFFFFFFFFFFFFC
+// CHECK-NEXT:       0xAB R_X86_64_GOTPC32 _GLOBAL_OFFSET_TABLE_ 0x1
+// CHECK-NEXT:       0xB1 R_X86_64_GOTPC64 _GLOBAL_OFFSET_TABLE_ 0x2
+// CHECK-NEXT:       0xB9 R_X86_64_SIZE64 blah 0x0
+// CHECK-NEXT:       0xC1 R_X86_64_SIZE64 blah 0x20
+// CHECK-NEXT:       0xC9 R_X86_64_SIZE64 blah 0xFFFFFFFFFFFFFFE0
+// CHECK-NEXT:       0xD4 R_X86_64_SIZE32 blah 0x0
+// CHECK-NEXT:       0xDB R_X86_64_SIZE32 blah 0x20
+// CHECK-NEXT:       0xE2 R_X86_64_SIZE32 blah 0xFFFFFFFFFFFFFFE0
+// CHECK-NEXT:       0xE6 R_X86_64_GOTPCREL foo 0x0
+// CHECK-NEXT:       0xEA R_X86_64_PLT32 foo 0x0
+// CHECK-NEXT:       0xFE R_X86_64_32 .text 0xFE
+// CHECK-NEXT:       0x103 R_X86_64_PC16 pr23771 0xFFFFFFFFFFFFFFFE
+// CHECK-NEXT:       0x105 R_X86_64_PC32 pr23272 0x0
 // CHECK-NEXT:     ]
 // CHECK-NEXT:   }