[LLD][ELF][ARM] Do not insert interworking thunks for non STT_FUNC symbols

[peter.smith]

Summary

ELF for the ARM architecture requires linkers to provide interworking for symbols that are of type STT_FUNC. Interworking for other symbols must be encoded directly in the object file. LLD was always providing interworking, regardless of the symbol type, this breaks some programs that have branches from Thumb state targeting STT_NOTYPE symbols that have bit 0 clear, but they are in fact internal labels in a Thumb function. LLD treats these symbols as ARM and inserts a transition to Arm.

This fixes the problem for in range branches, R_ARM_JUMP24, R_ARM_THM_JUMP24 and R_ARM_THM_JUMP19. This is expected to be the vast majority of problem cases as branching to an internal label close to the function.

There is at least one follow up patch required.

• R_ARM_CALL and R_ARM_THM_CALL may do interworking via BL/BLX substitution. A fix for this is dependent on D73254

In theory range-extension thunks can be altered to not change state when the symbol type is not STT_FUNC. I think this is a corner case on a corner case, but will need to check if BFD does it.

Fixes (part of) https://github.com/ClangBuiltLinux/linux/issues/773
ELF for the Arm Architecture https://static.docs.arm.com/ihi0044/g/aaelf32.pdf

[nickdesaulniers]

I've tested this patch, and with it applied, I can successfully boot an LLD-linked thumb2 kernel. Big thanks @peter.smith !!

lld/test/ELF/arm-thumb-interwork-notfunc.s

10

how about adding a .type func_with_notype, %notype and again for thumb_func_with_notype (just to be explicit about the symbol type)?

[peter.smith]

Uploaded revision with explicit STT_NOTYPE symbol. Will wait till tomorrow to commit so I can handle any failures (just about to leave the office).

[MaskRay]

lld/ELF/Arch/ARM.cpp

281

== 1 can be deleted. Can s be STT_GNU_IFUNC (isGnuIFunc())?

292

Can s be STT_GNU_IFUNC?

[peter.smith]

lld/ELF/Arch/ARM.cpp

281

I'll remove the 1, ifunc calls always have expr R_PLT_PC.

292

Ifuncs are always called via the PLT so they use R_PLT_PC. I'll add a test to show this.

[peter.smith]

Removed redundant ==1, added test for branch to ifunc symbol.

[MaskRay]

lld/test/ELF/arm-thumb-interwork-ifunc.s

34 ↗

