[LLD][AArch64] Detach Landing Pad creation from Thunk creation #116402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move Landing Pad Creation to a new function that checks each thunk every pass to see if it needs a landing pad. This permits a thunk to be created without needing a landing pad, but later needing one due to drifting out of direct branch range and requiring an indirect branch. We record all the Thunks created so far in a new vector rather than trying to iterate over the DenseMap as we need a deterministic order of adding LandingPadThunks due to the short branch fall through. We cannot use normalizeExistingThunk() either as that only iterates through live thunks. Fixes: https://crbug.com/377438309 Original PR: llvm#108989 Sending without a new test case to fix existing test. A new regression test will come in a separate PR as coming up with a small enough reproducer for this case is non-trivial.
A leftover from a previous attempt that I forgot to get rid of.
MaskRay
reviewed
Nov 15, 2024
lld/ELF/Relocations.cpp
Outdated
| bool ThunkCreator::addSyntheticLandingPads() { | ||
| bool addressesChanged = false; | ||
| for (Thunk *t : allThunks) { | ||
| if (t->needsSyntheticLandingPad()) { |
There was a problem hiding this comment.
I've flipped the condition so it can continue. This loses a level of indentation. We need to go through all the thunks so we can't return.
MaskRay
reviewed
Nov 15, 2024
lld/ELF/Relocations.cpp
Outdated
| std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend); | ||
| if (isNew) { | ||
| addressesChanged = true; | ||
| ThunkSection *ts = getISThunkSec(cast<InputSection>(dr.section)); |
There was a problem hiding this comment.
The used-once variable can be avoided
MaskRay
approved these changes
Nov 15, 2024
* Invert condition to allow early continue. * Remove used once variable.
smithp35
commented
Nov 15, 2024
lld/ELF/Relocations.cpp
Outdated
| bool ThunkCreator::addSyntheticLandingPads() { | ||
| bool addressesChanged = false; | ||
| for (Thunk *t : allThunks) { | ||
| if (t->needsSyntheticLandingPad()) { |
There was a problem hiding this comment.
I've flipped the condition so it can continue. This loses a level of indentation. We need to go through all the thunks so we can't return.
lld/ELF/Relocations.cpp
Outdated
| std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend); | ||
| if (isNew) { | ||
| addressesChanged = true; | ||
| ThunkSection *ts = getISThunkSec(cast<InputSection>(dr.section)); |
|
@llvm/pr-subscribers-lld Author: Peter Smith (smithp35) ChangesMove Landing Pad Creation to a new function that checks each thunk every pass to see if it needs a landing pad. This permits a thunk to be created without needing a landing pad, but later needing one due to drifting out of direct branch range and requiring an indirect branch. We record all the Thunks created so far in a new vector rather than trying to iterate over the DenseMap as we need a deterministic order of adding LandingPadThunks due to the short branch fall through. We cannot use normalizeExistingThunk() either as that only iterates through live thunks. Fixes: https://crbug.com/377438309 Sending without a new test case to fix existing test. A new regression test will come in a separate PR as coming up with a small enough reproducer for this case is non-trivial. Full diff: https://github.com/llvm/llvm-project/pull/116402.diff 2 Files Affected:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 261ef9f832d9c2..6ef91af51568d4 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -2293,6 +2293,30 @@ bool ThunkCreator::normalizeExistingThunk(Relocation &rel, uint64_t src) {
return false;
}
+// When indirect branches are restricted, such as AArch64 BTI Thunks may need
+// to target a linker generated landing pad instead of the target. This needs
+// to be done once per pass as the need for a BTI thunk is dependent whether
+// a thunk is short or long. We iterate over all the thunks to make sure we
+// catch thunks that have been created but are no longer live. Non-live thunks
+// are not reachable via normalizeExistingThunk() but are still written.
+bool ThunkCreator::addSyntheticLandingPads() {
+ bool addressesChanged = false;
+ for (Thunk *t : allThunks) {
+ if (!t->needsSyntheticLandingPad())
+ continue;
+ Thunk *lpt;
+ bool isNew;
+ auto &dr = cast<Defined>(t->destination);
+ std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend);
+ if (isNew) {
+ addressesChanged = true;
+ getISThunkSec(cast<InputSection>(dr.section))->addThunk(lpt);
+ }
+ t->landingPad = lpt->getThunkTargetSym();
+ }
+ return addressesChanged;
+}
+
// Process all relocations from the InputSections that have been assigned
// to InputSectionDescriptions and redirect through Thunks if needed. The
// function should be called iteratively until it returns false.
@@ -2326,6 +2350,9 @@ bool ThunkCreator::createThunks(uint32_t pass,
if (pass == 0 && ctx.target->getThunkSectionSpacing())
createInitialThunkSections(outputSections);
+ if (ctx.arg.emachine == EM_AARCH64)
+ addressesChanged = addSyntheticLandingPads();
+
// Create all the Thunks and insert them into synthetic ThunkSections. The
// ThunkSections are later inserted back into InputSectionDescriptions.
// We separate the creation of ThunkSections from the insertion of the
@@ -2360,20 +2387,7 @@ bool ThunkCreator::createThunks(uint32_t pass,
ts = getISDThunkSec(os, isec, isd, rel, src);
ts->addThunk(t);
thunks[t->getThunkTargetSym()] = t;
-
- // When indirect branches are restricted, such as AArch64 BTI
- // Thunks may need to target a linker generated landing pad
- // instead of the target.
- if (t->needsSyntheticLandingPad()) {
- Thunk *lpt;
- auto &dr = cast<Defined>(t->destination);
- std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend);
- if (isNew) {
- ts = getISThunkSec(cast<InputSection>(dr.section));
- ts->addThunk(lpt);
- }
- t->landingPad = lpt->getThunkTargetSym();
- }
+ allThunks.push_back(t);
}
// Redirect relocation to Thunk, we never go via the PLT to a Thunk
diff --git a/lld/ELF/Relocations.h b/lld/ELF/Relocations.h
index 64e67c2c968207..ed0c665c7ffe9c 100644
--- a/lld/ELF/Relocations.h
+++ b/lld/ELF/Relocations.h
@@ -183,6 +183,8 @@ class ThunkCreator {
bool normalizeExistingThunk(Relocation &rel, uint64_t src);
+ bool addSyntheticLandingPads();
+
Ctx &ctx;
// Record all the available Thunks for a (Symbol, addend) pair, where Symbol
@@ -216,6 +218,9 @@ class ThunkCreator {
Thunk *>
landingPadsBySectionAndAddend;
+ // All the nonLandingPad thunks that have been created, in order of creation.
+ std::vector<Thunk *> allThunks;
+
// The number of completed passes of createThunks this permits us
// to do one time initialization on Pass 0 and put a limit on the
// number of times it can be called to prevent infinite loops.
|
|
@llvm/pr-subscribers-lld-elf Author: Peter Smith (smithp35) ChangesMove Landing Pad Creation to a new function that checks each thunk every pass to see if it needs a landing pad. This permits a thunk to be created without needing a landing pad, but later needing one due to drifting out of direct branch range and requiring an indirect branch. We record all the Thunks created so far in a new vector rather than trying to iterate over the DenseMap as we need a deterministic order of adding LandingPadThunks due to the short branch fall through. We cannot use normalizeExistingThunk() either as that only iterates through live thunks. Fixes: https://crbug.com/377438309 Sending without a new test case to fix existing test. A new regression test will come in a separate PR as coming up with a small enough reproducer for this case is non-trivial. Full diff: https://github.com/llvm/llvm-project/pull/116402.diff 2 Files Affected:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 261ef9f832d9c2..6ef91af51568d4 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -2293,6 +2293,30 @@ bool ThunkCreator::normalizeExistingThunk(Relocation &rel, uint64_t src) {
return false;
}
+// When indirect branches are restricted, such as AArch64 BTI Thunks may need
+// to target a linker generated landing pad instead of the target. This needs
+// to be done once per pass as the need for a BTI thunk is dependent whether
+// a thunk is short or long. We iterate over all the thunks to make sure we
+// catch thunks that have been created but are no longer live. Non-live thunks
+// are not reachable via normalizeExistingThunk() but are still written.
+bool ThunkCreator::addSyntheticLandingPads() {
+ bool addressesChanged = false;
+ for (Thunk *t : allThunks) {
+ if (!t->needsSyntheticLandingPad())
+ continue;
+ Thunk *lpt;
+ bool isNew;
+ auto &dr = cast<Defined>(t->destination);
+ std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend);
+ if (isNew) {
+ addressesChanged = true;
+ getISThunkSec(cast<InputSection>(dr.section))->addThunk(lpt);
+ }
+ t->landingPad = lpt->getThunkTargetSym();
+ }
+ return addressesChanged;
+}
+
// Process all relocations from the InputSections that have been assigned
// to InputSectionDescriptions and redirect through Thunks if needed. The
// function should be called iteratively until it returns false.
@@ -2326,6 +2350,9 @@ bool ThunkCreator::createThunks(uint32_t pass,
if (pass == 0 && ctx.target->getThunkSectionSpacing())
createInitialThunkSections(outputSections);
+ if (ctx.arg.emachine == EM_AARCH64)
+ addressesChanged = addSyntheticLandingPads();
+
// Create all the Thunks and insert them into synthetic ThunkSections. The
// ThunkSections are later inserted back into InputSectionDescriptions.
// We separate the creation of ThunkSections from the insertion of the
@@ -2360,20 +2387,7 @@ bool ThunkCreator::createThunks(uint32_t pass,
ts = getISDThunkSec(os, isec, isd, rel, src);
ts->addThunk(t);
thunks[t->getThunkTargetSym()] = t;
-
- // When indirect branches are restricted, such as AArch64 BTI
- // Thunks may need to target a linker generated landing pad
- // instead of the target.
- if (t->needsSyntheticLandingPad()) {
- Thunk *lpt;
- auto &dr = cast<Defined>(t->destination);
- std::tie(lpt, isNew) = getSyntheticLandingPad(dr, t->addend);
- if (isNew) {
- ts = getISThunkSec(cast<InputSection>(dr.section));
- ts->addThunk(lpt);
- }
- t->landingPad = lpt->getThunkTargetSym();
- }
+ allThunks.push_back(t);
}
// Redirect relocation to Thunk, we never go via the PLT to a Thunk
diff --git a/lld/ELF/Relocations.h b/lld/ELF/Relocations.h
index 64e67c2c968207..ed0c665c7ffe9c 100644
--- a/lld/ELF/Relocations.h
+++ b/lld/ELF/Relocations.h
@@ -183,6 +183,8 @@ class ThunkCreator {
bool normalizeExistingThunk(Relocation &rel, uint64_t src);
+ bool addSyntheticLandingPads();
+
Ctx &ctx;
// Record all the available Thunks for a (Symbol, addend) pair, where Symbol
@@ -216,6 +218,9 @@ class ThunkCreator {
Thunk *>
landingPadsBySectionAndAddend;
+ // All the nonLandingPad thunks that have been created, in order of creation.
+ std::vector<Thunk *> allThunks;
+
// The number of completed passes of createThunks this permits us
// to do one time initialization on Pass 0 and put a limit on the
// number of times it can be called to prevent infinite loops.
|
smithp35
added a commit
to smithp35/llvm-project
that referenced
this pull request
Nov 18, 2024
A follow up to PR llvm#116402 to add a regression test. The original change fixed the reproducer but that was not suitable to use as a regression test. This test case will fail with a LLD prior to llvm#116402. The disassembly for the thunk that starts as a short thunk but is later a long thunk isn't quite right. It is missing a $d mapping symbol. I think this can be fixed, but I've not done that in this patch to keep it test only. It is not a regression introduced in llvm#116402. I've also removed a spurious --threads=1 I noticed in the original test aarch64-thunk-bti.s
smithp35
added a commit
that referenced
this pull request
Nov 19, 2024
A follow up to PR #116402 to add a regression test. The original change fixed the reproducer but that was not suitable to use as a regression test. This test case will fail with a LLD prior to #116402. The disassembly for the thunk that starts as a short thunk but is later a long thunk isn't quite right. It is missing a $d mapping symbol. I think this can be fixed, but I've not done that in this patch to keep it test only. It is not a regression introduced in #116402. I've also removed a spurious --threads=1 I noticed in the original test aarch64-thunk-bti.s
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move Landing Pad Creation to a new function that checks each thunk every pass to see if it needs a landing pad. This permits a thunk to be created without needing a landing pad, but later needing one due to drifting out of direct branch range and requiring an indirect branch.
We record all the Thunks created so far in a new vector rather than trying to iterate over the DenseMap as we need a deterministic order of adding LandingPadThunks due to the short branch fall through. We cannot use normalizeExistingThunk() either as that only iterates through live thunks.
Fixes: https://crbug.com/377438309
Original PR: #108989
Sending without a new test case to fix existing test. A new regression test will come in a separate PR as coming up with a small enough reproducer for this case is non-trivial.