[ELF] - Fix R_AARCH64_ADR_GOT_PAGE, R_AARCH64_LD64_GOT_LO12 handling against IFUNC symbols.

[grimar]

Summary

This is the same issue as was fixed in https://reviews.llvm.org/D54145,
but for AArch64.

[grimar]

With that, the test case from the https://reviews.llvm.org/D54145 description starts to work correctly.

[peter.smith]

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

llvm/tools/lld/test/ELF/gotpc-relax.s:27:16: error: DISASM-NEXT: expected string not found in input
# DISASM-NEXT: 20101f: {{.*}} movq 8154(%rip), %rax
               ^
<stdin>:19:2: note: scanning from here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax
 ^
<stdin>:19:25: note: possible intended match here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax

It will be worth double checking to see if this hasn't broken x86? Or am I missing something?

[grimar]

In D54314#1293000, @peter.smith wrote:

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

llvm/tools/lld/test/ELF/gotpc-relax.s:27:16: error: DISASM-NEXT: expected string not found in input
# DISASM-NEXT: 20101f: {{.*}} movq 8154(%rip), %rax
               ^
<stdin>:19:2: note: scanning from here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax
 ^
<stdin>:19:25: note: possible intended match here
 20101f: 48 8b 05 da 0f 00 00 movq 4058(%rip), %rax

It will be worth double checking to see if this hasn't broken x86? Or am I missing something?

I assumed this should be independent change, so I did not test them together. I'll try to merge them and check the case you are observing. Thanks!

[grimar]

In D54314#1293000, @peter.smith wrote:

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

I merged this one with D54145 today manually and run the test suite. It does not fail for me.
They are fully independent I beleive, so it is what I expected.
Here is the merged version I got:

patch.patch122 KBDownload

I think just something went wrong on your side perhaps?

[ruiu]

ELF/Relocations.h

38

Please add comments to the entries you are adding in this patch. We have to add comments to all these enums. These constants are extremely important but has no description at all. As a starter, we shouldn't add new entries without comments.

[grimar]

• Added comments for new R_* entries.

[peter.smith]

In D54314#1294189, @grimar wrote:

In D54314#1293000, @peter.smith wrote:

I wasn't sure if this was to be applied on top of D54154 or not so I merged the two and ended up with a test failure in the test modifications done in ELF/gotpc-relax.s for D54154.

I merged this one with D54145 today manually and run the test suite. It does not fail for me.
They are fully independent I beleive, so it is what I expected.
Here is the merged version I got:

patch.patch122 KBDownload

I think just something went wrong on your side perhaps?

Yes, comparing the diffs, it looks like I missed out adding R_GOT_PC_PLT to needsPlt and needsGot from D54145. With that I get the x86 test passing. Apologies for the noise.

[ruiu]

ELF/InputSection.cpp

608

So, this is not specific to AArch64 and

627

this is? It seems a bit weird.

[ruiu]

Oh okay, so you have two patches that contain duplicate changes. I'll review the other one first.

[grimar]

ELF/InputSection.cpp

627

That is because AArch64 use 2 relocations at the same time:

adrp x8, :got:myfunc               # R_AARCH64_ADR_GOT_PAGE
ldr  x8, [x8, :got_lo12:myfunc] # R_AARCH64_LD64_GOT_LO12

R_AARCH64_ADR_GOT_PAGE has R_GOT_PAGE_PC expression and
R_AARCH64_LD64_GOT_LO12 has R_GOT initially and then
converted to *_PLT expressions with 'toPlt()'

[ruiu]

ELF/InputSection.cpp

627

I mean this is actually AArch64 specific, no? It clearly uses getAarch64Page. It should be named R_AARCH64_*.

[grimar]

ELF/InputSection.cpp

627

Probably. Then R_GOT_PAGE_PC should also be renamed. I was consistent and only added a suffix _PLT.

[ruiu]

ELF/InputSection.cpp

627

Can you rename them in another patch?

[grimar]

• Rebased, renamed new relocation expression.

ELF/InputSection.cpp

627

Done in https://reviews.llvm.org/rLLD346749.

[peter.smith]

I've tested the patch against the original C++ example in D54145 and that works ok. I get a segfault when I remove -fPIC though. It does work correctly when I compile with clang but link with ld.bfd (From binutils 2.25).

