[ELF] Don't relax R_X86_64_GOTPCRELX if addend != -4

[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 an 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.

If assembled by the integrated assembler, we emit R_X86_64_GOTPCREL for
relocations that definitely cannot be relaxed (D92114), so this patch is not
needed.

This patch disables the relaxation, which is compatible with the implementation in GNU ld
("Add R_X86_64_[REX_]GOTPCRELX support to gas and ld").

[grimar]

My understanding is that R_X86_64_*RELX relocation should never be emitted when a relaxation is not possible.
ABI doesn't say anything about addends I think ("B.2 Optimize GOTPCRELX Relocations", p151, https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf).
So, why should this be handled on linker side and not on the compiler side?

[MaskRay]

In D91993#2413016, @grimar wrote:

My understanding is that R_X86_64_*RELX relocation should never be emitted when a relaxation is not possible.
ABI doesn't say anything about addends I think ("B.2 Optimize GOTPCRELX Relocations", p151, https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf).
So, why should this be handled on linker side and not on the compiler side?

There are two questions:

• Whether movl x@GOTPCREL+4(%rip), %eax is valid?
• Whether the expression should assemble to R_X86_64_GOTPCRELX?

For the first, I tend to think it is valid - this allows the compiler generates an instruction to load the half part of the GOT entry.
For the second, my understanding is that R_X86_64_GOTPCREL can be entirely replaced by R_X86_64_[REX_]GOTPCRELX. If so, it is certainly reasonable to assemble to R_X86_64_GOTPCRELX. I can send a clarification on binutils, though.

[MaskRay]

In D91993#2414225, @MaskRay wrote:

In D91993#2413016, @grimar wrote:

My understanding is that R_X86_64_*RELX relocation should never be emitted when a relaxation is not possible.
ABI doesn't say anything about addends I think ("B.2 Optimize GOTPCRELX Relocations", p151, https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf).
So, why should this be handled on linker side and not on the compiler side?

There are two questions:

• Whether movl x@GOTPCREL+4(%rip), %eax is valid?
• Whether the expression should assemble to R_X86_64_GOTPCRELX?

For the first, I tend to think it is valid - this allows the compiler generates an instruction to load the half part of the GOT entry.
For the second, my understanding is that R_X86_64_GOTPCREL can be entirely replaced by R_X86_64_[REX_]GOTPCRELX. If so, it is certainly reasonable to assemble to R_X86_64_GOTPCRELX. I can send a clarification on binutils, though.

Asked on binutils and HJ Lu said movl x@GOTPCREL+4(%rip), %eax is valid (https://sourceware.org/pipermail/binutils/2020-November/114264.html) and R_X86_64_GOTPCRELX is correct.
So I think we should proceed with this patch. I also created a gold bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=26939

[grimar]

In D91993#2414393, @MaskRay wrote:

In D91993#2414225, @MaskRay wrote:

In D91993#2413016, @grimar wrote:

My understanding is that R_X86_64_*RELX relocation should never be emitted when a relaxation is not possible.
ABI doesn't say anything about addends I think ("B.2 Optimize GOTPCRELX Relocations", p151, https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/x86-64-psABI-1.0.pdf).
So, why should this be handled on linker side and not on the compiler side?

There are two questions:

• Whether movl x@GOTPCREL+4(%rip), %eax is valid?
• Whether the expression should assemble to R_X86_64_GOTPCRELX?

For the first, I tend to think it is valid - this allows the compiler generates an instruction to load the half part of the GOT entry.
For the second, my understanding is that R_X86_64_GOTPCREL can be entirely replaced by R_X86_64_[REX_]GOTPCRELX. If so, it is certainly reasonable to assemble to R_X86_64_GOTPCRELX. I can send a clarification on binutils, though.

Asked on binutils and HJ Lu said movl x@GOTPCREL+4(%rip), %eax is valid (https://sourceware.org/pipermail/binutils/2020-November/114264.html) and R_X86_64_GOTPCRELX is correct.
So I think we should proceed with this patch. I also created a gold bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=26939

OK. Thanks for the information. I still don't understand why the direction is to add job for the linker instead of not emitting a potentially relaxable relocation from the begining on the compiler side.
But since it is what will be added to x86-64-ABI, I have no arguments for not doing this in LLD.

I'd add the TargetInfo::adjustGotPcExpr in a separate patch though to avoid having related PPC64 changes.

lld/ELF/Arch/PPC64.cpp

1395

It feels that splitting the adjustRelaxExpr to adjustGotPcExpr and renaming the first one to adjustTlsExpr should be done separatelly
and then this diff will only contain X86_64 related code?

lld/test/ELF/x86-64-gotpc-offset.s

1

We have x86-64-gotpc-relax.s test case for testing relaxations. I think this test should live there?

[MaskRay]

Extract refactoring part

lld/test/ELF/x86-64-gotpc-offset.s

1

That file is mainly about different instructions... We can use a new file for offsets, I think ....

[grimar]

LGTM.

[jhenderson]

FWIW, I'm with @grimar in the sense that I think this is poor and unnecessary behaviour provided for by the psABI. Why emit the relocation type in this context, when the primary purpose (assuming I'm not mistaken) of this relocation type is to enable relaxations? Why not emit the generic GOTPCREL relocation?

[MaskRay]

In D91993#2416238, @jhenderson wrote:

FWIW, I'm with @grimar in the sense that I think this is poor and unnecessary behaviour provided for by the psABI. Why emit the relocation type in this context, when the primary purpose (assuming I'm not mistaken) of this relocation type is to enable relaxations? Why not emit the generic GOTPCREL relocation?

MC could emit R_X86_64_GOTPCREL in this case. Then call GNU as' emitting R_X86_64_GOTPCRELX a bug?

Hmm, I think the assembler behavior is fine, because R_X86_64_GOTPCREL in all cases can be replaced by R_X86_64_GOTPCRELX and R_X86_64_REX_GOTPCRELX, if the user does not mind the instructions are relaxed.

Note, R_X86_64_GOTPCRELX already does not guarantee ensured relaxation, e.g. when the symbol is preemptible the linker cannot relax the instruction, when the instruction is not one of call,jmp,adc,add,and,cmp,or,sbb,sub,test,xor, etc.

[MaskRay]

If we want to support clang -fno-integrated-as + LLD, we still need this.

I think making the check on the MC side still makes sense: D92114

[grimar]

In D91993#2416496, @MaskRay wrote:

Note, R_X86_64_GOTPCRELX already does not guarantee ensured relaxation, e.g. when the symbol is preemptible the linker cannot relax the instruction, when the instruction is not one of call,jmp,adc,add,and,cmp,or,sbb,sub,test,xor, etc.

Seems that, e.g., R_X86_64_GOTPCREL should give a guarantee that relaxation will not be performed though. As far I understand GNU linkers can relax R_X86_64_GOTPCREL too? It looks like a violation of ABI?
The suggested change (for ABI) about addend then seems only needed to support this behavior of GNU linkers. In LLVM we probably should just not emit a potentially relaxable relocation from the begining.

[jhenderson]

In D91993#2416496, @MaskRay wrote:

In D91993#2416238, @jhenderson wrote:

FWIW, I'm with @grimar in the sense that I think this is poor and unnecessary behaviour provided for by the psABI. Why emit the relocation type in this context, when the primary purpose (assuming I'm not mistaken) of this relocation type is to enable relaxations? Why not emit the generic GOTPCREL relocation?

