Skip to content

Reland "[LLD][ELF] Don't spill to same memory region" #130851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2025
Merged

Reland "[LLD][ELF] Don't spill to same memory region" #130851

merged 1 commit into from
Mar 11, 2025

Conversation

mysterymath
Copy link
Contributor

Relands #129795

Remove skipped potential spills from their parent input section descriptions to prevent undefined behavior.

@mysterymath
Reland "[LLD][ELF] Don't spill to same memory region"
ceca521 
Relands llvm#129795

Remove skipped potential spills from their parent input section
descriptions to prevent undefined behavior.
@mysterymath mysterymath merged commit 009dfb4 into llvm:main Mar 11, 2025
6 of 10 checks passed
@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2025

@llvm/pr-subscribers-lld

Author: Daniel Thornburgh (mysterymath)

Changes

Relands #129795

Remove skipped potential spills from their parent input section descriptions to prevent undefined behavior.

Full diff: https://github.com/llvm/llvm-project/pull/130851.diff

2 Files Affected:

  • (modified) lld/ELF/LinkerScript.cpp (+38-9)
  • (modified) lld/test/ELF/linkerscript/section-class.test (+100)
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 120f5271cf229..0d5b383f4b596 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/")) {
@@ -1559,6 +1559,8 @@ bool LinkerScript::spillSections() {
   if (potentialSpillLists.empty())
     return false;
 
+  DenseSet<PotentialSpillSection *> skippedSpills;
+
   bool spilled = false;
   for (SectionCommand *cmd : reverse(sectionCommands)) {
     auto *osd = dyn_cast<OutputDesc>(cmd);
@@ -1585,16 +1587,34 @@ 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.
+        auto canSpillHelp = [&](PotentialSpillSection *spill) {
+          // Spills to the same region that overflowed cannot help.
+          if (hasRegionOverflowed(osec->memRegion) &&
+              spill->getParent()->memRegion == osec->memRegion)
+            return false;
+          if (hasRegionOverflowed(osec->lmaRegion) &&
+              spill->getParent()->lmaRegion == osec->lmaRegion)
+            return false;
+          return true;
+        };
         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);
+          if (canSpillHelp(spill))
+            break;
+          skippedSpills.insert(spill);
+        }
+        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
@@ -1629,6 +1649,15 @@ bool LinkerScript::spillSections() {
     }
   }
 
+  // Clean up any skipped spills.
+  DenseSet<InputSectionDescription *> isds;
+  for (PotentialSpillSection *s : skippedSpills)
+    isds.insert(s->isd);
+  for (InputSectionDescription *isd : isds)
+    llvm::erase_if(isd->sections, [&](InputSection *s) {
+      return skippedSpills.contains(dyn_cast<PotentialSpillSection>(s));
+    });
+
   return spilled;
 }
 
