[LLD][ELF] Fix SHF_MERGE misalignment when spilled #119289
Conversation
Section merging can increase section alignment after potential spill sections are created. Since this operation is never performed on spill sections, they can keep their earlier, smaller, alignment, which produces a misalignment if a spill occurs. This change propagates alignment increases forward after merging.
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Daniel Thornburgh (mysterymath) ChangesSection merging can increase section alignment after potential spill sections are created. Since this operation is never performed on spill sections, they can keep their earlier, smaller, alignment, which produces a misalignment if a spill occurs. This change propagates alignment increases forward after merging. Full diff: https://github.com/llvm/llvm-project/pull/119289.diff 2 Files Affected:
diff --git a/lld/ELF/OutputSections.cpp b/lld/ELF/OutputSections.cpp
index 546dc58b4bc843..5e77eeb1ed5988 100644
--- a/lld/ELF/OutputSections.cpp
+++ b/lld/ELF/OutputSections.cpp
@@ -249,12 +249,27 @@ void OutputSection::finalizeInputSections() {
// catch misuses.
isd->sectionBases.clear();
+
// Some input sections may be removed from the list after ICF.
for (InputSection *s : isd->sections)
commitSection(s);
}
- for (auto *ms : mergeSections)
+ for (auto *ms : mergeSections) {
+ // Merging may have increased the alignment of a spillable section. Update
+ // the alignment of potential spill sections and their containing output
+ // sections.
+ if (!script->potentialSpillLists.empty()) {
+ if (auto it = script->potentialSpillLists.find(ms);
+ it != script->potentialSpillLists.end()) {
+ for (PotentialSpillSection *s = it->second.head; s; s = s->next) {
+ s->addralign = std::max(s->addralign, ms->addralign);
+ s->parent->addralign = std::max(s->parent->addralign, s->addralign);
+ }
+ }
+ }
+
ms->finalizeContents();
+ }
}
static void sortByOrder(MutableArrayRef<InputSection *> in,
diff --git a/lld/test/ELF/linkerscript/section-class.test b/lld/test/ELF/linkerscript/section-class.test
index 7fce13bfe3e025..ae1a5026ef0490 100644
--- a/lld/test/ELF/linkerscript/section-class.test
+++ b/lld/test/ELF/linkerscript/section-class.test
@@ -310,6 +310,7 @@ SECTIONS {
.byte 0x12, 0x34
.section .b,"aM",@progbits,1
+.p2align 1
.byte 0x12
# RUN: llvm-mc -n -filetype=obj -triple=x86_64 merge.s -o merge.o
@@ -317,10 +318,11 @@ SECTIONS {
#--- spill-merge.lds
## SHF_MERGE sections are spilled according to the class refs of the first
## merged input section (the one giving the resulting section its name).
+## Spills take into account increases in section alignment due to merging.
MEMORY {
a : ORIGIN = 0, LENGTH = 1
- b : ORIGIN = 1, LENGTH = 2
- c : ORIGIN = 3, LENGTH = 2
+ b : ORIGIN = 1, LENGTH = 4
+ c : ORIGIN = 5, LENGTH = 4
}
SECTIONS {
@@ -336,8 +338,8 @@ SECTIONS {
# SPILL-MERGE: Name Type Address Off Size
# SPILL-MERGE: .first PROGBITS 0000000000000000 000190 000000
-# SPILL-MERGE-NEXT: .second PROGBITS 0000000000000001 001001 000002
-# SPILL-MERGE-NEXT: .third PROGBITS 0000000000000003 001003 000000
+# SPILL-MERGE-NEXT: .second PROGBITS 0000000000000002 001002 000003
+# SPILL-MERGE-NEXT: .third PROGBITS 0000000000000006 001006 000000
#--- link-order.s
.section .a,"a",@progbits
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Prabhuk
left a comment
Prabhuk
left a comment
There was a problem hiding this comment.
Looks good to me. But I'd wait for at least one of the LLD experts to chime in.
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
LGTM too.
One thing that made me think is the update to the OutputSection addralign. I initially thought that we'd be at risk of increasing OutputSection alignment for the PotentialSections that aren't used, however on reflection I think we have to do that to make sure we converge.
lld/ELF/OutputSections.cpp
Outdated
| // Merging may have increased the alignment of a spillable section. Update | ||
| // the alignment of potential spill sections and their containing output | ||
| // sections. | ||
| if (!script->potentialSpillLists.empty()) { |
| .byte 0x12, 0x34 | ||
|
|
||
| .section .b,"aM",@progbits,1 | ||
| .p2align 1 |
There was a problem hiding this comment.
An alignment of 2 works, but a larger alignment makes the effect clearer.
There was a problem hiding this comment.
We also need to dump the section content (e.g. -x second )
5 participants
Section merging can increase section alignment after potential spill sections are created. Since this operation is never performed on spill sections, they can keep their earlier, smaller, alignment, which produces a misalignment if a spill occurs.
This change propagates alignment increases forward after merging.