MC could emit R_X86_64_GOTPCREL in this case. Then call GNU as' emitting R_X86_64_GOTPCRELX a bug?

I wouldn't call it a bug in the assembler, as my understanding from your comments earlier is that it is conformant with the ABI. I do think I'd consider it a possible bug in the psABI though, but maybe I'm misunderstanding the intent of GOTPCRELX (I thought it indicated that the target place is a candidate for relaxation, which the linker may choose to do).

Hmm, I think the assembler behavior is fine, because R_X86_64_GOTPCREL in all cases can be replaced by R_X86_64_GOTPCRELX and R_X86_64_REX_GOTPCRELX, if the user does not mind the instructions are relaxed.

Note, R_X86_64_GOTPCRELX already does not guarantee ensured relaxation, e.g. when the symbol is preemptible the linker cannot relax the instruction, when the instruction is not one of call,jmp,adc,add,and,cmp,or,sbb,sub,test,xor, etc.

In D91993#2417791, @grimar wrote:

Seems that, e.g., R_X86_64_GOTPCREL should give a guarantee that relaxation will not be performed though. As far I understand GNU linkers can relax R_X86_64_GOTPCREL too? It looks like a violation of ABI?

I think our downstream platform has previously relaxed GOTPCREL too (although I'd have to go back and check). I don't think it hurts for a linker to perform an optimisation it can see is safe. I wouldn't say it's a violation of the ABI in this case, rather just something not covered by the ABI. Probably in all those cases that it did that, the relocation is now a GOTPCRELX, but I don't think it always was.

The suggested change (for ABI) about addend then seems only needed to support this behavior of GNU linkers. In LLVM we probably should just not emit a potentially relaxable relocation from the begining.

Strictly, it could be argued that we'd actually be violating the ABI in that case, though I don't disagree with it. The psABI states that in the case of mov name@GOTPCREL(%rip), %reg a GOTPCRELX relocation should be emitted. I'm note convinced that pattern specifically states the same for the +4 version you mentioned before, but HJ Lu seems to suggest it does, so if we didn't emit a GOTPCRELX, we'd be ignoring that statement.

Anyway, that's the other patch (D92114). Since we'll have to accept the GNU assembler output, it probably makes sense to do this patch. I haven't reviewed the content, but seems okay in principle. Feel free to commit once reviewers are happy.

[MaskRay]

In D91993#2417791, @grimar wrote:

In D91993#2416496, @MaskRay wrote:

Note, R_X86_64_GOTPCRELX already does not guarantee ensured relaxation, e.g. when the symbol is preemptible the linker cannot relax the instruction, when the instruction is not one of call,jmp,adc,add,and,cmp,or,sbb,sub,test,xor, etc.

Seems that, e.g., R_X86_64_GOTPCREL should give a guarantee that relaxation will not be performed though. As far I understand GNU linkers can relax R_X86_64_GOTPCREL too? It looks like a violation of ABI?
The suggested change (for ABI) about addend then seems only needed to support this behavior of GNU linkers. In LLVM we probably should just not emit a potentially relaxable relocation from the begining.

GNU ld and gold can relax R_X86_64_GOTPCREL if the instruction is a movq. This behavior is not documented in the x86-64 psABI. I think LLD's choice is more sensible: no relaxation for R_X86_64_GOTPCREL.

(GNU ld's behavior is actually very complex (https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=56ceb5b5405af23eddd12e12d8ba849010120324 not changed a lot since it was committed))

I agree that it is probably impractical for us to change the x86-64 psABI now since the GNU ld behavior was set in stone in 2015 and the psABI is somewhat more about documenting the existing practice and less about what ought to be done.

Now as I see the purpose of this patch, it is entirely to make -fno-integrated-assembler work since GNU as is likely not going to change.

[MaskRay]

Improve tests and comments

[jhenderson]

lld/test/ELF/x86-64-gotpc-offset.s

8–12

Maybe worth using FileCheck numeric variables to caclulate the address values here, so that if LLD lays things out slightly differently in the future, the test doesn't need updating. It also helps make it easier to spot the value differences. See suggested edit for an example.

(I've also added a {{^}} to the start of the first line so that it doesn't accidentally match an address pattern elsewhere in the line)

14

Here and below. The single hash makes it look like the line has some sort of functional affect or is some commented out code that was accidentally left in.

[MaskRay]

comments

[jhenderson]

Looks good from my point of view. Might want @grimar to give thumbs up again too.

[grimar]

LGTM too.

lld/ELF/Arch/PPC64.cpp

public:
   void writeGotHeader(uint8_t *buf) const override;
   bool needsThunk(RelExpr expr, RelType type, const InputFile *file,
                   uint64_t branchAddr, const Symbol &s,
                   int64_t a) const override;
   uint32_t getThunkSectionSpacing() const override;
   bool inBranchRange(RelType type, uint64_t src, uint64_t dst) const override;
   RelExpr adjustRelaxExpr(RelType type, const uint8_t *data,
                           RelExpr expr) const override;
+  RelExpr adjustGotPcExpr(RelType type, int64_t addend,
+                          const uint8_t *loc) const override;
   void relaxGot(uint8_t *loc, const Relocation &rel,
                 uint64_t val) const override;
   void relaxTlsGdToIe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
   void relaxTlsGdToLe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
   void relaxTlsLdToLe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
bool PPC64::inBranchRange(RelType type, uint64_t src, uint64_t dst) const {
   int64_t offset = dst - src;
   if (type == R_PPC64_REL14)
     return isInt<16>(offset);
   if (type == R_PPC64_REL24 || type == R_PPC64_REL24_NOTOC)
     return isInt<26>(offset);
   llvm_unreachable("unsupported relocation type used in branch");
 }
 
 RelExpr PPC64::adjustRelaxExpr(RelType type, const uint8_t *data,
                                RelExpr expr) const {
+  if (type != R_PPC64_GOT_TLSGD_PCREL34 && expr == R_RELAX_TLS_GD_TO_IE)
+    return R_RELAX_TLS_GD_TO_IE_GOT_OFF;
+  if (expr == R_RELAX_TLS_LD_TO_LE)
+    return R_RELAX_TLS_LD_TO_LE_ABS;
+  return expr;
+}
+
+RelExpr PPC64::adjustGotPcExpr(RelType type, int64_t addend,
+                               const uint8_t *loc) const {
   if ((type == R_PPC64_GOT_PCREL34 || type == R_PPC64_PCREL_OPT) &&
       config->pcRelOptimize) {
     // It only makes sense to optimize pld since paddi means that the address
     // of the object in the GOT is required rather than the object itself.
-    assert(data && "Expecting an instruction encoding here");
-    if ((readPrefixedInstruction(data) & 0xfc000000) == 0xe4000000)
+    if ((readPrefixedInstruction(loc) & 0xfc000000) == 0xe4000000)
       return R_PPC64_RELAX_GOT_PC;
   }
-
-  if (type != R_PPC64_GOT_TLSGD_PCREL34 && expr == R_RELAX_TLS_GD_TO_IE)
-    return R_RELAX_TLS_GD_TO_IE_GOT_OFF;
-  if (expr == R_RELAX_TLS_LD_TO_LE)
-    return R_RELAX_TLS_LD_TO_LE_ABS;
-  return expr;
+  return R_GOT_PC;
 }
 
 // Reference: 3.7.4.1 of the 64-bit ELF V2 abi supplement.
 // The general dynamic code sequence for a global `x` uses 4 instructions.
 // Instruction                    Relocation                Symbol
 // addis r3, r2, x@got@tlsgd@ha   R_PPC64_GOT_TLSGD16_HA      x
 // addi  r3, r3, x@got@tlsgd@l    R_PPC64_GOT_TLSGD16_LO      x
 // bl __tls_get_addr(x@tlsgd)     R_PPC64_TLSGD               x

lld/ELF/Arch/X86_64.cpp

public:
   void writePltHeader(uint8_t *buf) const override;
   void writePlt(uint8_t *buf, const Symbol &sym,
                 uint64_t pltEntryAddr) const override;
   void relocate(uint8_t *loc, const Relocation &rel,
                 uint64_t val) const override;
   void applyJumpInstrMod(uint8_t *loc, JumpModType type,
                          unsigned size) const override;
 
-  RelExpr adjustRelaxExpr(RelType type, const uint8_t *data,
-                          RelExpr expr) const override;
+  RelExpr adjustGotPcExpr(RelType type, int64_t addend,
+                          const uint8_t *loc) const override;
   void relaxGot(uint8_t *loc, const Relocation &rel,
                 uint64_t val) const override;
   void relaxTlsGdToIe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
   void relaxTlsGdToLe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
   void relaxTlsIeToLe(uint8_t *loc, const Relocation &rel,
                       uint64_t val) const override;
void X86_64::relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const {
   case R_X86_64_GOTPC64:
     write64le(loc, val);
     break;
   default:
     llvm_unreachable("unknown relocation");
   }
 }
 
-RelExpr X86_64::adjustRelaxExpr(RelType type, const uint8_t *data,
-                                RelExpr relExpr) const {
-  if (type != R_X86_64_GOTPCRELX && type != R_X86_64_REX_GOTPCRELX)
-    return relExpr;
-  const uint8_t op = data[-2];
-  const uint8_t modRm = data[-1];
+RelExpr X86_64::adjustGotPcExpr(RelType type, int64_t addend,
+                                const uint8_t *loc) const {
+  // Only R_X86_64_[REX_]GOTPCRELX can be relaxed. addend != -4 means the
+  // instruction does not load the full GOT entry, we cannot relax the
+  // relocation. E.g. movl x@GOTPCREL+4(%rip), %rax (addend=0) loads the high 32
+  // bits of the GOT entry.
+  if ((type != R_X86_64_GOTPCRELX && type != R_X86_64_REX_GOTPCRELX) ||
+      addend != -4)
+    return R_GOT_PC;
+  const uint8_t op = loc[-2];
+  const uint8_t modRm = loc[-1];
 
   // FIXME: When PIC is disabled and foo is defined locally in the
   // lower 32 bit address space, memory operand in mov can be converted into
   // immediate operand. Otherwise, mov must be changed to lea. We support only
   // latter relaxation at this moment.
   if (op == 0x8b)
     return R_RELAX_GOT_PC;
 
   // Relax call and jmp.
   if (op == 0xff && (modRm == 0x15 || modRm == 0x25))
     return R_RELAX_GOT_PC;
 
   // We don't support test/binop instructions without a REX prefix.
   if (type == R_X86_64_GOTPCRELX)
-    return relExpr;
+    return R_GOT_PC;
 
   // Relaxation of test, adc, add, and, cmp, or, sbb, sub, xor.
   // If PIC then no relaxation is available.
-  return config->isPic ? relExpr : R_RELAX_GOT_PC_NOPIC;
+  return config->isPic ? R_GOT_PC : R_RELAX_GOT_PC_NOPIC;
 }
 
 // A subset of relaxations can only be applied for no-PIC. This method
 // handles such relaxations. Instructions encoding information was taken from:
 // "Intel 64 and IA-32 Architectures Software Developer's Manual V2"
 // (http://www.intel.com/content/dam/www/public/us/en/documents/manuals/
 //    64-ia-32-architectures-software-developer-instruction-set-reference-manual-325383.pdf)
 static void relaxGotNoPic(uint8_t *loc, uint64_t val, uint8_t op,

lld/ELF/Relocations.cpp

static void scanReloc(InputSectionBase &sec, OffsetGetter &getOffset, RelTy *&i,
   //
   // If we know that a PLT entry will be resolved within the same ELF module, we
   // can skip PLT access and directly jump to the destination function. For
   // example, if we are linking a main executable, all dynamic symbols that can
   // be resolved within the executable will actually be resolved that way at
   // runtime, because the main executable is always at the beginning of a search
   // list. We can leverage that fact.
   if (!sym.isPreemptible && (!sym.isGnuIFunc() || config->zIfuncNoplt)) {
-    if (expr == R_GOT_PC && !isAbsoluteValue(sym)) {
-      expr = target->adjustRelaxExpr(type, relocatedAddr, expr);
-    } else {
+    if (expr != R_GOT_PC) {
       // The 0x8000 bit of r_addend of R_PPC_PLTREL24 is used to choose call
       // stub type. It should be ignored if optimized to R_PC.
       if (config->emachine == EM_PPC && expr == R_PPC32_PLTREL)
         addend &= ~0x8000;
       // R_HEX_GD_PLT_B22_PCREL (call a@GDPLT) is transformed into
       // call __tls_get_addr even if the symbol is non-preemptible.
       if (!(config->emachine == EM_HEXAGON &&
            (type == R_HEX_GD_PLT_B22_PCREL ||
             type == R_HEX_GD_PLT_B22_PCREL_X ||
             type == R_HEX_GD_PLT_B32_PCREL_X)))
       expr = fromPlt(expr);
+    } else if (!isAbsoluteValue(sym)) {
+      expr = target->adjustGotPcExpr(type, addend, relocatedAddr);
     }
   }
 
   // If the relocation does not emit a GOT or GOTPLT entry but its computation
   // uses their addresses, we need GOT or GOTPLT to be created.
   //
   // The 4 types that relative GOTPLT are all x86 and x86-64 specific.
   if (oneof<R_GOTPLTONLY_PC, R_GOTPLTREL, R_GOTPLT, R_TLSGD_GOTPLT>(expr)) {

lld/ELF/Target.h

public:
 
   // If a target needs to rewrite calls to __morestack to instead call
   // __morestack_non_split when a split-stack enabled caller calls a
   // non-split-stack callee this will return true. Otherwise returns false.
   bool needsMoreStackNonSplit = true;
 
   virtual RelExpr adjustRelaxExpr(RelType type, const uint8_t *data,
                                   RelExpr expr) const;
+  virtual RelExpr adjustGotPcExpr(RelType type, int64_t addend,
+                                  const uint8_t *loc) const;
   virtual void relaxGot(uint8_t *loc, const Relocation &rel,
                         uint64_t val) const;
   virtual void relaxTlsGdToIe(uint8_t *loc, const Relocation &rel,
                               uint64_t val) const;
   virtual void relaxTlsGdToLe(uint8_t *loc, const Relocation &rel,
                               uint64_t val) const;
   virtual void relaxTlsIeToLe(uint8_t *loc, const Relocation &rel,
                               uint64_t val) const;

lld/ELF/Target.cpp

bool TargetInfo::inBranchRange(RelType type, uint64_t src, uint64_t dst) const {
   return true;
 }
 
 RelExpr TargetInfo::adjustRelaxExpr(RelType type, const uint8_t *data,
                                     RelExpr expr) const {
   return expr;
 }
 
+RelExpr TargetInfo::adjustGotPcExpr(RelType type, int64_t addend,
+                                    const uint8_t *data) const {
+  return R_GOT_PC;
+}
+
 void TargetInfo::relaxGot(uint8_t *loc, const Relocation &rel,
                           uint64_t val) const {
   llvm_unreachable("Should not have claimed to be relaxable");
 }
 
 void TargetInfo::relaxTlsGdToLe(uint8_t *loc, const Relocation &rel,
                                 uint64_t val) const {
   llvm_unreachable("Should not have claimed to be relaxable");

lld/test/ELF/x86-64-gotpc-offset.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-objdump -s -d %t | FileCheck %s
+
+# CHECK:      Contents of section .got:
+# CHECK-NEXT:  2021a0 9d112000 00000000
+
+## The second instruction has an offset (addend!=-4). It is incorrect to relax
+## movl to leal.
+# CHECK:      leaq 6(%rip), %rax  # {{.*}} <foo>
+# CHECK-NEXT: movl {{.*}}(%rip), %eax  # 2021a4
+  movq foo@GOTPCREL(%rip), %rax
+  movl foo@GOTPCREL+4(%rip), %eax
+
+.globl foo
+foo:
+  nop