(On Diff #240637)

The addresses before b/bl/b.w/blx can be omitted.

[MaskRay]

lld/test/ELF/arm-thumb-interwork-notfunc.s

24

all -> All

[MaskRay]

lld/test/ELF/arm-thumb-interwork-ifunc.s

6 ↗

(On Diff #240637)

Non-preemptible ifuncs are called directly, not via a PLT entry.

// Relocations.cpp
  if (!sym.isPreemptible && (!sym.isGnuIFunc() || config->zIfuncNoplt)) {
    if (expr == R_GOT_PC && !isAbsoluteValue(sym)) {
      expr = target->adjustRelaxExpr(type, relocatedAddr, expr);
    } else {
      // 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;
      expr = fromPlt(expr); ///////////////// this optimizes R_PLT_PC to R_PC, but the code path is not exercised for ifunc.
    }
  }

[peter.smith]

Thanks for the comments, I'll fix up those last ones tomorrow.

[grimar]

lld/ELF/Arch/ARM.cpp

267

Unrelated?

[peter.smith]

lld/ELF/Arch/ARM.cpp

267

Assuming this is the:

// If S is an undefined weak symbol and does not have a PLT entry then it
// will be resolved as a branch to the next instruction.
if (s.isUndefWeak() && !s.isInPlt())
  return false;

I think that is still relevant. It would be for cases like:

.weak weakfunction
.type weakfunction, %function
.thumb
b weakfunction
 // we want b weakfunction to resolve to the next instruction if weakfunction is undefined
bx lr

If weakfunction were undefined then it would have a value of the address of the next instruction which would have bit 0 clear. This would look to the code below as if it were an Arm function.

It could be possible to integrate the check into the code below, something like (not tested):

case R_ARM_THM_JUMP24:
    // Source is Thumb, all PLT entries are ARM so interworking is required.
    // Otherwise we need to interwork if STT_FUNC Symbol has bit 0 clear (ARM).
    if (expr == R_PLT_PC || (s.isFunc() && !s.isUndefWeak() && (s.getVA() & 1) == 0))

The early exit is, to me, a bit simpler to understand, but I'd be happy to explore the alternative if you'd prefer, possibly a follow up patch?

[grimar]

lld/ELF/Arch/ARM.cpp

267

I meant the change in the function signature seems to be because of a clang-format :)
No arguments were changed or added.

[peter.smith]

lld/ELF/Arch/ARM.cpp

267

Yes that is true, the function symbol presumably due to an automatic refactoring had overflowed 80 columns so I took the opportunity to fix it. I can split it out into a separate patch if you want, but it seemed sufficiently small to roll it in with this one?

[grimar]

lld/ELF/Arch/ARM.cpp

267

I've noticed this because tried to find what changed in the method signature at first.
If you formatted the same lines you changed then it would be completly fine, but this lines are a bit too far form another changes you have and often such refactorings are committed as separate NFC changes which does not requre a review.
Also this makes the commit history cleaner (and it is much easier to make a git "blame" operation when the history is clean).

So I'd really suggest to commit such refactoring(s) as a NFC change separatelly if you want to do them.

[grimar]

Aside from the nit discussed, this looks probably reasonable change to me, I am not an expert in ARM though.

[peter.smith]

lld/ELF/Arch/ARM.cpp

267

Thanks, will split out and commit separately as NFC.

[nickdesaulniers]

@hans can we please cherry pick this into the llvm-10 release, if it's not too late?

It landed as 4f38ab250ff4680375c4c01db0a88c157093c665. 3238b03c197741207dea8cc3bc3273f74b448460 landed before it, but doesn't cherry-pick cleanly, where as I believe that 4f38ab250ff4680375c4c01db0a88c157093c665 does. 3238b03c197741207dea8cc3bc3273f74b448460 is NFC and not required.

[MaskRay]

This comment was removed by MaskRay.

[hans]

In D73474#1845127, @nickdesaulniers wrote:

@hans can we please cherry pick this into the llvm-10 release, if it's not too late?

It landed as 4f38ab250ff4680375c4c01db0a88c157093c665. 3238b03c197741207dea8cc3bc3273f74b448460 landed before it, but doesn't cherry-pick cleanly, where as I believe that 4f38ab250ff4680375c4c01db0a88c157093c665 does. 3238b03c197741207dea8cc3bc3273f74b448460 is NFC and not required.

Cherry-picked 4f38ab250ff4680375c4c01db0a88c157093c665 as 81d73c6de33b2282f7174bd378699feac69bc5aa. Please let me know if anything else is needed.

lld/ELF/Arch/ARM.cpp

 }
 
 void ARM::addPltSymbols(InputSection &isec, uint64_t off) const {
   addSyntheticLocal("$a", STT_NOTYPE, off, 0, isec);
   addSyntheticLocal("$d", STT_NOTYPE, off + 12, 0, isec);
 }
 
 bool ARM::needsThunk(RelExpr expr, RelType type, const InputFile *file,
-                     uint64_t branchAddr, const Symbol &s, int64_t /*a*/) const {
+                     uint64_t branchAddr, const Symbol &s,
+                     int64_t /*a*/) const {
   // If S is an undefined weak symbol and does not have a PLT entry then it
   // will be resolved as a branch to the next instruction.
   if (s.isUndefWeak() && !s.isInPlt())
     return false;
   // A state change from ARM to Thumb and vice versa must go through an
   // interworking thunk if the relocation type is not R_ARM_CALL or
   // R_ARM_THM_CALL.
   switch (type) {
   case R_ARM_PC24:
   case R_ARM_PLT32:
   case R_ARM_JUMP24:
     // Source is ARM, all PLT entries are ARM so no interworking required.
-    // Otherwise we need to interwork if Symbol has bit 0 set (Thumb).
-    if (expr == R_PC && ((s.getVA() & 1) == 1))
+    // Otherwise we need to interwork if STT_FUNC Symbol has bit 0 set (Thumb).
+    if (s.isFunc() && expr == R_PC && ((s.getVA() & 1) == 1))
       return true;
     LLVM_FALLTHROUGH;
   case R_ARM_CALL: {
     uint64_t dst = (expr == R_PLT_PC) ? s.getPltVA() : s.getVA();
     return !inBranchRange(type, branchAddr, dst);
   }
   case R_ARM_THM_JUMP19:
   case R_ARM_THM_JUMP24:
     // Source is Thumb, all PLT entries are ARM so interworking is required.
-    // Otherwise we need to interwork if Symbol has bit 0 clear (ARM).
-    if (expr == R_PLT_PC || ((s.getVA() & 1) == 0))
+    // Otherwise we need to interwork if STT_FUNC Symbol has bit 0 clear (ARM).
+    if (expr == R_PLT_PC || (s.isFunc() && (s.getVA() & 1) == 0))
       return true;
     LLVM_FALLTHROUGH;
   case R_ARM_THM_CALL: {
     uint64_t dst = (expr == R_PLT_PC) ? s.getPltVA() : s.getVA();
     return !inBranchRange(type, branchAddr, dst);
   }
   }
   return false;

lld/test/ELF/arm-thumb-interwork-notfunc.s

+// REQUIRES: arm
+// RUN: llvm-mc --triple=armv7a-linux-gnueabihf -arm-add-build-attributes -filetype=obj -o %t.o %s
+// RUN: ld.lld %t.o -o %t
+// RUN: llvm-objdump --no-show-raw-insn -d %t | FileCheck %s
+
+.syntax unified
+.section .arm_target, "ax", %progbits
+.balign 4
+.arm
+arm_func_with_notype:
+ bx lr
+
+.section .thumb_target, "ax", %progbits
+.balign 4
+.thumb
+thumb_func_with_notype:
+ bx lr
+
+/// all the symbols that are targets of the branch relocations do not have
+/// type STT_FUNC. LLD should not insert interworking thunks as non STT_FUNC
+/// symbols have no state information, the ABI assumes the user has manually
+/// done the interworking.
+.section .arm_caller, "ax", %progbits
+.balign 4
+.arm
+.global _start
+_start:
+ b .arm_target
+ b arm_func_with_notype
+ b .thumb_target
+ b thumb_func_with_notype
+
+ .section .thumb_caller, "ax", %progbits
+ .thumb
+ .balign 4
+ b.w .arm_target
+ b.w arm_func_with_notype
+ b.w .thumb_target
+ b.w thumb_func_with_notype
+ beq.w .arm_target
+ beq.w arm_func_with_notype
+ beq.w .thumb_target
+ beq.w thumb_func_with_notype
+
+// CHECK:      000110bc _start:
+// CHECK-NEXT: 110bc: b       #-16
+// CHECK-NEXT: 110c0: b       #-20
+// CHECK-NEXT: 110c4: b       #-20
+// CHECK-NEXT: 110c8: b       #-24
+// CHECK:      110cc: b.w     #-28
+// CHECK-NEXT: 110d0: b.w     #-32
+// CHECK-NEXT: 110d4: b.w     #-32
+// CHECK-NEXT: 110d8: b.w     #-36
+// CHECK-NEXT: 110dc: beq.w   #-44
+// CHECK-NEXT: 110e0: beq.w   #-48
+// CHECK-NEXT: 110e4: beq.w   #-48
+// CHECK-NEXT: 110e8: beq.w   #-52

lld/test/ELF/arm-thunk-edgecase.s

 // RUN: llvm-objdump -triple=armv7a-none-linux-gnueabi -d %tarm_to_thumb.so | FileCheck -check-prefix=ARM-TO-THUMB %s
 // RUN: llvm-objdump -triple=thumbv7a-none-linux-gnueabi -d %tthumb_to_arm.so | FileCheck -check-prefix=THUMB-TO-ARM %s
 
 .syntax unified
 
 .arm
 .section .text_armfunc, "ax", %progbits
 .globl armfunc
+.type armfunc, %function
 armfunc:
   b thumbfunc
 
 .thumb
 .section .text_thumbfunc, "ax", %progbits
 .globl thumbfunc
 .thumb_func
 thumbfunc: