[PATCH] D108897: [lld/mac] Set branchRange a bit more carefully

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: lld/MachO/ConcatOutputSection.cpp:178
   uint64_t stubsInRangeVA = isecEnd + maxPotentialThunks * target->thunkSize +
                             in.stubs->getSize() - branchRange;
   log("thunks = " + std::to_string(thunkMap.size()) +
----------------
int3 wrote:
> I think the size of the stubs table is factored in here
The size of the stubs table yes, but not the size of of thunks needed to reach it as far as I understand. For example:

```
f:
  b stubFun40
  b stubFun39
  ret
g:
  .fill X MB
  ret
stubTable:
  ...40 entries stubFun1, stubFun2, ...
```

Where X is chosen so that you can jump until stubFun39 from the start of f without a thunk. That means we'll finalize f and g, and after g is finalized, call this estimateStubsInRange() function, and then process the relocations in f. `maxPotentialThunks` is 0 since there are only stub calls in this program. So we set `stubsInRangeVA` at the address of the 2nd `b` in f. But now we process the first `b`, realize it needs a thunk (since it's before `stubsInRangeVA`), which means we insert that thunk after `g` before the stubs table. But that shifts the stub table 12 bytes further back, which means the b to stubFun39 is now no longer in range, even though according to `stubsInRangeVA` still thinks it is.

(Right?)

>From what I understand, that's what `maxPotentialThunks` is designed to prevent, but it doesn't count stub calls.


================
Comment at: lld/MachO/ConcatOutputSection.cpp:258
       uint64_t lowVA = branchRange < callVA ? callVA - branchRange : 0;
-      uint64_t highVA = callVA + branchRange;
+      uint64_t highVA = callVA + branchRange - 4;
       // Calculate our call referent address
----------------
int3 wrote:
> thakis wrote:
> > tschuett wrote:
> > > thakis wrote:
> > > > (See also `AArch64::inBranchRange` in lld/ELF/Arch/AArch64.cpp)
> > > Do you have something with more information than a 4. Maybe sizeof() or a named  constant?
> > It's really just `1 * 4` in simplified. The range of a two's complement number is -2^n..(2^n - 1), and the call offset is multiplied by 4 to get more range (since function start addresses are required to be 4-aligned on arm64).
> the literal constant thing doesn't bother me, but I'm bothered that this is arm64 logic leaking out into generic ConcatOutputSection code. I guess the issue is that the max forward branch range is different from the max backward branch range. How about having `forwardBranchRange` and `backwardBranchRange` instead?
Oh, you mean because it's a 4 instead of a 1? That's a good point.

But it's also a bit awkward since it suggests we support a setup where forward and backward jumps support meaningfully different ranges, and we need to check min() of both branch ranges etc. But maybe it's good to be explicit about that.

Done.


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

https://reviews.llvm.org/D108897