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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lld/MachO/Arch/ARM64.cpp:137-138
   thunkSize = sizeof(thunkCode);
-  branchRange = maxIntN(28) - thunkSize;
+  backwardBranchRange = 128 * 1024 * 1024;
+  forwardBranchRange = backwardBranchRange - 4;
   stubHelperHeaderSize = sizeof(stubHelperHeaderCode);
----------------
would be nice to have a comment explaining the `- 4`


================
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()) +
----------------
thakis wrote:
> 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.
oh gotcha. yeah, I think you're right


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

https://reviews.llvm.org/D108897