[ELF] - Fix R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX when target is IFUNC.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=38074. This simple reproducible is below.

Imagine the following code:

user.cpp:

extern "C" int myfunc();

int main() {
 int (*p)()  = &myfunc;

 return p();
}

library.cpp:

int myfunc2() {
   return 2;
}

extern "C" int myfunc();
__asm__ (".type myfunc, @gnu_indirect_function");

typeof(myfunc) * myfunc_dispatch (void) __asm__ ("myfunc");
typeof(myfunc) * myfunc_dispatch (void)  {
  return myfunc2;
}

If you run the application linked with gold, the output will be correct:

> clang library.o user.o -fPIC -static -o test
> ./test 
> echo $?
2

The application linked with LLD (-fuse-ld=lld) shows 32 for me instead.

The issue is that when calling a function, for example,
LLD generates a .got entry that points to the IFUNC resolver function when
instead, it should use the PLT entries properly for handling the IFUNC.

So we should create a got entry that points to PLT entry, which itself loads the value from
.got.plt, relocated with R_X86_64_IRELATIVE to make things work. Patch do that.

[peter.smith]

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

I haven't found out why as yet. I'll do some digging to see if there is anything obvious that we are missing. If you have access to a X86 Linux box this is reproducible with a GCC aarch64-linux-gnu cross compilation toolchain and the linux user mode emulator qemu-aarch64.

test/ELF/gotpc-relax.s

26

my git diff is showing some trailing whitespace characters on the lines ending with 0x203000

Not a massive problem, but worth removing.

[grimar]

In D54145#1288739, @peter.smith wrote:

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

I haven't found out why as yet. I'll do some digging to see if there is anything obvious that we are missing. If you have access to a X86 Linux box this is reproducible with a GCC aarch64-linux-gnu cross compilation toolchain and the linux user mode emulator qemu-aarch64.

Thanks! My quick guess is that perhaps some new Expr is passed to toPlt(RelExpr Expr) for AArch64 case and hence condition if (needsPlt(Expr) && !Sym.isInPlt()) is not satisfied.
I'll try to take a look at what is going on there during this week.

[peter.smith]

In D54145#1288767, @grimar wrote:

In D54145#1288739, @peter.smith wrote:

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

I haven't found out why as yet. I'll do some digging to see if there is anything obvious that we are missing. If you have access to a X86 Linux box this is reproducible with a GCC aarch64-linux-gnu cross compilation toolchain and the linux user mode emulator qemu-aarch64.

Thanks! My quick guess is that perhaps some new Expr is passed to toPlt(RelExpr Expr) for AArch64 case and hence condition if (needsPlt(Expr) && !Sym.isInPlt()) is not satisfied.
I'll try to take a look at what is going on there during this week.

Yes I agree. It looks like AArch64 uses R_AARCH64_ADR_GOT_PAGE which translates to R_GOT_PAGE_PC. Whereas on ARM we have R_ARM_GOT_PREL which translates to R_GOT_PC. So at a first approximation it looks like adding support for R_GOT_PAGE_PC_PLT might work.

[grimar]

In D54145#1288739, @peter.smith wrote:

I've tested this out on X86, Arm and AArch64 and it works for X86 and Arm. Unfortunately, it doesn't seem to work with AArch64 yet. I don't see any R_AARCH64_IRELATIVE relocations.

I see the R_AARCH64_IRELATIVE for some reason. Am I doing something differently?

I haven't found out why as yet. I'll do some digging to see if there is anything obvious that we are missing. If you have access to a X86 Linux box this is reproducible with a GCC aarch64-linux-gnu cross compilation toolchain and the linux user mode emulator qemu-aarch64.

It seems there is something different on AArch64 perhaps. If I take user.cpp and library.cpp files from description
and clang version 8.0.0 (trunk 346310) + LLD 8.0.0 (trunk 345820) (without this patch applied) I have:

clang -target aarch64-linux-gnueabi user.cpp -c -o user.o
clang -target aarch64-linux-gnueabi library.cpp -c -o library.o
clang -target aarch64-linux-gnueabi user.o library.o -static -fuse-ld=lld -o out
qemu-aarch64 -L /usr/aarch64-linux-gnu out
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault (core dumped)

And output has IRELATIVE:

readelf -r out -W

Relocation section '.rela.plt' at offset 0x238 contains 1 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
00000000002819f0  0000000000000408 R_AARCH64_IRELATIVE                       2201c8