diff --git a/lld/test/ELF/linkerscript/section-class.test b/lld/test/ELF/linkerscript/section-class.test
index 5c30b6343653e..27273f3855c32 100644
--- a/lld/test/ELF/linkerscript/section-class.test
+++ b/lld/test/ELF/linkerscript/section-class.test
@@ -450,3 +450,103 @@ 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 that overflowed do not consume address assignment passes.
+MEMORY {
+  a : ORIGIN = 0, LENGTH = 0
+  b : ORIGIN = 0, LENGTH = 3
+  c : ORIGIN = 3, LENGTH = 3
+  d : ORIGIN = 6, LENGTH = 3
+}
+SECTIONS {
+  CLASS(class) { *(.one_byte_section .two_byte_section) }
+  .a00 : { CLASS(class) } >a AT>c
+  .a01 : { CLASS(class) } >a AT>d
+  .a02 : { CLASS(class) } >a AT>d
+  .a03 : { CLASS(class) } >a AT>d
+  .a04 : { CLASS(class) } >a AT>d
+  .a05 : { CLASS(class) } >a AT>d
+  .a06 : { CLASS(class) } >a AT>d
+  .a07 : { CLASS(class) } >a AT>d
+  .a08 : { CLASS(class) } >a AT>d
+  .a09 : { CLASS(class) } >a AT>d
+  .a10 : { CLASS(class) } >a AT>d
+  .a11 : { CLASS(class) } >a AT>d
+  .a12 : { CLASS(class) } >a AT>d
+  .a13 : { CLASS(class) } >a AT>d
+  .a14 : { CLASS(class) } >a AT>d
+  .a15 : { CLASS(class) } >a AT>d
+  .a16 : { CLASS(class) } >a AT>d
+  .a17 : { CLASS(class) } >a AT>d
+  .a18 : { CLASS(class) } >a AT>d
+  .a19 : { CLASS(class) } >a AT>d
+  .a20 : { CLASS(class) } >a AT>d
+  .a21 : { CLASS(class) } >a AT>d
+  .a22 : { CLASS(class) } >a AT>d
+  .a23 : { CLASS(class) } >a AT>d
+  .a24 : { CLASS(class) } >a AT>d
+  .a25 : { CLASS(class) } >a AT>d
+  .a26 : { CLASS(class) } >a AT>d
+  .a27 : { CLASS(class) } >a AT>d
+  .a28 : { CLASS(class) } >a AT>d
+  .a29 : { CLASS(class) } >a AT>d
+  .a30 : { CLASS(class) } >a AT>d
+  .b : { CLASS(class) } >b AT>d
+}
+
+# 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
+
+#--- same-lma-region.lds
+## Spills to the same load region that overflowed do not consume address assignment passes.
+MEMORY {
+  a : ORIGIN = 0, LENGTH = 0
+  b : ORIGIN = 0, LENGTH = 3
+  c : ORIGIN = 3, LENGTH = 3
+  d : ORIGIN = 6, LENGTH = 3
+}
+SECTIONS {
+  CLASS(class) { *(.one_byte_section .two_byte_section) }
+  .a00 : { CLASS(class) } >c AT>a
+  .a01 : { CLASS(class) } >d AT>a
+  .a02 : { CLASS(class) } >d AT>a
+  .a03 : { CLASS(class) } >d AT>a
+  .a04 : { CLASS(class) } >d AT>a
+  .a05 : { CLASS(class) } >d AT>a
+  .a06 : { CLASS(class) } >d AT>a
+  .a07 : { CLASS(class) } >d AT>a
+  .a08 : { CLASS(class) } >d AT>a
+  .a09 : { CLASS(class) } >d AT>a
+  .a10 : { CLASS(class) } >d AT>a
+  .a11 : { CLASS(class) } >d AT>a
+  .a12 : { CLASS(class) } >d AT>a
+  .a13 : { CLASS(class) } >d AT>a
+  .a14 : { CLASS(class) } >d AT>a
+  .a15 : { CLASS(class) } >d AT>a
+  .a16 : { CLASS(class) } >d AT>a
+  .a17 : { CLASS(class) } >d AT>a
+  .a18 : { CLASS(class) } >d AT>a
+  .a19 : { CLASS(class) } >d AT>a
+  .a20 : { CLASS(class) } >d AT>a
+  .a21 : { CLASS(class) } >d AT>a
+  .a22 : { CLASS(class) } >d AT>a
+  .a23 : { CLASS(class) } >d AT>a
+  .a24 : { CLASS(class) } >d AT>a
+  .a25 : { CLASS(class) } >d AT>a
+  .a26 : { CLASS(class) } >d AT>a
+  .a27 : { CLASS(class) } >d AT>a
+  .a28 : { CLASS(class) } >d AT>a
+  .a29 : { CLASS(class) } >d AT>a
+  .a30 : { CLASS(class) } >d AT>a
+  .b : { CLASS(class) } >d AT>b
+}
+
+# RUN: ld.lld -T same-lma-region.lds -o same-lma-region spill.o
+# RUN: llvm-readelf -S same-lma-region | FileCheck %s --check-prefix=SAME-LMA-REGION
+
+# SAME-LMA-REGION:      Name          Type     Address          Off    Size
+# SAME-LMA-REGION:      .b PROGBITS 0000000000000006 001006 000003

