[PATCH] D70637: [ELF][AArch64] Support R_AARCH64_{CALL26,JUMP26} range extension thunks with addends

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In D70637#1759352 <https://reviews.llvm.org/D70637#1759352>, @MaskRay wrote:

> > Can you elaborate? My understanding is that needsThunk() and inBranchRange() assume all Arm branches to have an immediate (which becomes the relocation addend) of -8 and Thumb branches to have -4. This cancels the PC-bias of 8-bytes in Arm and 4-bytes in Thumb.
> > 
> > This is my current thinking. If the addend from the branch immediate, whether read from the instruction, or assumed to be -8 for an Arm branch or -4 for a Thumb branch, is passed in to needsThunk() I think that we could do:
> > 
> > src = branchAddr +  (Is Thumb branch then + 4, else +8) 
> >  dst = (expr == R_PLT_PC) ? s.getPltVA(addend) : s.getVA(addend);
>
> Created D70690 <https://reviews.llvm.org/D70690> as a prerequisite change to make this patch affect ARM even less.
>
> The update in `test/ELF/arm-thumb-mix-range-thunk-os.s` is related to the following diff:
>
>   if (auto *d = dyn_cast<Defined>(rel.sym))
>     if (!d->isInPlt() && d->section)
>       thunkVec = &thunkedSymbolsBySectionAndAddend[{
>           {d->section->repl, d->value}, rel.addend}];  // If I use 0 instead of rel.addend,
>   if (!thunkVec)
>     thunkVec = &thunkedSymbols[{rel.sym, rel.addend}]; // If I use 0 instead of rel.addend,  test/ELF/arm-thumb-mix-range-thunk-os.s will not need an update.
>   
>
> The R_ARM_THM_CALL in tfunc33 causes creation of a Thumb thunk. The R_ARM_CALL in tfunc34 have different implicit addends, so making `rel.addend` part of the key will create an ARM thunk, rather than reuse the existing Thumb thunk. If reusing the thunk is the intended behavior, I'll change it to
>
>   // This ternary expression makes the code less general.
>   int64_t addend = config->emachine == EM_ARM ? 0 : rel.addend;
>   
>   if (auto *d = dyn_cast<Defined>(rel.sym))
>     if (!d->isInPlt() && d->section)
>       thunkVec = &thunkedSymbolsBySectionAndAddend[{
>           {d->section->repl, d->value}, addend}];
>   if (!thunkVec)
>     thunkVec = &thunkedSymbols[{rel.sym, addend}];
>   
>
> can keep the test unchanged.


Yes it is intentional, an Arm or Thumb BL instruction can be changed into a BLX instruction that changes state, so it is possible for both an Arm and Thumb relocation to share the same Thunk.

Instead of

  // This ternary expression makes the code less general.
  int64_t addend = config->emachine == EM_ARM ? 0 : rel.addend;

May I suggest

  int64_t addend = rel.addend + getPCBias(type, rel.addend);

This could be virtual function for Target, but could just be static function that in effect cancels -4 or -8 to 0 on Arm, but still accounts for non -4 or -8 addends. The body would be something like:

    if (Config->EMachine != EM_ARM)
      return 0;
    switch (type) {
    case R_ARM_THM_JUMP19:
    case R_ARM_THM_JUMP24:
    case R_ARM_THM_CALL:
      return 4;
    default:
      return 8;
    }
  }

This is more code, but it is a bit more general.

>> I would like to fix this but it will mean adding Symbol, or at least the result of S.isFunc() as a parameter to relocateOne(). If that doesn't sound too objectionable I'd be happy to work on it.
> 
> I support that we add Symbol to the parameter list of relocateOne. This can improve `reportRangeError` messages as well. Right now we don't report which symbol has the problem. With the Symbol information the message can be more specific.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70637/new/

https://reviews.llvm.org/D70637