If I use gcc (gcc version 5.4.0 20160609 (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9)
to produce objects instead, a signal code is different, but the situation is about the same in general:

aarch64-linux-gnu-gcc library.cpp -c -o library.o
aarch64-linux-gnu-gcc user.cpp -c -o user.o
~/LLVM/build_lldb/bin/clang -target aarch64-linux-gnueabi user.o library.o -static -fuse-ld=lld -o out

 qemu-aarch64 -L /usr/aarch64-linux-gnu out
�S��
��[����#qemu: uncaught target signal 6 (Aborted) - core dumped
Aborted (core dumped)

readelf -r out -W
Relocation section '.rela.plt' at offset 0x238 contains 1 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
00000000002819f0  0000000000000408 R_AARCH64_IRELATIVE                       2201bc

If I stop using LLD (so, use default /usr/lib/gcc-cross/aarch64-linux-gnu/5.4.0/../../../../aarch64-linux-gnu/bin/ld -vGNU ld (GNU Binutils for Ubuntu) 2.28)
it works fine.

So I am observing R_AARCH64_IRELATIVE is present in both cases, but it crashes even without this patch.

I would start tracking AArch64 issue status separately probably and I think we can land this patch first.

[peter.smith]

In my example I was using -fpic , without it then we get a different relocation type R_AARCH64_ADR_PREL_PG_HI21 (with -fpic it is R_AARCH64_ADR_GOT_PAGE), I suspect that is why I didn't see the IRELATIVE relocation. I don't mind if we keep this patch to be specific to non-AArch64, but if we do the we either shouldn't close the PR until we've made it work, or raise another PR to make sure we don't forget about it.

[grimar]

In D54145#1291565, @peter.smith wrote:

In my example I was using -fpic , without it then we get a different relocation type R_AARCH64_ADR_PREL_PG_HI21 (with -fpic it is R_AARCH64_ADR_GOT_PAGE), I suspect that is why I didn't see the IRELATIVE relocation. I don't mind if we keep this patch to be specific to non-AArch64, but if we do the we either shouldn't close the PR until we've made it work, or raise another PR to make sure we don't forget about it.

Ah, my mistake, sorry. I did not use -fPIC because of wrong assumption (it did not segfault with fPIC and I started to try to catch the crash, though it seems to be a different issue).
So, you right, with -fPIC I do not see the relocation and was able to reproduce the issue mentioned in description finally.
I'll try to fix it then.

(and sure we should not close the PR without a fix for AArch64.)

[ruiu]

Where do you create .got.iplt?

[grimar]

In D54145#1296604, @ruiu wrote:

Where do you create .got.iplt?

After converting expression to PLT expression (Expr = toPlt(Expr);):
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L993

code flow goes inside if (needsPlt(Expr) && !Sym.isInPlt()) {:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L1018

And adds an entry to In.IgotPlt inside addPltEntry.

[ruiu]

Oh okay, that was just a typo. (Your patch description contains a mention about .got.iplt)

[grimar]

In D54145#1296682, @ruiu wrote:

Oh okay, that was just a typo. (Your patch description contains a mention about .got.iplt)

Right. I thought In.IgotPlt has .got.iplt input section name. But it is .got.plt atm, just like the output.

[peter.smith]

My understanding is that the .igot, .igot.plt come from the bfd linker script.

.got            : { *(.got) *(.igot) }
. = DATA_SEGMENT_RELRO_END (SIZEOF (.got.plt) >= 24 ? 24 : 0, .);
.got.plt        : { *(.got.plt)  *(.igot.plt) }

As you can see they are logically part of the .got with the .igot used to make sure the ifunc resolver entries happen last (and are resolved last).

[grimar]

In D54145#1296727, @peter.smith wrote:

My understanding is that the .igot, .igot.plt come from the bfd linker script.

.got            : { *(.got) *(.igot) }
. = DATA_SEGMENT_RELRO_END (SIZEOF (.got.plt) >= 24 ? 24 : 0, .);
.got.plt        : { *(.got.plt)  *(.igot.plt) }

As you can see they are logically part of the .got with the .igot used to make sure the ifunc resolver entries happen last (and are resolved last).

Yeah, look like I saw them there. I thought we do the same though.
i.e. that means our script can not discard/move igot.plt now it seems since we use the `got.plt' for both on x64. Does not seem it is useful though.

[grimar]

• Removed trailing whitespace from the test case.
• Rebased.

[grimar]

Ping.
Rui, can we land it?

[grimar]

Ping.

[grimar]

Ping.

[grimar]

Ping #4

[ruiu]

ELF/Relocations.h

57

Can you add a comment as to what this enum value represents?

[grimar]

• Addressed review comment.

Note:
Russia starts celebrating the NY, the day after NY, the day after the day after NY and so on. I'll be back on 9 January.

[grimar]

ELF/Relocations.h

57

Done.

[ruiu]

LGTM

[MaskRay]

@grimar D57371 fixed the "R_X86_64_REX_GOTPCRELX referencing ifunc should not be relaxed" bug, via the following line:

if (!Sym.IsPreemptible && !Sym.isGnuIFunc()) {

Target::adjustRelaxExpr is not called for a non-preemptible ifunc. I think this patch is not needed.

clang -fuse-ld=lld -Wa,-mrelax-relocations=yes a.c b.c -fPIC -static -o a

I know one issue, which is unrelated to this patch: config->hasDynSymTab = !sharedFiles.empty() || config->isPic || config->exportDynamic;
This condition does not take dynamic relocations into account.

So, clang -fuse-ld=lld -Wa,-mrelax-relocations=yes a.c b.c -fPIC -static -o a -z ifunc-noplt -z notext does not generate a dynamic R_X86_64_REX_GOTPCRELX.
Adding -Wl,--export-dynamic will produce a dynamic R_X86_64_REX_GOTPCRELX.
config->hasDynSymTab can affect Symbol::includeInDynSym, which can be called before Writer. So I don't know an easy fix.

@markj

[grimar]

In D54145#1868678, @MaskRay wrote:

@grimar D57371 fixed the "R_X86_64_REX_GOTPCRELX referencing ifunc should not be relaxed" bug, via the following line:

if (!Sym.IsPreemptible && !Sym.isGnuIFunc()) {

Target::adjustRelaxExpr is not called for a non-preemptible ifunc. I think this patch is not needed.

I think you are right. Seems I did not realise that the last comment from Peter Wu (https://bugs.llvm.org/show_bug.cgi?id=38074#c9)
is actually a one+ year old when reopened the bug ~2 weeks ago.

Thank you for spotting!

[markj]

In D54145#1868678, @MaskRay wrote:

I know one issue, which is unrelated to this patch: config->hasDynSymTab = !sharedFiles.empty() || config->isPic || config->exportDynamic;
This condition does not take dynamic relocations into account.

So, clang -fuse-ld=lld -Wa,-mrelax-relocations=yes a.c b.c -fPIC -static -o a -z ifunc-noplt -z notext does not generate a dynamic R_X86_64_REX_GOTPCRELX.
Adding -Wl,--export-dynamic will produce a dynamic R_X86_64_REX_GOTPCRELX.
config->hasDynSymTab can affect Symbol::includeInDynSym, which can be called before Writer. So I don't know an easy fix.

I can't see an easy solution other than to further constrain the use of -zifunc-noplt to require !-static || --export-dynamic. This would be fine for FreeBSD's use of the option.

ELF/InputSection.cpp

static uint64_t getRelocTargetVA(const InputFile *File, RelType Type, int64_t A,
   case R_GOT_OFF:
   case R_RELAX_TLS_GD_TO_IE_GOT_OFF:
     return Sym.getGotOffset() + A;
   case R_AARCH64_GOT_PAGE_PC:
   case R_AARCH64_GOT_PAGE_PC_PLT:
   case R_AARCH64_RELAX_TLS_GD_TO_IE_PAGE_PC:
     return getAArch64Page(Sym.getGotVA() + A) - getAArch64Page(P);
   case R_GOT_PC:
+  case R_GOT_PC_PLT:
   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:
     return Sym.getVA(A) - In.MipsGot->getGp(File);
   case R_MIPS_GOT_GP:
     return In.MipsGot->getGp(File) + A;

ELF/Relocations.h

enum RelExpr {
   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_PC,
+  // The expression is used for IFUNC support. Used to describe the
+  // PC-relative address of the GOT entry containing redirection to the IPLT.
+  R_GOT_PC_PLT,
   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,
   R_MIPS_GOT_OFF32,

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_AARCH64_PLT_PAGE_PC,
-                        R_GOT_PLT, R_AARCH64_GOT_PAGE_PC_PLT>(Expr);
+                        R_GOT_PLT, R_GOT_PC_PLT, R_AARCH64_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_AARCH64_GOT_PAGE_PC,
-                        R_AARCH64_GOT_PAGE_PC_PLT, R_GOT_PC, R_GOT_FROM_END,
-                        R_GOT_PLT>(Expr);
+                        R_AARCH64_GOT_PAGE_PC_PLT, R_GOT_PC, R_GOT_PC_PLT,
+                        R_GOT_FROM_END, R_GOT_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_AARCH64_PAGE_PC,
                         R_RELAX_GOT_PC>(Expr);
static RelExpr toPlt(RelExpr Expr) {
   case R_AARCH64_PAGE_PC:
     return R_AARCH64_PLT_PAGE_PC;
   case R_AARCH64_GOT_PAGE_PC:
     return R_AARCH64_GOT_PAGE_PC_PLT;
   case R_ABS:
     return R_PLT;
   case R_GOT:
     return R_GOT_PLT;
+  case R_GOT_PC:
+    return R_GOT_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.

test/ELF/gotpc-relax.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -relax-relocations -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: ld.lld %t.o -o %t1
+# RUN: llvm-objdump -D %t1 | FileCheck --check-prefix=DISASM %s
 # RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=RELOC %s
-# RUN: llvm-objdump -d %t1 | FileCheck --check-prefix=DISASM %s
-
-## There is no relocations.
-# RELOC:    Relocations [
-# RELOC:    ]
 
 # 0x201003 + 7 - 10 = 0x201000
 # 0x20100a + 7 - 17 = 0x201000
 # 0x201011 + 7 - 23 = 0x201001
 # 0x201018 + 7 - 30 = 0x201001
 # DISASM:      Disassembly of section .text:
 # DISASM-NEXT: foo:
 # DISASM-NEXT:   201000: 90 nop
 # DISASM:      hid:
 # DISASM-NEXT:   201001: 90 nop
 # DISASM:      ifunc:
 # DISASM-NEXT:   201002: c3 retq
 # DISASM:      _start:
 # DISASM-NEXT: leaq -10(%rip), %rax
 # DISASM-NEXT: leaq -17(%rip), %rax
 # DISASM-NEXT: leaq -23(%rip), %rax
 # DISASM-NEXT: leaq -30(%rip), %rax
-# DISASM-NEXT: movq 4058(%rip), %rax
-# DISASM-NEXT: movq 4051(%rip), %rax
+# IFUNC are not relaxable and accessed via the corresponding PLT entry.
+# So we should generate a .got entry that points to a PLT entry that loads an entry
+# from the .got.plt which is subject to an R_X86_64_IRELATIV relocation.
+# 0x20101f + 8154 + 7 = 0x203000
+# DISASM-NEXT:  20101f: {{.*}} movq 8154(%rip), %rax
+# DISASM-NEXT:  201026: {{.*}} movq 8147(%rip), %rax
 # DISASM-NEXT: leaq -52(%rip), %rax
 # DISASM-NEXT: leaq -59(%rip), %rax
 # DISASM-NEXT: leaq -65(%rip), %rax
 # DISASM-NEXT: leaq -72(%rip), %rax
-# DISASM-NEXT: movq 4016(%rip), %rax
-# DISASM-NEXT: movq 4009(%rip), %rax
+# DISASM-NEXT:  201049: {{.*}} movq 8112(%rip), %rax
+# DISASM-NEXT:  201050: {{.*}} movq 8105(%rip), %rax
 # DISASM-NEXT: callq -93 <foo>
 # DISASM-NEXT: callq -99 <foo>
 # DISASM-NEXT: callq -104 <hid>
 # DISASM-NEXT: callq -110 <hid>
-# DISASM-NEXT: callq *3979(%rip)
-# DISASM-NEXT: callq *3973(%rip)
+# 0x20101f + 8154 + 6 = 0x203000
+# DISASM-NEXT:  20106f: {{.*}} callq *8075(%rip)
+# DISASM-NEXT:  201075: {{.*}} callq *8069(%rip)
 # DISASM-NEXT: jmp   -128 <foo>
 # DISASM-NEXT: nop
 # DISASM-NEXT: jmp   -134 <foo>
 # DISASM-NEXT: nop
 # DISASM-NEXT: jmp   -139 <hid>
 # DISASM-NEXT: nop
 # DISASM-NEXT: jmp   -145 <hid>
 # DISASM-NEXT: nop
-# DISASM-NEXT: jmpq  *3943(%rip)
-# DISASM-NEXT: jmpq  *3937(%rip)
+# 201093 + 8039 + 6 = 0x203000
+# DISASM-NEXT:  201093: {{.*}} jmpq *8039(%rip)
+# DISASM-NEXT:  201099: {{.*}} jmpq *8033(%rip)
+
+# DISASM:      Disassembly of section .plt:
+# DISASM-NEXT: .plt:
+# 0x2010a0 + 3930 + 6 = 0x202000 = address of .got.plt entry.
+# DISASM-NEXT:   2010a0: {{.*}} jmpq *3930(%rip)
+
+# We have a R_X86_64_IRELATIVE relocation for this entry.
+# DISASM:      Disassembly of section .got.plt:
+# DISASM-NEXT: .got.plt:
+# DISASM-NEXT:   202000:
+
+## GOT[0] contains 0x2010a0. It is an address of PLT entry for ifunc.
+# DISASM:      Disassembly of section .got:
+# DISASM-NEXT: .got:
+# DISASM-NEXT:   203000: a0
+# DISASM-NEXT:   203001: 10 20
+# DISASM-NEXT:   203003: 00
+
+## We have one relocation in IPLT for `ifunc`
+# RELOC:      Relocations [
+# RELOC-NEXT:   Section {{.*}} .rela.plt {
+# RELOC-NEXT:     0x202000 R_X86_64_IRELATIVE - 0x201002
+# RELOC-NEXT:   }
+# RELOC-NEXT: ]
 
 .text
 .globl foo
 .type foo, @function
 foo:
  nop
 
 .globl hid