@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2025

@llvm/pr-subscribers-lld-elf

Author: Daniel Thornburgh (mysterymath)

Changes

Relands #129795

Remove skipped potential spills from their parent input section descriptions to prevent undefined behavior.

Full diff: https://github.com/llvm/llvm-project/pull/130851.diff

2 Files Affected:

  • (modified) lld/ELF/LinkerScript.cpp (+38-9)
  • (modified) lld/test/ELF/linkerscript/section-class.test (+100)
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 120f5271cf229..0d5b383f4b596 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/")) {
@@ -1559,6 +1559,8 @@ bool LinkerScript::spillSections() {
   if (potentialSpillLists.empty())
     return false;
 
+  DenseSet<PotentialSpillSection *> skippedSpills;
+
   bool spilled = false;
   for (SectionCommand *cmd : reverse(sectionCommands)) {
     auto *osd = dyn_cast<OutputDesc>(cmd);
@@ -1585,16 +1587,34 @@ 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.
+        auto canSpillHelp = [&](PotentialSpillSection *spill) {
+          // Spills to the same region that overflowed cannot help.
+          if (hasRegionOverflowed(osec->memRegion) &&
+              spill->getParent()->memRegion == osec->memRegion)
+            return false;
+          if (hasRegionOverflowed(osec->lmaRegion) &&
+              spill->getParent()->lmaRegion == osec->lmaRegion)
+            return false;
+          return true;
+        };
         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);
+          if (canSpillHelp(spill))
+            break;
+          skippedSpills.insert(spill);
+        }
+        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
@@ -1629,6 +1649,15 @@ bool LinkerScript::spillSections() {
     }
   }
 
+  // Clean up any skipped spills.
+  DenseSet<InputSectionDescription *> isds;
+  for (PotentialSpillSection *s : skippedSpills)
+    isds.insert(s->isd);
+  for (InputSectionDescription *isd : isds)
+    llvm::erase_if(isd->sections, [&](InputSection *s) {
+      return skippedSpills.contains(dyn_cast<PotentialSpillSection>(s));
+    });
+
   return spilled;
 }
 
