Page MenuHomePhabricator

[lld/mac] Set branchRange a bit more carefully
ClosedPublic

Authored by thakis on Aug 29 2021, 12:26 PM.

Details

Reviewers
gkm
int3
Group Reviewers
Restricted Project
Commits
rG9721197520e5: [lld/mac] Set branchRange a bit more carefully
Summary
  • Don't subtract thunkSize from branchRange. Most places care about the actual maximal branch range. Subtract thunkSize in the one place that wants to leave room for a thunk.
  • Set it to 0x800_0000 instead of 0xFF_FFFF
  • Subtract 4 for the positive branch direction since it's a two's complement 24bit number sign-extended mutiplied by 4, so its range is -0x800_0000..+0x7FF_FFFC
  • Make boundary checks include the boundary values

This doesn't make a huge difference in practice. It's preparation
for a "real" fix for PR51578 -- but it also lets the repro in comment 0
in that bug place one more thunk before hitting the TODO.

Diff Detail

Event Timeline

thakis created this revision.Aug 29 2021, 12:26 PM
Herald added a project: Restricted Project. · View Herald Transcript
thakis requested review of this revision.Aug 29 2021, 12:26 PM
thakis added inline comments.
lld/MachO/ConcatOutputSection.cpp
167

I don't understand the !tp.first->isInStubs() check here btw – a jump to an import stub might still require a thunk if the stub is towards the end of the stubs table and the stubs table is large, no?

(I mean, the whole thing here is an over-estimate so in practice it should be fine, but is there something I'm missing here?)

259–261

(See also AArch64::inBranchRange in lld/ELF/Arch/AArch64.cpp)

tschuett added inline comments.
lld/MachO/ConcatOutputSection.cpp
259–261

Do you have something with more information than a 4. Maybe sizeof() or a named constant?

thakis added inline comments.Aug 29 2021, 3:58 PM
lld/MachO/ConcatOutputSection.cpp
259–261

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

int3 added a subscriber: int3.Aug 29 2021, 5:54 PM
int3 added inline comments.
lld/MachO/Arch/ARM64.cpp
137–143

nit: 1 << 28 would be clearer imo

lld/MachO/ConcatOutputSection.cpp
178

I think the size of the stubs table is factored in here

259–261

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?

thakis updated this revision to Diff 369435.Aug 30 2021, 6:23 AM

forwardBranchRange / backwardBranchRange

lld/MachO/ConcatOutputSection.cpp
178

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.

259–261

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.

int3 accepted this revision.Aug 30 2021, 8:47 AM

lgtm

lld/MachO/Arch/ARM64.cpp
137–144

would be nice to have a comment explaining the - 4

lld/MachO/ConcatOutputSection.cpp
178

oh gotcha. yeah, I think you're right

This revision is now accepted and ready to land.Aug 30 2021, 8:47 AM
This revision was automatically updated to reflect the committed changes.
thakis marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2021, 9:36 AM
thakis added inline comments.Aug 30 2021, 9:57 AM
lld/MachO/ConcatOutputSection.cpp
178

Made https://reviews.llvm.org/D108924 for this then :)