[ELF][ARM] Add getPCBias()

[MaskRay]

Summary

ThunkCreator::getThunk and ThunkCreator::normalizeExistingThunk
currently assume that the implicit addends are -8 for ARM and -4 for
Thumb. In D70637, ThunkCreator::getThunk will need to take care of the
relocation addend explicitly.

Add the utility function getPCBias() as a prerequisite so that the getThunk change in D70637
can be more general.

[grimar]

lld/ELF/Arch/ARM.cpp

391 ↗

(On Diff #230952)

nit: I guess using if might be a bit nicer and shorter here.

[peter.smith]

Is there any reason not to have just one function inBranchRange() and assume that it needs to take care of the relocation addend explicitly, that way the PC Bias could be limited to the ARM implementation of inBranchRange() and inBranchRangeBeforePCBias wouldn't be needed?

The only other call sites that I could find that would need to be adjusted are:
In Relocations.cpp the relocation addend could be passed to getISDThunkSec (line 1634) so that it can be passed through to the two calls to target->inBranchRange() in there.
In ARMErrataFix.cpp we are generating the branch instruction (it is always Thumb and its addend is hard-coded to -4) so we could easily account for this at the call site.
needsThunk() would either need the addend to pass through to inBranchRange(). In D70637 you are passing in the addend, in this patch it might be simple enough to say if Arm Relocation pass in -8, if thumb pass in -4.

It maybe that across this patch and D70637 we can convert all the callsites so that there is just inBranchRange() which expects the relocation addend to have been accounted for in dst.

[MaskRay]

In D70690#1759922, @peter.smith wrote:

Is there any reason not to have just one function inBranchRange() and assume that it needs to take care of the relocation addend explicitly, that way the PC Bias could be limited to the ARM implementation of inBranchRange() and inBranchRangeBeforePCBias wouldn't be needed?

If we require that the call sites of inBranchRange() account for both the addend and the PC Bias (e.g. target->inBranchRange(rel.type, src, t->getThunkTargetSym()->getVA(rel.addend) + getPCBias(rel.type))), I find that we just need to adjust ThunkCreator::getThunk and ThunkCreator::normalizeExistingThunk.

The only other call sites that I could find that would need to be adjusted are:
In Relocations.cpp the relocation addend could be passed to getISDThunkSec (line 1634) so that it can be passed through to the two calls to target->inBranchRange() in there.
In ARMErrataFix.cpp we are generating the branch instruction (it is always Thumb and its addend is hard-coded to -4) so we could easily account for this at the call site.
needsThunk() would either need the addend to pass through to inBranchRange(). In D70637 you are passing in the addend, in this patch it might be simple enough to say if Arm Relocation pass in -8, if thumb pass in -4.

The two places will not be needed to be adjusted.

It maybe that across this patch and D70637 we can convert all the callsites so that there is just inBranchRange() which expects the relocation addend to have been accounted for in dst.

[MaskRay]

TargetInfo::inBranchRangeBeforePCBias -> getPCBias()

[MaskRay]

Change the parameter type of type to RelType

[peter.smith]

LGTM. I think this looks a lot cleaner thanks. I also agree that the getISDThunk call sites don't need modifying as at this point the Thunk is our destination and we don't want an addend from the thunk symbol.

lld/ELF/Relocations.cpp

static bool isThunkSectionCompatible(InputSection *source,
                                      SectionBase *target) {
   // We can't reuse thunks in different loadable partitions because they might
   // not be loaded. But partition 1 (the main partition) will always be loaded.
   if (source->partition != target->partition)
     return target->partition == 1;
   return true;
 }
 
+static int64_t getPCBias(RelType type) {
+  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;
+  }
+}
+
 std::pair<Thunk *, bool> ThunkCreator::getThunk(InputSection *isec,
                                                 Relocation &rel, uint64_t src) {
   std::vector<Thunk *> *thunkVec = nullptr;
 
   // We use (section, offset) pair to find the thunk position if possible so
   // that we create only one thunk for aliased symbols or ICFed sections.
   if (auto *d = dyn_cast<Defined>(rel.sym))
     if (!d->isInPlt() && d->section)
       thunkVec = &thunkedSymbolsBySection[{d->section->repl, d->value}];
   if (!thunkVec)
     thunkVec = &thunkedSymbols[rel.sym];
 
   // Check existing Thunks for Sym to see if they can be reused
   for (Thunk *t : *thunkVec)
     if (isThunkSectionCompatible(isec, t->getThunkTargetSym()->section) &&
         t->isCompatibleWith(*isec, rel) &&
-        target->inBranchRange(rel.type, src, t->getThunkTargetSym()->getVA()))
+        target->inBranchRange(rel.type, src,
+                              t->getThunkTargetSym()->getVA(rel.addend) +
+                                  getPCBias(rel.type)))
       return std::make_pair(t, false);
 
   // No existing compatible Thunk in range, create a new one
   Thunk *t = addThunk(*isec, rel);
   thunkVec->push_back(t);
   return std::make_pair(t, true);
 }
 
 // Return true if the relocation target is an in range Thunk.
 // Return false if the relocation is not to a Thunk. If the relocation target
 // was originally to a Thunk, but is no longer in range we revert the
 // relocation back to its original non-Thunk target.
 bool ThunkCreator::normalizeExistingThunk(Relocation &rel, uint64_t src) {
   if (Thunk *t = thunks.lookup(rel.sym)) {
-    if (target->inBranchRange(rel.type, src, rel.sym->getVA()))
+    if (target->inBranchRange(rel.type, src,
+                              rel.sym->getVA(rel.addend) + getPCBias(rel.type)))
       return true;
     rel.sym = &t->destination;
     if (rel.sym->isInPlt())
       rel.expr = toPlt(rel.expr);
   }
   return false;
 }