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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

> 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