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

[thevinster]

Summary

One of our internal arm64 apps hit a thunk out of range error when building
with LLD. Per the comment, I'm arbitrarily increasing slop size to 256.

[oontvoo]

lld/MachO/ConcatOutputSection.cpp

247

Where is this number from?
(would have asked the same question for '100').

Is this just kicking the can down the road?

[smeenai]

lld/MachO/ConcatOutputSection.cpp

247

Also can we make it a power of two? :p

[thevinster]

lld/MachO/ConcatOutputSection.cpp

247

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.

247

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

[int3]

Maybe make it configurable too?

[thakis]

For "why 100": My repro worked with 10, so I figured if I put in ten times that nobody should ever hit this again. Looks like that didn't work out 😓

This looks fine. 256 is fine too. It means we insert thunks we don't need slightly more often, but still only very rarely.

[oontvoo]

lld/MachO/ConcatOutputSection.cpp

247

makes sense! thanks for the details!

[thevinster]

Maybe make it configurable too?

I lean towards not making it configurable because it's not meant to be adjusted frequently (or even adjusted at all by the user). I'm hoping 256 (which seems to be the consensus) is enough to handle virtually everything now.

[thevinster]

Increase to 256

[glandium]

FWIW, we've had this happen recently on non-optimized builds of Firefox with the 256 value (although it's not happening anymore as of writing, whatever made it disappear)... is there a point where increasing the slop is going to be a problem?

[int3]

is there a point where increasing the slop is going to be a problem?

Not really, it just makes things slightly less efficient size-wise. We should deal with this more cleanly though by making the slop a dynamically-adjusted parameter (i.e. if it isn't enough, LLD should just bump it up and retry w/o user intervention)

lld/MachO/ConcatOutputSection.cpp

for (size_t callIdx = 0, endIdx = inputs.size(); callIdx < endIdx;
 
     // Assign addresses up-to the forward branch-range limit.
     // Every call instruction needs a small number of bytes (on Arm64: 4),
     // and each inserted thunk needs a slightly larger number of bytes
     // (on Arm64: 12). If a section starts with a branch instruction and
     // contains several branch instructions in succession, then the distance
     // from the current position to the position where the thunks are inserted
     // grows. So leave room for a bunch of thunks.
-    unsigned slop = 100 * thunkSize;
+    unsigned slop = 256 * thunkSize;
     while (finalIdx < endIdx && isecAddr + inputs[finalIdx]->getSize() <
                                     isecVA + forwardBranchRange - slop)
       finalizeOne(inputs[finalIdx++]);
 
     if (!isec->hasCallSites)
       continue;
 
     if (finalIdx == endIdx && stubsInRangeVA == TargetInfo::outOfRangeVA) {