[PATCH] D116705: [lld-macho] Increase slops to prevent thunk out of range

[Vincent Lee via Phabricator via llvm-commits]

thevinster added a subscriber: thakis.
thevinster added inline comments.


================
Comment at: lld/MachO/ConcatOutputSection.cpp:247
     // grows. So leave room for a bunch of thunks.
-    unsigned slop = 100 * thunkSize;
+    unsigned slop = 200 * thunkSize;
     while (finalIdx < endIdx && isecAddr + inputs[finalIdx]->getSize() <
----------------
smeenai wrote:
> oontvoo wrote:
> > Where is this number from?
> > (would have asked the same question for '100').
> > 
> > Is this just kicking the can down the road?
> Also can we make it a power of two? :p
200 is 2 * 100. The 2x multiplier is arbitrary as stated in the summary. To address the question of why it was 100, my guess is that it needed to be a number sufficiently large enough to handle multiple branch instructions in succession. PR51578 has a better explanation of the history of this change. 

> Is this just kicking the can down the road?

My interpretation of PR51578 is that this increasing slop sizes is the solution to this problem. The actual solution is a larger fix and overly complicates things when "leaving more room for thunks will fix this in practice and should be good enough." cc: @thakis who came up with this  approach. 


================
Comment at: lld/MachO/ConcatOutputSection.cpp:247
     // grows. So leave room for a bunch of thunks.
-    unsigned slop = 100 * thunkSize;
+    unsigned slop = 200 * thunkSize;
     while (finalIdx < endIdx && isecAddr + inputs[finalIdx]->getSize() <
----------------
thevinster wrote:
> smeenai wrote:
> > oontvoo wrote:
> > > Where is this number from?
> > > (would have asked the same question for '100').
> > > 
> > > Is this just kicking the can down the road?
> > Also can we make it a power of two? :p
> 200 is 2 * 100. The 2x multiplier is arbitrary as stated in the summary. To address the question of why it was 100, my guess is that it needed to be a number sufficiently large enough to handle multiple branch instructions in succession. PR51578 has a better explanation of the history of this change. 
> 
> > Is this just kicking the can down the road?
> 
> My interpretation of PR51578 is that this increasing slop sizes is the solution to this problem. The actual solution is a larger fix and overly complicates things when "leaving more room for thunks will fix this in practice and should be good enough." cc: @thakis who came up with this  approach. 
I don't see why not. Did you have a number in mind? The closest power of two to 200 seems to be 256. I'm also happy to bump this number even higher if people feel like it should be higher. D108930 says "bumping it to 1000 would probably be fine." 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116705