diff --git a/lld/test/ELF/linkerscript/section-class.test b/lld/test/ELF/linkerscript/section-class.test
index 5c30b6343653e..27273f3855c32 100644
--- a/lld/test/ELF/linkerscript/section-class.test
+++ b/lld/test/ELF/linkerscript/section-class.test
@@ -450,3 +450,103 @@ 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 that overflowed do not consume address assignment passes.
+MEMORY {
+  a : ORIGIN = 0, LENGTH = 0
+  b : ORIGIN = 0, LENGTH = 3
+  c : ORIGIN = 3, LENGTH = 3
+  d : ORIGIN = 6, LENGTH = 3
+}
+SECTIONS {
+  CLASS(class) { *(.one_byte_section .two_byte_section) }
+  .a00 : { CLASS(class) } >a AT>c
+  .a01 : { CLASS(class) } >a AT>d
+  .a02 : { CLASS(class) } >a AT>d
+  .a03 : { CLASS(class) } >a AT>d
+  .a04 : { CLASS(class) } >a AT>d
+  .a05 : { CLASS(class) } >a AT>d
+  .a06 : { CLASS(class) } >a AT>d
+  .a07 : { CLASS(class) } >a AT>d
+  .a08 : { CLASS(class) } >a AT>d
+  .a09 : { CLASS(class) } >a AT>d
+  .a10 : { CLASS(class) } >a AT>d
+  .a11 : { CLASS(class) } >a AT>d
+  .a12 : { CLASS(class) } >a AT>d
+  .a13 : { CLASS(class) } >a AT>d
+  .a14 : { CLASS(class) } >a AT>d
+  .a15 : { CLASS(class) } >a AT>d
+  .a16 : { CLASS(class) } >a AT>d
+  .a17 : { CLASS(class) } >a AT>d
+  .a18 : { CLASS(class) } >a AT>d
+  .a19 : { CLASS(class) } >a AT>d
+  .a20 : { CLASS(class) } >a AT>d
+  .a21 : { CLASS(class) } >a AT>d
+  .a22 : { CLASS(class) } >a AT>d
+  .a23 : { CLASS(class) } >a AT>d
+  .a24 : { CLASS(class) } >a AT>d
+  .a25 : { CLASS(class) } >a AT>d
+  .a26 : { CLASS(class) } >a AT>d
+  .a27 : { CLASS(class) } >a AT>d
+  .a28 : { CLASS(class) } >a AT>d
+  .a29 : { CLASS(class) } >a AT>d
+  .a30 : { CLASS(class) } >a AT>d
+  .b : { CLASS(class) } >b AT>d
+}
+
+# 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
+
+#--- same-lma-region.lds
+## Spills to the same load region that overflowed do not consume address assignment passes.
+MEMORY {
+  a : ORIGIN = 0, LENGTH = 0
+  b : ORIGIN = 0, LENGTH = 3
+  c : ORIGIN = 3, LENGTH = 3
+  d : ORIGIN = 6, LENGTH = 3
+}
+SECTIONS {
+  CLASS(class) { *(.one_byte_section .two_byte_section) }
+  .a00 : { CLASS(class) } >c AT>a
+  .a01 : { CLASS(class) } >d AT>a
+  .a02 : { CLASS(class) } >d AT>a
+  .a03 : { CLASS(class) } >d AT>a
+  .a04 : { CLASS(class) } >d AT>a
+  .a05 : { CLASS(class) } >d AT>a
+  .a06 : { CLASS(class) } >d AT>a
+  .a07 : { CLASS(class) } >d AT>a
+  .a08 : { CLASS(class) } >d AT>a
+  .a09 : { CLASS(class) } >d AT>a
+  .a10 : { CLASS(class) } >d AT>a
+  .a11 : { CLASS(class) } >d AT>a
+  .a12 : { CLASS(class) } >d AT>a
+  .a13 : { CLASS(class) } >d AT>a
+  .a14 : { CLASS(class) } >d AT>a
+  .a15 : { CLASS(class) } >d AT>a
+  .a16 : { CLASS(class) } >d AT>a
+  .a17 : { CLASS(class) } >d AT>a
+  .a18 : { CLASS(class) } >d AT>a
+  .a19 : { CLASS(class) } >d AT>a
+  .a20 : { CLASS(class) } >d AT>a
+  .a21 : { CLASS(class) } >d AT>a
+  .a22 : { CLASS(class) } >d AT>a
+  .a23 : { CLASS(class) } >d AT>a
+  .a24 : { CLASS(class) } >d AT>a
+  .a25 : { CLASS(class) } >d AT>a
+  .a26 : { CLASS(class) } >d AT>a
+  .a27 : { CLASS(class) } >d AT>a
+  .a28 : { CLASS(class) } >d AT>a
+  .a29 : { CLASS(class) } >d AT>a
+  .a30 : { CLASS(class) } >d AT>a
+  .b : { CLASS(class) } >d AT>b
+}
+
+# RUN: ld.lld -T same-lma-region.lds -o same-lma-region spill.o
+# RUN: llvm-readelf -S same-lma-region | FileCheck %s --check-prefix=SAME-LMA-REGION
+
+# SAME-LMA-REGION:      Name          Type     Address          Off    Size
+# SAME-LMA-REGION:      .b PROGBITS 0000000000000006 001006 000003

@mysterymath mysterymath deleted the lld-spill-bug branch March 11, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lld:ELF lld
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mysterymath @llvmbot