I'll track down the segfault and will report back as soon as I can. With luck we are just missing something out rather than there being a problem. I think what we have here is a definite improvement though so we could do further work in a follow up patch.

[grimar]

In D54314#1296721, @peter.smith wrote:

I've tested the patch against the original C++ example in D54145 and that works ok. I get a segfault when I remove -fPIC though. It does work correctly when I compile with clang but link with ld.bfd (From binutils 2.25).

I'll track down the segfault and will report back as soon as I can. With luck we are just missing something out rather than there being a problem. I think what we have here is a definite improvement though so we could do further work in a follow up patch.

I guess the segfault you are talking about is the same I mentioned here: https://reviews.llvm.org/D54145#1291558.
It reproduced without any of my patched, so seems to be a different issue to fix for AArch64.

[peter.smith]

Yes it is the same one as reported there. I think you are most likely right that it is independent of this particular change.

[peter.smith]

Tracing the segfault in the example:

extern "C" int myfunc();

int main() {

int (*p)() = &myfunc;

return p();

}

0000000000000000 <main>:
   0:	d10083ff 	sub	sp, sp, #0x20
   4:	a9017bfd 	stp	x29, x30, [sp, #16]
   8:	910043fd 	add	x29, sp, #0x10
   c:	90000008 	adrp	x8, 0 <myfunc>
			c: R_AARCH64_ADR_PREL_PG_HI21	myfunc
  10:	91000108 	add	x8, x8, #0x0
			10: R_AARCH64_ADD_ABS_LO12_NC	myfunc
  14:	b81fc3bf 	stur	wzr, [x29, #-4]
  18:	f90003e8 	str	x8, [sp]
  1c:	f94003e8 	ldr	x8, [sp]
  20:	d63f0100 	blr	x8
  24:	a9417bfd 	ldp	x29, x30, [sp, #16]
  28:	910083ff 	add	sp, sp, #0x20
  2c:	d65f03c0 	ret

The R_AARCH64_ADR_PREL_PG_HI21 is represented by R_PAGE_PC. This is actually correctly changed to R_PLT_PAGE_PC by toPlt and a PLT is generated. Unfortunately the resolution of the relocation is incorrect.

000000000022016c <main>:
  22016c:	d10083ff 	sub	sp, sp, #0x20
  220170:	a9017bfd 	stp	x29, x30, [sp, #16]
  220174:	910043fd 	add	x29, sp, #0x10
  220178:	90000008 	adrp	x8, 220000 <_start>
  22017c:	911d4108 	add	x8, x8, #0x750
// The previous 2 instructions should resolve to 27f750
  220180:	b81fc3bf 	stur	wzr, [x29, #-4]
  220184:	f90003e8 	str	x8, [sp]
  220188:	f94003e8 	ldr	x8, [sp]
  22018c:	d63f0100 	blr	x8
  220190:	a9417bfd 	ldp	x29, x30, [sp, #16]
  220194:	910083ff 	add	sp, sp, #0x20
  220198:	d65f03c0 	ret
  22019c:	00000000 	.inst	0x00000000 ; undefined

...

000000000027f750 <.plt>:
  27f750:	d0000010 	adrp	x16, 281000 <main_arena+0x5c8>
  27f754:	f944f211 	ldr	x17, [x16, #2528]
  27f758:	91278210 	add	x16, x16, #0x9e0
  27f75c:	d61f0220 	br	x17

Not quite sure what has gone wrong here.

[peter.smith]

It looks like the handling for R_PLT_PAGE_PC is broken. It is handled exactly as R_PAGE_PC which always uses Sym.getVA(A). I think it should be using (Sym.getPltVA() + A). I'll submit a separate patch for this as it is unrelated.

[peter.smith]

Created D54474 for the related non -fpic segfault.

[grimar]

Can we land it, Rui?

[grimar]

Ping.

[peter.smith]

I think this looks good. It is likely that US based people will be away for thanks giving so it will be worth pinging Rui next week.

[ruiu]

LGTM

ELF/InputSection.cpp

static uint64_t getRelocTargetVA(const InputFile *File, RelType Type, int64_t A,
   case R_RELAX_TLS_LD_TO_LE_ABS:
   case R_RELAX_GOT_PC_NOPIC:
     return Sym.getVA(A);
   case R_ADDEND:
     return A;
   case R_ARM_SBREL:
     return Sym.getVA(A) - getARMStaticBase(Sym);
   case R_GOT:
+  case R_GOT_PLT:
   case R_RELAX_TLS_GD_TO_IE_ABS:
     return Sym.getGotVA() + A;
   case R_GOTONLY_PC:
     return In.Got->getVA() + A - P;
   case R_GOTONLY_PC_FROM_END:
     return In.Got->getVA() + A - P + In.Got->getSize();
   case R_GOTREL:
     return Sym.getVA(A) - In.Got->getVA();
   case R_GOTREL_FROM_END:
     return Sym.getVA(A) - In.Got->getVA() - In.Got->getSize();
   case R_GOT_FROM_END:
   case R_RELAX_TLS_GD_TO_IE_END:
     return Sym.getGotOffset() + A - In.Got->getSize();
   case R_TLSLD_GOT_OFF:
   case R_GOT_OFF:
   case R_RELAX_TLS_GD_TO_IE_GOT_OFF:
     return Sym.getGotOffset() + A;
   case R_GOT_PAGE_PC:
+  case R_GOT_PAGE_PC_PLT:
   case R_RELAX_TLS_GD_TO_IE_PAGE_PC:
     return getAArch64Page(Sym.getGotVA() + A) - getAArch64Page(P);
   case R_GOT_PC:
   case R_RELAX_TLS_GD_TO_IE:
     return Sym.getGotVA() + A - P;
   case R_HEXAGON_GOT:
     return Sym.getGotVA() - In.GotPlt->getVA();
   case R_MIPS_GOTREL:

ELF/Relocations.h

 // from files are converted to these types so that the main code
 // doesn't have to know about architecture-specific details.
 enum RelExpr {
   R_INVALID,
   R_ABS,
   R_ADDEND,
   R_ARM_SBREL,
   R_GOT,
+  // The expression is used for IFUNC support. Evaluates to GOT entry,
+  // containing redirection to the IPLT.
+  R_GOT_PLT,
   R_GOTONLY_PC,
   R_GOTONLY_PC_FROM_END,
   R_GOTREL,
   R_GOTREL_FROM_END,
   R_GOT_FROM_END,
   R_GOT_OFF,
   R_GOT_PAGE_PC,
+  // The expression is used for IFUNC support. Describes PC-relative
+  // address of the memory page of GOT entry. This entry is used for
+  // a redirection to IPLT.
+  R_GOT_PAGE_PC_PLT,
   R_GOT_PC,
   R_HEXAGON_GOT,
   R_HINT,
   R_MIPS_GOTREL,
   R_MIPS_GOT_GP,
   R_MIPS_GOT_GP_PC,
   R_MIPS_GOT_LOCAL_PAGE,
   R_MIPS_GOT_OFF,

ELF/Relocations.cpp

 }
 
 static bool isAbsoluteValue(const Symbol &Sym) {
   return isAbsolute(Sym) || Sym.isTls();
 }
 
 // Returns true if Expr refers a PLT entry.
 static bool needsPlt(RelExpr Expr) {
-  return isRelExprOneOf<R_PLT_PC, R_PPC_CALL_PLT, R_PLT, R_PLT_PAGE_PC>(Expr);
+  return isRelExprOneOf<R_PLT_PC, R_PPC_CALL_PLT, R_PLT, R_PLT_PAGE_PC,
+                        R_GOT_PLT, R_GOT_PAGE_PC_PLT>(Expr);
 }
 
 // Returns true if Expr refers a GOT entry. Note that this function
 // returns false for TLS variables even though they need GOT, because
 // TLS variables uses GOT differently than the regular variables.
 static bool needsGot(RelExpr Expr) {
   return isRelExprOneOf<R_GOT, R_GOT_OFF, R_HEXAGON_GOT, R_MIPS_GOT_LOCAL_PAGE,
                         R_MIPS_GOT_OFF, R_MIPS_GOT_OFF32, R_GOT_PAGE_PC,
-                        R_GOT_PC, R_GOT_FROM_END>(Expr);
+                        R_GOT_PC, R_GOT_FROM_END, R_GOT_PLT, R_GOT_PAGE_PC_PLT>(
+      Expr);
 }
 
 // True if this expression is of the form Sym - X, where X is a position in the
 // file (PC, or GOT for example).
 static bool isRelExpr(RelExpr Expr) {
   return isRelExprOneOf<R_PC, R_GOTREL, R_GOTREL_FROM_END, R_MIPS_GOTREL,
                         R_PPC_CALL, R_PPC_CALL_PLT, R_PAGE_PC,
                         R_RELAX_GOT_PC>(Expr);
static RelExpr toPlt(RelExpr Expr) {
   case R_PPC_CALL:
     return R_PPC_CALL_PLT;
   case R_PC:
     return R_PLT_PC;
   case R_PAGE_PC:
     return R_PLT_PAGE_PC;
   case R_ABS:
     return R_PLT;
+  case R_GOT:
+    return R_GOT_PLT;
+  case R_GOT_PAGE_PC:
+    return R_GOT_PAGE_PC_PLT;
   default:
     return Expr;
   }
 }
 
 static RelExpr fromPlt(RelExpr Expr) {
   // We decided not to use a plt. Optimize a reference to the plt to a
   // reference to the symbol itself.
static void addPltEntry(PltSection *Plt, GotPltSection *GotPlt,
   GotPlt->addEntry(Sym);
   Rel->addReloc(
       {Type, GotPlt, Sym.getGotPltOffset(), !Sym.IsPreemptible, &Sym, 0});
 }
 
 template <class ELFT> static void addGotEntry(Symbol &Sym) {
   In.Got->addEntry(Sym);
 
-  RelExpr Expr = Sym.isTls() ? R_TLS : R_ABS;
+  RelExpr Expr;
+  if (Sym.isTls())
+    Expr = R_TLS;
+  else if (Sym.isGnuIFunc())
+    Expr = R_PLT;
+  else
+    Expr = R_ABS;
+
   uint64_t Off = Sym.getGotOffset();
 
   // If a GOT slot value can be calculated at link-time, which is now,
   // we can just fill that out.
   //
   // (We don't actually write a value to a GOT slot right now, but we
   // add a static relocation to a Relocations vector so that
   // InputSection::relocate will do the work for us. We may be able

test/ELF/aarch64-gnu-ifunc2.s

+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64-none-linux-gnu %s -o %t.o
+# RUN: ld.lld -static %t.o -o %tout
+# RUN: llvm-objdump -D %tout | FileCheck %s
+# RUN: llvm-readobj -r %tout | FileCheck %s --check-prefix=RELOC
+
+# CHECK:      Disassembly of section .text:
+# CHECK-NEXT: myfunc:
+# CHECK-NEXT:   210000:
+
+# CHECK:      main:
+# adrp x8, 0x230000, 0x230000 == address in .got
+# CHECK-NEXT:   210004: {{.*}} adrp    x8, #131072
+# CHECK-NEXT:   210008: {{.*}} ldr     x8, [x8]
+# CHECK-NEXT:   21000c: {{.*}} ret
+
+# CHECK:      Disassembly of section .plt:
+# CHECK-NEXT: .plt:
+# adrp x16, 0x220000, 0x220000 == address in .got.plt
+# CHECK-NEXT:   210010: {{.*}} adrp    x16, #65536
+# CHECK-NEXT:   210014: {{.*}} ldr     x17, [x16]
+# CHECK-NEXT:   210018: {{.*}} add     x16, x16, #0
+# CHECK-NEXT:   21001c: {{.*}} br      x17
+
+# CHECK:      Disassembly of section .got.plt:
+# CHECK-NEXT: .got.plt:
+# CHECK-NEXT:   220000:
+
+# CHECK:      Disassembly of section .got:
+# CHECK-NEXT: .got:
+# 0x210010 == address in .plt
+# CHECK-NEXT:   230000: 10 00 21 00
+
+# RELOC:      Relocations [
+# RELOC-NEXT:   Section {{.*}} .rela.plt {
+# RELOC-NEXT:     0x220000 R_AARCH64_IRELATIVE - 0x210000
+# RELOC-NEXT:   }
+# RELOC-NEXT: ]
+
+.text
+.globl myfunc
+.type myfunc,@gnu_indirect_function
+myfunc:
+ ret
+
+.text
+.globl main
+.type main,@function
+main:
+ adrp x8, :got:myfunc
+ ldr  x8, [x8, :got_lo12:myfunc]
+ ret