[lld-macho] Ensure all sections in __TEXT get thunks if necessary #87818
Conversation
|
@llvm/pr-subscribers-lld-macho @llvm/pr-subscribers-lld Author: Leonard Grey (speednoisemovement) ChangesThis is a proposed fix for issue #52767. It has some flaws, but I wanted to put it out for discussion. We decide whether a section has thunk callsites by comparing its size with the branch range. This can fail in the case of sections that are themselves smaller than the branch range, but need to branch to addresses in other sections that are farther than the branch range away. We ran into this in Chrome ASAN builds with the Probably the right way to do this is to check if the total size of text sections that can serve as branch targets is bigger than the range, but I couldn't think of a nice way to do this. This change sets a bit on the segment if one section is found to need thunks. Sections processed afterwards in the same segment will take the thunks branch regardless of their size. This makes the assumption that the biggest section comes first (probably true almost always) and that a single section is bigger than the branch range (probably not true at the margins). Full diff: https://github.com/llvm/llvm-project/pull/87818.diff 3 Files Affected:
diff --git a/lld/MachO/ConcatOutputSection.cpp b/lld/MachO/ConcatOutputSection.cpp
index 279423720be9d5..cd87f9cc4c3caf 100644
--- a/lld/MachO/ConcatOutputSection.cpp
+++ b/lld/MachO/ConcatOutputSection.cpp
@@ -127,10 +127,20 @@ bool TextOutputSection::needsThunks() const {
uint64_t isecAddr = addr;
for (ConcatInputSection *isec : inputs)
isecAddr = alignToPowerOf2(isecAddr, isec->align) + isec->getSize();
- if (isecAddr - addr + in.stubs->getSize() <=
- std::min(target->backwardBranchRange, target->forwardBranchRange))
+ // Other sections besides __text might be small enough to pass this
+ // test but nevertheless need thunks for calling into oher sections.
+ // An imperfect heuristic to use in this case is that if a section
+ // we've already processed in this segment needs thunks, so do the
+ // rest.
+ bool needsThunks = parent && parent->needsThunks;
+ if (!needsThunks &&
+ isecAddr - addr + in.stubs->getSize() <=
+ std::min(target->backwardBranchRange, target->forwardBranchRange))
return false;
// Yes, this program is large enough to need thunks.
+ if (parent) {
+ parent->needsThunks = true;
+ }
for (ConcatInputSection *isec : inputs) {
for (Reloc &r : isec->relocs) {
if (!target->hasAttr(r.type, RelocAttrBits::BRANCH))
diff --git a/lld/MachO/OutputSegment.h b/lld/MachO/OutputSegment.h
index 7a0c4a2065a15b..5f39734ffdbd7a 100644
--- a/lld/MachO/OutputSegment.h
+++ b/lld/MachO/OutputSegment.h
@@ -57,6 +57,7 @@ class OutputSegment {
uint32_t initProt = 0;
uint32_t flags = 0;
uint8_t index;
+ bool needsThunks = false;
llvm::TinyPtrVector<Defined *> segmentStartSymbols;
llvm::TinyPtrVector<Defined *> segmentEndSymbols;
diff --git a/lld/test/MachO/arm64-thunks.s b/lld/test/MachO/arm64-thunks.s
index 0e4157ece72de2..ea40effd696f59 100644
--- a/lld/test/MachO/arm64-thunks.s
+++ b/lld/test/MachO/arm64-thunks.s
@@ -7,6 +7,7 @@
## (3) a second thunk is created when the first one goes out of range
## (4) early calls to a dylib stub use a thunk, and later calls the stub
## directly
+## (5) Thunks are created for all sections in the text segment with branches.
## Notes:
## 0x4000000 = 64 Mi = half the magnitude of the forward-branch range
@@ -168,6 +169,10 @@
# CHECK: [[#%x, NAN_PAGE + NAN_OFFSET]] <__stubs>:
+# CHECK: Disassembly of section __TEXT,__lcxx_override:
+# CHECK: <_z>:
+# CHECK: bl 0x[[#%x, A_THUNK_0]] <_a.thunk.0>
+
.subsections_via_symbols
.text
@@ -300,3 +305,12 @@ _main:
bl _h
bl ___nan
ret
+
+.section __TEXT,__lcxx_override,regular,pure_instructions
+
+.globl _z
+.no_dead_strip _z
+.p2align 2
+_z:
+ bl _a
+ ret
|
|
So this doesn't totally do it; Chromium ASAN So, contra the original description, moving the section closer to and/or before stubs does help in this specific case; it's just not sufficient to solve the entire problem. I think we want to refactor how we do this, maybe by ensuring all |
|
Dropping this in favor of #99052 |
This supersedes #87818 and fixes #52767 When calculating arm64 thunks, we make a few assumptions that may not hold when considering code sections outside of `__text`: 1. That a section needs thunks only if its size is larger than the branch range. 2. That any calls into `__stubs` are necessarily forward jumps (that is, the section with the jump is ordered before `__stubs`) Sections like this exist in the wild, most prominently the `__lcxx_overrides` section introduced in #69498 This change: - Ensures that if one section in `__TEXT` gets thunks, all of them do. - Makes all code sections in `__TEXT` contiguous (and guaranteed to be placed before `__stubs`)
This supersedes llvm#87818 and fixes llvm#52767 When calculating arm64 thunks, we make a few assumptions that may not hold when considering code sections outside of `__text`: 1. That a section needs thunks only if its size is larger than the branch range. 2. That any calls into `__stubs` are necessarily forward jumps (that is, the section with the jump is ordered before `__stubs`) Sections like this exist in the wild, most prominently the `__lcxx_overrides` section introduced in llvm#69498 This change: - Ensures that if one section in `__TEXT` gets thunks, all of them do. - Makes all code sections in `__TEXT` contiguous (and guaranteed to be placed before `__stubs`)
Summary: This supersedes #87818 and fixes #52767 When calculating arm64 thunks, we make a few assumptions that may not hold when considering code sections outside of `__text`: 1. That a section needs thunks only if its size is larger than the branch range. 2. That any calls into `__stubs` are necessarily forward jumps (that is, the section with the jump is ordered before `__stubs`) Sections like this exist in the wild, most prominently the `__lcxx_overrides` section introduced in #69498 This change: - Ensures that if one section in `__TEXT` gets thunks, all of them do. - Makes all code sections in `__TEXT` contiguous (and guaranteed to be placed before `__stubs`) Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251191
This is a proposed fix for issue #52767. It has some flaws, but I wanted to put it out for discussion.
We decide whether a section has thunk callsites by comparing its size with the branch range. This can fail in the case of sections that are themselves smaller than the branch range, but need to branch to addresses in other sections that are farther than the branch range away. We ran into this in Chrome ASAN builds with the
__lcxx_overridesection (see https://crbug.com/326898585). Putting the section closer to__text, or before__stubsdoesn't help since__textis large enough on its own to require thunks for branches into it.Probably the right way to do this is to check if the total size of text sections that can serve as branch targets is bigger than the range, but I couldn't think of a nice way to do this. This change sets a bit on the segment if one section is found to need thunks. Sections processed afterwards in the same segment will take the thunks branch regardless of their size.
This makes the assumption that the biggest section comes first (probably true almost always) and that a single section is bigger than the branch range (probably not true at the margins).