Page MenuHomePhabricator

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

Authored by peter.smith on Jan 27 2020, 7:03 AM.

Details

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

Diff Detail

Event Timeline

peter.smith created this revision.Jan 27 2020, 7:03 AM
nickdesaulniers accepted this revision.Jan 27 2020, 9:22 AM

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
11

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

This revision is now accepted and ready to land.Jan 27 2020, 9:22 AM

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 added inline comments.Jan 27 2020, 10:08 AM
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 marked 3 inline comments as done.Jan 27 2020, 10:29 AM
peter.smith added inline comments.
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 marked an inline comment as done.

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

MaskRay accepted this revision.Jan 27 2020, 10:34 AM
MaskRay added inline comments.
lld/test/ELF/arm-thumb-interwork-ifunc.s
35

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

MaskRay added inline comments.Jan 27 2020, 10:35 AM
lld/test/ELF/arm-thumb-interwork-notfunc.s
24

all -> All

MaskRay added inline comments.Jan 27 2020, 10:40 AM
lld/test/ELF/arm-thumb-interwork-ifunc.s
7

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.
    }
  }

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

grimar added inline comments.Jan 28 2020, 1:01 AM
lld/ELF/Arch/ARM.cpp
267

Unrelated?

peter.smith marked an inline comment as done.Jan 28 2020, 1:36 AM
peter.smith added inline comments.
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 added inline comments.Jan 28 2020, 1:42 AM
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 marked an inline comment as done.Jan 28 2020, 1:56 AM
peter.smith added inline comments.
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 added inline comments.Jan 28 2020, 2:18 AM
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.

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

peter.smith marked an inline comment as done.Jan 28 2020, 3:28 AM
peter.smith added inline comments.
lld/ELF/Arch/ARM.cpp
267

Thanks, will split out and commit separately as NFC.

This revision was automatically updated to reflect the committed changes.

@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.

This comment was removed by MaskRay.
hans added a comment.Jan 29 2020, 12:28 PM

@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.