[LLD][ELF] Don't spill to same memory region #129795
Merged
Conversation
This file contains hidden or 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
This prevents spills to the same memory region from causing spilling to take too many passes to converge. Handling this at spilling time allows us to relax the generation of spill sections; specifically, multiple spills can now be generated per output section. This should be fairly benign, and it would eventually allow allow linker scripts to express things like holes or minimum addresses for parts of output sections. The linker could then spill within an output section whenever address constraints are violated.
|
@llvm/pr-subscribers-lld-elf Author: Daniel Thornburgh (mysterymath) ChangesThis prevents spills to the same memory region from causing spilling to take too many passes to converge. Handling this at spilling time allows us to relax the generation of spill sections; specifically, multiple spills can now be generated per output section. This should be fairly benign, and it would eventually allow allow linker scripts to express things like holes or minimum addresses for parts of output sections. The linker could then spill within an output section whenever address constraints are violated. Full diff: https://github.com/llvm/llvm-project/pull/129795.diff 2 Files Affected:
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 120f5271cf229..07f5ad974f754 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -563,7 +563,7 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
continue;
if (!cmd->matchesFile(*sec->file) || pat.excludesFile(*sec->file) ||
- sec->parent == &outCmd || !flagsMatch(sec))
+ !flagsMatch(sec))
continue;
if (sec->parent) {
@@ -626,7 +626,7 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
for (InputSectionDescription *isd : scd->sc.commands) {
for (InputSectionBase *sec : isd->sectionBases) {
- if (sec->parent == &outCmd || !flagsMatch(sec))
+ if (!flagsMatch(sec))
continue;
bool isSpill = sec->parent && isa<OutputSection>(sec->parent);
if (!sec->parent || (isSpill && outCmd.name == "/DISCARD/")) {
@@ -1585,16 +1585,29 @@ bool LinkerScript::spillSections() {
if (isa<PotentialSpillSection>(isec))
continue;
- // Find the next potential spill location and remove it from the list.
auto it = potentialSpillLists.find(isec);
if (it == potentialSpillLists.end())
- continue;
+ break;
+
+ // Consume spills until finding one that might help, then consume it.
PotentialSpillList &list = it->second;
- PotentialSpillSection *spill = list.head;
- if (spill->next)
- list.head = spill->next;
- else
- potentialSpillLists.erase(isec);
+ PotentialSpillSection *spill;
+ for (spill = list.head; spill; spill = spill->next) {
+ if (list.head->next)
+ list.head = spill->next;
+ else
+ potentialSpillLists.erase(isec);
+
+ // Spills to the same regions cannot help.
+ if (spill->getParent()->memRegion == osec->memRegion &&
+ spill->getParent()->lmaRegion == osec->lmaRegion)
+ continue;
+
+ // This spill might resolve the overflow.
+ break;
+ }
+ if (!spill)
+ continue;
// Replace the next spill location with the spilled section and adjust
// its properties to match the new location. Note that the alignment of
diff --git a/lld/test/ELF/linkerscript/section-class.test b/lld/test/ELF/linkerscript/section-class.test
index 5c30b6343653e..0938fe32de81a 100644
--- a/lld/test/ELF/linkerscript/section-class.test
+++ b/lld/test/ELF/linkerscript/section-class.test
@@ -450,3 +450,51 @@ SECTIONS {
# TO-DISCARD: error: section '.two_byte_section' cannot spill from/to /DISCARD/
# TO-DISCARD-WARN: warning: section '.two_byte_section' cannot spill from/to /DISCARD/
+
+#--- same-mem-region.lds
+## Spills to the same memory region do not consume address assignment passes.
+MEMORY {
+ a : ORIGIN = 0, LENGTH = 0
+ b : ORIGIN = 0, LENGTH = 3
+}
+SECTIONS {
+ CLASS(class) { *(.one_byte_section .two_byte_section) }
+ .a00 : { CLASS(class) } >a
+ .a01 : { CLASS(class) } >a
+ .a02 : { CLASS(class) } >a
+ .a03 : { CLASS(class) } >a
+ .a04 : { CLASS(class) } >a
+ .a05 : { CLASS(class) } >a
+ .a06 : { CLASS(class) } >a
+ .a07 : { CLASS(class) } >a
+ .a08 : { CLASS(class) } >a
+ .a09 : { CLASS(class) } >a
+ .a10 : { CLASS(class) } >a
+ .a11 : { CLASS(class) } >a
+ .a12 : { CLASS(class) } >a
+ .a13 : { CLASS(class) } >a
+ .a14 : { CLASS(class) } >a
+ .a15 : { CLASS(class) } >a
+ .a16 : { CLASS(class) } >a
+ .a17 : { CLASS(class) } >a
+ .a18 : { CLASS(class) } >a
+ .a19 : { CLASS(class) } >a
+ .a20 : { CLASS(class) } >a
+ .a21 : { CLASS(class) } >a
+ .a22 : { CLASS(class) } >a
+ .a23 : { CLASS(class) } >a
+ .a24 : { CLASS(class) } >a
+ .a25 : { CLASS(class) } >a
+ .a26 : { CLASS(class) } >a
+ .a27 : { CLASS(class) } >a
+ .a28 : { CLASS(class) } >a
+ .a29 : { CLASS(class) } >a
+ .a30 : { CLASS(class) } >a
+ .b : { CLASS(class) } >b
+}
+
+# RUN: ld.lld -T same-mem-region.lds -o same-mem-region spill.o
+# RUN: llvm-readelf -S same-mem-region | FileCheck %s --check-prefix=SAME-MEM-REGION
+
+# SAME-MEM-REGION: Name Type Address Off Size
+# SAME-MEM-REGION: .b PROGBITS 0000000000000000 001000 000003
|
|
@llvm/pr-subscribers-lld Author: Daniel Thornburgh (mysterymath) ChangesThis prevents spills to the same memory region from causing spilling to take too many passes to converge. Handling this at spilling time allows us to relax the generation of spill sections; specifically, multiple spills can now be generated per output section. This should be fairly benign, and it would eventually allow allow linker scripts to express things like holes or minimum addresses for parts of output sections. The linker could then spill within an output section whenever address constraints are violated. Full diff: https://github.com/llvm/llvm-project/pull/129795.diff 2 Files Affected:
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 120f5271cf229..07f5ad974f754 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -563,7 +563,7 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
continue;
if (!cmd->matchesFile(*sec->file) || pat.excludesFile(*sec->file) ||
- sec->parent == &outCmd || !flagsMatch(sec))
+ !flagsMatch(sec))
continue;
if (sec->parent) {
@@ -626,7 +626,7 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd,
for (InputSectionDescription *isd : scd->sc.commands) {
for (InputSectionBase *sec : isd->sectionBases) {
- if (sec->parent == &outCmd || !flagsMatch(sec))
+ if (!flagsMatch(sec))
continue;
bool isSpill = sec->parent && isa<OutputSection>(sec->parent);
if (!sec->parent || (isSpill && outCmd.name == "/DISCARD/")) {
@@ -1585,16 +1585,29 @@ bool LinkerScript::spillSections() {
if (isa<PotentialSpillSection>(isec))
continue;
- // Find the next potential spill location and remove it from the list.
auto it = potentialSpillLists.find(isec);
if (it == potentialSpillLists.end())
- continue;
+ break;
+
+ // Consume spills until finding one that might help, then consume it.
PotentialSpillList &list = it->second;
- PotentialSpillSection *spill = list.head;
- if (spill->next)
- list.head = spill->next;
- else
- potentialSpillLists.erase(isec);
+ PotentialSpillSection *spill;
+ for (spill = list.head; spill; spill = spill->next) {
+ if (list.head->next)
+ list.head = spill->next;
+ else
+ potentialSpillLists.erase(isec);
+
+ // Spills to the same regions cannot help.
+ if (spill->getParent()->memRegion == osec->memRegion &&
+ spill->getParent()->lmaRegion == osec->lmaRegion)
+ continue;
+
+ // This spill might resolve the overflow.
+ break;
+ }
+ if (!spill)
+ continue;
// Replace the next spill location with the spilled section and adjust
// its properties to match the new location. Note that the alignment of
diff --git a/lld/test/ELF/linkerscript/section-class.test b/lld/test/ELF/linkerscript/section-class.test
index 5c30b6343653e..0938fe32de81a 100644
--- a/lld/test/ELF/linkerscript/section-class.test
+++ b/lld/test/ELF/linkerscript/section-class.test
@@ -450,3 +450,51 @@ SECTIONS {
# TO-DISCARD: error: section '.two_byte_section' cannot spill from/to /DISCARD/
# TO-DISCARD-WARN: warning: section '.two_byte_section' cannot spill from/to /DISCARD/
+
+#--- same-mem-region.lds
+## Spills to the same memory region do not consume address assignment passes.
+MEMORY {
+ a : ORIGIN = 0, LENGTH = 0
+ b : ORIGIN = 0, LENGTH = 3
+}
+SECTIONS {
+ CLASS(class) { *(.one_byte_section .two_byte_section) }
+ .a00 : { CLASS(class) } >a
+ .a01 : { CLASS(class) } >a
+ .a02 : { CLASS(class) } >a
+ .a03 : { CLASS(class) } >a
+ .a04 : { CLASS(class) } >a
+ .a05 : { CLASS(class) } >a
+ .a06 : { CLASS(class) } >a
+ .a07 : { CLASS(class) } >a
+ .a08 : { CLASS(class) } >a
+ .a09 : { CLASS(class) } >a
+ .a10 : { CLASS(class) } >a
+ .a11 : { CLASS(class) } >a
+ .a12 : { CLASS(class) } >a
+ .a13 : { CLASS(class) } >a
+ .a14 : { CLASS(class) } >a
+ .a15 : { CLASS(class) } >a
+ .a16 : { CLASS(class) } >a
+ .a17 : { CLASS(class) } >a
+ .a18 : { CLASS(class) } >a
+ .a19 : { CLASS(class) } >a
+ .a20 : { CLASS(class) } >a
+ .a21 : { CLASS(class) } >a
+ .a22 : { CLASS(class) } >a
+ .a23 : { CLASS(class) } >a
+ .a24 : { CLASS(class) } >a
+ .a25 : { CLASS(class) } >a
+ .a26 : { CLASS(class) } >a
+ .a27 : { CLASS(class) } >a
+ .a28 : { CLASS(class) } >a
+ .a29 : { CLASS(class) } >a
+ .a30 : { CLASS(class) } >a
+ .b : { CLASS(class) } >b
+}
+
+# RUN: ld.lld -T same-mem-region.lds -o same-mem-region spill.o
+# RUN: llvm-readelf -S same-mem-region | FileCheck %s --check-prefix=SAME-MEM-REGION
+
+# SAME-MEM-REGION: Name Type Address Off Size
+# SAME-MEM-REGION: .b PROGBITS 0000000000000000 001000 000003
|
smithp35
reviewed
Mar 5, 2025
There was a problem hiding this comment.
Thanks for the fix. I expect that was a fun one to track down.
I got a little surprised by the removal of sec->parent == &outCmd as these came first and it seemed like these might help rather than hinder. IIUC these are no longer necessary, rather than being required (tests still pass if I remove them).
| @@ -563,7 +563,7 @@ LinkerScript::computeInputSections(const InputSectionDescription *cmd, | |||
| continue; | |||
|
|
|||
| if (!cmd->matchesFile(*sec->file) || pat.excludesFile(*sec->file) || | |||
| sec->parent == &outCmd || !flagsMatch(sec)) | |||
| !flagsMatch(sec)) | |||
There was a problem hiding this comment.
Just so I understand. These changes are not required for the fix, but they are no longer necessary because of the changes to spillSections()
There was a problem hiding this comment.
Yes, that's correct. This was code added to prevent this roughly this issue, but it appears to have been neither necessary nor sufficient.
As-is, this shouldn't create any behavioral differences, since spilling to a different memory region implies spilling to a different output section. But this also does hint towards a future way to express very detailed address constraints within an output section, which is something that has been on my radar for a while.
For example:
CLASS(text) ( *(.text) }
.text {
CLASS(text)
. = 0x1234;
*(.text.at_0x1234)
CLASS(text)
}
The first pass would assign everything to the first CLASS reference, but this would cause dot to move backwards. The spiller could detect the amount dot moved backwards and spill that much from a preceding class, just like it does for memory region overages. The difference is that later references within the same output section may help to resolve this.
I don't think there's actually anything you could achieve with this that you couldn't achieve by splitting everything into fine-grained output sections, but to me it would just help to make spilling feel more orthogonal with linker features like dot assignment. Broadly, I'd tend to think that any address assignment failure should trigger spilling if that might have a chance to prevent the issue on the next pass.
|
|
||
| # RUN: ld.lld -T same-mem-region.lds -o same-mem-region spill.o | ||
| # RUN: llvm-readelf -S same-mem-region | FileCheck %s --check-prefix=SAME-MEM-REGION | ||
|
|
There was a problem hiding this comment.
Is there a reasonable way to test the differences in mem region and lma region? For example an output section is >a AT> b I don't think it necessary has to be a within a pass limit though, just showing it is possible.
There was a problem hiding this comment.
Good idea, and this uncovered a subtlety. We should only skip entries with the same mem region if it is currently overflowing; same with lma region. Added a test and fix.
mysterymath
force-pushed
the
lld-spill-bug
branch
from
b1ddf30 to
e833a6d
Compare
smithp35
approved these changes
Mar 6, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/85/builds/6399 Here is the relevant piece of the build log for the reference |
mysterymath
added a commit
that referenced
this pull request
Mar 11, 2025
This reverts commit 5f21ee2.
mysterymath
added a commit
that referenced
this pull request
Mar 11, 2025
Reverts #129795 Breaks buildbots
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 11, 2025
…0815) Reverts llvm/llvm-project#129795 Breaks buildbots
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/25/builds/7096 Here is the relevant piece of the build log for the reference |
mysterymath
added a commit
to mysterymath/llvm-project
that referenced
this pull request
Mar 11, 2025
Relands llvm#129795 Remove skipped potential spills from their parent input section descriptions to prevent undefined behavior.
mysterymath
added a commit
that referenced
this pull request
Mar 11, 2025
Relands #129795 Remove skipped potential spills from their parent input section descriptions to prevent undefined behavior.
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.
This prevents useless spills to the same memory region from causing spilling to take too many passes to converge.
Handling this at spilling time allows us to relax the generation of spill sections; specifically, multiple spills can now be generated per output section. This should be fairly benign for performance, and it would eventually allow linker scripts to express things like holes or minimum addresses for parts of output sections. The linker could then spill within an output section whenever address constraints are violated.