[ELF] Orphan placement: remove hasInputSections condition #93761
Conversation
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) Changeshttps://reviews.llvm.org/D60131 (Change default output section type to https://reviews.llvm.org/D61197 (Fix getRankProximity to "ignore" not This condition added complexity, which turns out to be unneeded after
Full diff: https://github.com/llvm/llvm-project/pull/93761.diff 6 Files Affected:
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index c498153f3348b..4add995c93458 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -890,9 +890,7 @@ template <class ELFT> void Writer<ELFT>::setReservedSymbolSections() {
// countLeadingZeros.
static int getRankProximity(OutputSection *a, SectionCommand *b) {
auto *osd = dyn_cast<OutputDesc>(b);
- return (osd && osd->osec.hasInputSections)
- ? llvm::countl_zero(a->sortRank ^ osd->osec.sortRank)
- : -1;
+ return osd ? llvm::countl_zero(a->sortRank ^ osd->osec.sortRank) : -1;
}
// When placing orphan sections, we want to place them after symbol assignments
@@ -958,20 +956,16 @@ findOrphanPos(SmallVectorImpl<SectionCommand *>::iterator b,
sortRank = std::max(sortRank, foundSec->sortRank);
for (; i != e; ++i) {
auto *curSecDesc = dyn_cast<OutputDesc>(*i);
- if (!curSecDesc || !curSecDesc->osec.hasInputSections)
+ if (!curSecDesc)
continue;
if (getRankProximity(sec, curSecDesc) != proximity ||
sortRank < curSecDesc->osec.sortRank)
break;
}
- auto isOutputSecWithInputSections = [](SectionCommand *cmd) {
- auto *osd = dyn_cast<OutputDesc>(cmd);
- return osd && osd->osec.hasInputSections;
- };
- auto j =
- std::find_if(std::make_reverse_iterator(i), std::make_reverse_iterator(b),
- isOutputSecWithInputSections);
+ auto isOutputSec = [](SectionCommand *cmd) { return isa<OutputDesc>(cmd); };
+ auto j = std::find_if(std::make_reverse_iterator(i),
+ std::make_reverse_iterator(b), isOutputSec);
i = j.base();
// As a special case, if the orphan section is the last section, put
@@ -979,7 +973,7 @@ findOrphanPos(SmallVectorImpl<SectionCommand *>::iterator b,
// This matches bfd's behavior and is convenient when the linker script fully
// specifies the start of the file, but doesn't care about the end (the non
// alloc sections for example).
- auto nextSec = std::find_if(i, e, isOutputSecWithInputSections);
+ auto nextSec = std::find_if(i, e, isOutputSec);
if (nextSec == e)
return e;
diff --git a/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test b/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
index 2dcd0f8d6ce2f..9c6111008c818 100644
--- a/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
+++ b/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
@@ -16,22 +16,21 @@
## The output file must include all sections.
# RUN: llvm-readelf -S %t/a.elf | FileCheck %s
-# CHECK: There are 12 section headers, starting at offset 0x2140:
+# CHECK: There are 12 section headers, starting at offset 0x2138:
# CHECK: [Nr] Name Type Address Off Size ES Flg Lk Inf Al
# CHECK-NEXT: [ 0] NULL 0000000000000000 000000 000000 00 0 0 0
# CHECK-NEXT: [ 1] .nonalloc PROGBITS 0000000000000000 001064 001000 00 W 0 0 1
-# CHECK-NEXT: [ 2] .comment PROGBITS 0000000000000000 {{.*}} {{.*}} 01 MS 0 0 1
-# CHECK-NEXT: [ 3] .symtab SYMTAB 0000000000000000 {{.*}} {{.*}} 18 5 1 8
-# CHECK-NEXT: [ 4] .shstrtab STRTAB 0000000000000000 {{.*}} {{.*}} 00 0 0 1
-# CHECK-NEXT: [ 5] .strtab STRTAB 0000000000000000 {{.*}} {{.*}} 00 0 0 1
-# CHECK-NEXT: [ 6] .dat PROGBITS 0000000000000000 002137 000004 00 W 0 0 1
-# CHECK-NEXT: [ 7] .intvec0_out PROGBITS 0000000000000000 00213b 000000 00 W 0 0 1
-# CHECK-NEXT: [ 8] .intvec1_out PROGBITS 0000000000000000 00213b 000000 00 W 0 0 1
-# CHECK-NEXT: [ 9] .intvec2_out PROGBITS 0000000000000000 00213b 000000 00 W 0 0 1
+# CHECK-NEXT: [ 2] .dat PROGBITS 0000000000000000 002064 000004 00 W 0 0 1
+# CHECK-NEXT: [ 3] .intvec0_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1
+# CHECK-NEXT: [ 4] .intvec1_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1
+# CHECK-NEXT: [ 5] .intvec2_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1
+# CHECK-NEXT: [ 6] .comment PROGBITS 0000000000000000 {{.*}} {{.*}} 01 MS 0 0 1
+# CHECK-NEXT: [ 7] .symtab SYMTAB 0000000000000000 {{.*}} {{.*}} 18 9 1 8
+# CHECK-NEXT: [ 8] .shstrtab STRTAB 0000000000000000 {{.*}} {{.*}} 00 0 0 1
+# CHECK-NEXT: [ 9] .strtab STRTAB 0000000000000000 {{.*}} {{.*}} 00 0 0 1
# CHECK-NEXT: [10] .intvec3_out PROGBITS 00000000803fe060 001060 000004 00 AX 0 0 1
# CHECK-NEXT: [11] .text PROGBITS 00000000803fe064 001064 000000 00 AX 0 0 4
-
#--- a.s
.global _start
.text
diff --git a/lld/test/ELF/linkerscript/nobits-offset.s b/lld/test/ELF/linkerscript/nobits-offset.s
index 387eaed4cb146..b305cec6fb699 100644
--- a/lld/test/ELF/linkerscript/nobits-offset.s
+++ b/lld/test/ELF/linkerscript/nobits-offset.s
@@ -14,8 +14,8 @@
# CHECK: Name Type Address Off Size
# CHECK-NEXT: NULL 0000000000000000 000000 000000
-# CHECK-NEXT: .text PROGBITS 0000000000000000 000190 000000
# CHECK-NEXT: .sec1 NOBITS 0000000000000000 001000 000001
+# CHECK-NEXT: .text PROGBITS 0000000000000004 001000 000000
# CHECK-NEXT: .bss NOBITS 0000000000000400 001400 000001
# CHECK: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
diff --git a/lld/test/ELF/linkerscript/orphan-live-only.s b/lld/test/ELF/linkerscript/orphan-live-only.s
index 500005173a866..900532e0eab61 100644
--- a/lld/test/ELF/linkerscript/orphan-live-only.s
+++ b/lld/test/ELF/linkerscript/orphan-live-only.s
@@ -20,14 +20,14 @@
# CHECK: Section Headers
# CHECK: .pad
+# CHECK-NEXT: .orphan1
# CHECK-NEXT: .text
# CHECK-NEXT: .orphan2
# CHECK-NEXT: .ro
-# CHECK-NEXT: .orphan1
# CHECK: Segment Sections
-# CHECK-NEXT: .pad .text .orphan2
-# CHECK-NEXT: .ro .orphan1
+# CHECK-NEXT: .pad .orphan1 .text .orphan2
+# CHECK-NEXT: .ro
.section .text,"ax"
ret
diff --git a/lld/test/ELF/linkerscript/symbol-only-align.test b/lld/test/ELF/linkerscript/symbol-only-align.test
index f4ecf275ec0bd..c553e8138b5eb 100644
--- a/lld/test/ELF/linkerscript/symbol-only-align.test
+++ b/lld/test/ELF/linkerscript/symbol-only-align.test
@@ -23,13 +23,14 @@ SECTIONS {
## offset and addresses match.
# CHECK: Section Headers
-# CHECK: foo PROGBITS 0000000000[[ADDR:[0-9a-f]*]] [[ADDR]]
-# CHECK-NEXT: .data PROGBITS 0000000000[[ADDR]] [[ADDR]]
+# CHECK: .text PROGBITS
+# CHECK-NEXT: foo PROGBITS 0000000000001000 001000 000000
+# CHECK-NEXT: .dynsym DYNSYM 0000000000001000 001000
# CHECK: Program Headers
# CHECK: LOAD
-# CHECK-NEXT: LOAD 0x[[ADDR]] 0x0000000000[[ADDR]] 0x0000000000[[ADDR]]
+# CHECK-NEXT: LOAD 0x001000 0x0000000000001000 0x0000000000001000
# CHECK: Symbol table
-# CHECK: 0000000000[[ADDR]] 0 NOTYPE GLOBAL DEFAULT {{[0-9]+}} __start_foo
-# CHECK: 0000000000[[ADDR]] 0 NOTYPE GLOBAL DEFAULT {{[0-9]+}} __end_foo
+# CHECK: 0000000000001000 0 NOTYPE GLOBAL DEFAULT [[#]] __start_foo
+# CHECK: 0000000000001000 0 NOTYPE GLOBAL DEFAULT [[#]] __end_foo
diff --git a/lld/test/ELF/linkerscript/tls-nobits-offset.s b/lld/test/ELF/linkerscript/tls-nobits-offset.s
index 9aab32317be4c..7830b457e1b4d 100644
--- a/lld/test/ELF/linkerscript/tls-nobits-offset.s
+++ b/lld/test/ELF/linkerscript/tls-nobits-offset.s
@@ -14,8 +14,8 @@
# CHECK: Name Type Address Off Size
# CHECK-NEXT: NULL 0000000000000000 000000 000000
-# CHECK-NEXT: .text PROGBITS 0000000000000000 000190 000000
# CHECK-NEXT: .sec1 PROGBITS 0000000000000000 001000 000001
+# CHECK-NEXT: .text PROGBITS 0000000000000004 001004 000000
# CHECK-NEXT: .tbss NOBITS 0000000000000400 001400 000001
# CHECK: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
|
|
The description contains a lot of links to previous patches, but they are not strictly needed to understand this patch. In short, the previous |
There was a problem hiding this comment.
Basically, the linker now takes into account sections defined in a linker script when placing orphans, right? What if some kind of padding/filler section is defined in the script? It looks like it will attract orphan data sections when placed before actual data sections. Do you think a new RankFlag can be added to make real sections more preferable?
Yes.
We rely on // adjustOutputSections
if (isEmpty) {
sec->flags =
flags & ((sec->nonAlloc ? 0 : (uint64_t)SHF_ALLOC) | SHF_WRITE);
sec->sortRank = getSectionRank(*sec);
}
Orphan placement seems primarily focused on choosing a suitable region (R/RX/RW/non-ALLOC). A |
| # CHECK-NEXT: .pad .text .orphan2 | ||
| # CHECK-NEXT: .ro .orphan1 | ||
| # CHECK-NEXT: .pad .orphan1 .text .orphan2 | ||
| # CHECK-NEXT: .ro |
There was a problem hiding this comment.
This change of placement of .orphan1 does not look correct given that its flags are "a".
There was a problem hiding this comment.
IIRC, I think it was this behaviour that was the reason for the Live patch from https://reviews.llvm.org/D61197, i.e. to ignore .pad when placing orphans as stated by the comment on line 15 in this test.
There was a problem hiding this comment.
Only just read @igorkudrin's comment which basically explains why the above has changed. If a new RankFlag is preferable to retaining the use of hasInputSections() then +1 from me. The key thing from our point of view is that the behaviour in this test, orphan-live-only.s is maintained.
There was a problem hiding this comment.
w/o this patch; .orphan1 is after .ro (the second A, but the first A that contains input sections).
[ 1] .pad PROGBITS 0000000000000000 001000 000008 00 A 0 0 1
[ 2] .text PROGBITS 0000000000000008 001008 000001 00 AX 0 0 4
[ 3] .orphan2 PROGBITS 0000000000000009 001009 000001 00 AX 0 0 1
[ 4] .ro PROGBITS 000000000000000a 00100a 000001 00 A 0 0 1
[ 5] .orphan1 PROGBITS 000000000000000b 00100b 000001 00 A 0 0 1
w/ this patch; .orphan1 is after .ro (the first A).
[ 1] .pad PROGBITS 0000000000000000 001000 000008 00 A 0 0 1
[ 2] .orphan1 PROGBITS 0000000000000008 001008 000001 00 A 0 0 1
[ 3] .text PROGBITS 000000000000000c 00100c 000001 00 AX 0 0 4
[ 4] .orphan2 PROGBITS 000000000000000d 00100d 000001 00 AX 0 0 1
[ 5] .ro PROGBITS 000000000000000e 00100e 000001 00 A 0 0 1
I think the new behavior is more reasonable.
The key thing from our point of view is that the behaviour in this test, orphan-live-only.s is maintained.
orphan-live-only.s might be overly reduced. Is there a better justification for the skip-padding-section behavior?
Currently output sections without an input section rely heavily on the flags propagation rule.
With the introduction of osec (READONLY) : { } and possible future addition for SHF_EXECINSTR, I think these sections should be used as an anchor as well.
For the reasonable use cases, WA pad WA pad WA, w/o or w/ this patch, a new WA section will be placed at the end of the consecutive WA/pad sequence.
The same applies to A pad A pad A.
Therefore, I don't think a new RankFlag is needed.
|
Will take a look next week if it hasn't already been approved. Apologies out of the office till the end of the week. |
My concerns are for cases like: Of course, this example is oversimplified, but I see something similar in our projects. For this sample, GNU linkers and current |
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thanks. Take your time!
Thanks for the example. This and #92987 convince me that we should do #94099 (when there are multiple most similar sections, select the last one if its rank <= orphan's rank). With #94099, this patch will not cause a test change and your I think this patch is still controversial. #94099 is probably a clear improvement. |
Rebased after #94099 I think this simplification is applicable. |
There was a problem hiding this comment.
I found that llvm-project/lld/test/ELF/linkerscript/orphan-live-only.s failed after applying this patch. It looks like this was mentioned in an earlier comment, but it doesn't look like that change has made it into the tests. I could be doing something wrong, so please ignore me if I have.
I agree that the change to orphan-live-only.s is reasonable.
I've also run the bin/ld.lld -shared test.o -T test.t -o test.so from an earlier comment and that also gives a good result with the change
I think the non-alloc synthetic section placement could do with some improvement but I think that would have to be done with a separate patch.
| # CHECK-NEXT: [ 3] .intvec0_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1 | ||
| # CHECK-NEXT: [ 4] .intvec1_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1 | ||
| # CHECK-NEXT: [ 5] .intvec2_out PROGBITS 0000000000000000 002068 000000 00 W 0 0 1 | ||
| # CHECK-NEXT: [ 6] .comment PROGBITS 0000000000000000 {{.*}} {{.*}} 01 MS 0 0 1 |
There was a problem hiding this comment.
This makes me think we haven't got the rank order quite right, or we need a special case for non-alloc (possibly even synthetic non-alloc) sections so that these go at the end.
For example GNU ld (which removes intvec0_out to intvec2_out) gives:
Section Headers:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al
[ 0] NULL 0000000000000000 000000 000000 00 0 0 0
[ 1] .nonalloc PROGBITS 0000000000000000 001064 001000 00 W 0 0 1
[ 2] .dat PROGBITS 0000000010000000 001000 000004 00 A 0 0 1
[ 3] .intvec3_out PROGBITS 00000000803fe060 001060 000004 00 AX 0 0 1
[ 4] .symtab SYMTAB 0000000000000000 002068 000048 18 5 1 8
[ 5] .strtab STRTAB 0000000000000000 0020b0 000012 00 0 0 1
[ 6] .shstrtab STRTAB 0000000000000000 0020c2 000037 00 0 0 1
I think it is better than it was before though.
There was a problem hiding this comment.
Agreed. I was thinking of a special case at the while (nonScriptI != e) (near Writer.cpp:1313) loop that we keep non-SHF_ALLOC orphan at the end.
It is probably still worth doing.
| @@ -16,22 +16,21 @@ | |||
| ## The output file must include all sections. | |||
| # RUN: llvm-readelf -S %t/a.elf | FileCheck %s | |||
|
|
|||
| # CHECK: There are 12 section headers, starting at offset 0x2140: | |||
| # CHECK: There are 12 section headers, starting at offset 0x2138: | |||
| # CHECK: [Nr] Name Type Address Off Size ES Flg Lk Inf Al | |||
| # CHECK-NEXT: [ 0] NULL 0000000000000000 000000 000000 00 0 0 0 | |||
| # CHECK-NEXT: [ 1] .nonalloc PROGBITS 0000000000000000 001064 001000 00 W 0 0 1 | |||
There was a problem hiding this comment.
I can't comment on the line, this is a forward reference. In the linker script
.nonalloc : { *(.nonalloc) } > MEM
MEM does not exist, only MEM and MEM1. GNU ld gives a warning in this case
ld.bfd:/data_nvme0n1/work/llvm-project/build/tools/lld/test/ELF/linkerscript/Output/memory-nonalloc-no-warn.test.tmp/a.lds:10: warning: memory region `MEM' not declared
As a noalloc section MEM may have been intentional, but my guess is that it was a typo.
There was a problem hiding this comment.
The difference in section header table offsets is due to less alignment padding by coincidence.
.nonalloc : { *(.nonalloc) } > MEM
lld reports warning: ignoring memory region assignment for non-allocatable section '.nonalloc'.
lld also supports an error (e.g. error: memory region 'ram' not declared for memory-err.s) but its precedence is lower than the non-allocatable warning.
There was a problem hiding this comment.
With #93761 , this patch will not need to modify this test file, but GitHub PR doesn't seem to make it clear.
Created using spr 1.3.5-bogner
The description mentioned
Perhaps we do need a |
Created using spr 1.3.5-bogner [skip ci]
Created using spr 1.3.5-bogner
MaskRay
changed the base branch from
main
to
users/MaskRay/spr/main.elf-orphan-placement-remove-hasinputsections-condition
|
I think #94519 looks good. Probably worth landing that first and rebasing this one. |
https://reviews.llvm.org/D85867 changed the way we assign file offsets (alloc sections first, then non-alloc sections). It also removed a non-alloc special case from `findOrphanPos`. Looking at the memory-nonalloc-no-warn.test change, which would be needed by #93761, it makes sense to restore the previous behavior: when placing non-alloc orphan sections, keep these sections at the end so that the section index order matches the file offset order. This change is cosmetic. In sections-nonalloc.s, GNU ld places the orphan `other3` in the middle and the orphan .symtab/.shstrtab/.strtab at the end. Pull Request: #94519
Created using spr 1.3.5-bogner [skip ci]
MaskRay
changed the base branch from
users/MaskRay/spr/main.elf-orphan-placement-remove-hasinputsections-condition
to
main
|
Rebased. Since output sections with |
Maybe I'm missing something, but the latest revision of this patch with my example still places synthetic sections in an executable segment. Prioritizing the last section of the same similarity (#94099) does little to address the core problem I was trying to demonstrate: output sections without input sections have no reliable flags, and thus when used as potential anchors for orphan sections, the output can be unexpected; another example can easily be conjured up where such sections occur in the last segment, attracting orphans there. So for me, this patch favors simplification over correctness, and I would stick with the latter, especially since the proposed simplification does not seem critical. |
I think the new behavior is expected. Use the example at #93761 (comment). the section layout after this patch is: If |
No other linkers put Moreover, if we change the linker script, following the idea of the result will be As you can see, the problem here is exactly the same. |
(I use GNU ld's rank system does not differentiate .dynsym and PROGBITS read-only sections. lld has a more fine-grained rank system and prefers to place .dynsym after the first read-only PROGBITS section, when PHDRS/MEMORY is specified.
What's the problem? |
Oh, I see, I was too concise. I meant that input sections Our usage scenario is not a dedicated linker script carefully prepared for a particular application. It is rather a platform/SDK that provides a set of predefined linker scripts, and all developers should use one of them for their applications; it is generally not possible for a developer to tune the linker scripts they use. Also, it doesn't seem feasible to specify all possible input sections in the linker scripts, so we rely on a predictable placement of orphan sections. Thus, I would prefer not to fix what is not broken, unless there is a clear benefit to doing so. |
OK, your argument is that .other ( I think both arguments are reasonable and no one is significantly better than the other. .dynsym has a different rank than PROGBITS read-only sections and deserves an explicit output section to make a portable script. |
Not exactly. The output section '.other' could be I prefer to follow the principle of least astonishment here.
I agree. Which explanations for these two cases do you think of? |
|
Is it possible to assign more suitable attributes to output sections without input sections? Maybe take them from the closest output sections of the same segment? |
|
I have sympathy with MaskRay's position in that it is a somewhat arbitrary rule that OutputSections can't affect orphan placement, and may result in surprise given the existing GNU ld documentation (https://sourceware.org/binutils/docs/ld/Orphan-Sections.html). Having said that I think we may want to define what type and flags our OutputSections without InputSections get their type to given people more stability. My understanding of GNU ld is that it will by default give the OutputSection SHT_PROGBITS type and "RAW" flags. This can be altered to READONLY with recent releses, although I don't think LLD supports that. Empiricially LLD seems to inherit the type and flags from a previous OutputSection (I've not checked the code). This largely seems to do the right thing although I'm not convinced that inheriting the "x" flag is the right thing to do. My, little thought, opening bid would be something like:
However that is separate from this pull request. To progress this one do we have an example of one of these sample linker scripts having a problematic Output Section acting as an anchor for orphans, or is it a theoretical risk? |
So far, the risk is theoretical. I just don't like it when data can be written to an executable segment unexpectedly. Since |
Thanks!
GNU ld since Sep 2020 defaults to "RA" flags. Older versions used "RAW". Orphan sections use the default type GNU ld introduced "READONLY" in July 2021 (https://sourceware.org/pipermail/binutils/2021-July/117492.html)
We inherit I have also considered that flags setting syntax can help guide orphan placement.
This ( Considering this and our shift towards explicit flags syntax, removing |
https://reviews.llvm.org/D60131 (Change default output section type to
SHT_PROGBITS) caused a orphan placement regression for Fuchsia
zircon.elf: #40998 The orphan sectioncode_patch_tablewas placedbefore the first output section description
.text.boot0, breaking theaddress requirement.
https://reviews.llvm.org/D61197 (Fix getRankProximity to "ignore" not
live sections) fixed the regression by adding a
Livecondition (whichlater became
hasInputSections).This condition added complexity, which turns out to be unneeded after
The new orphan placement rule is slightly different (orphans can be
placed after an output section that does not contain input sections),
but simpler and probably more reasonable.
.sec1 (NOLOAD) : { . += 1; }foowithout an input section gets the flagsA.The orphan
.dynsymcan now be placed after it.(Due to
PHDRS,.dynsymwith a lower rank must be placed after the anchor.)anchor (.sec1)'s and is placed after the anchor.
memory-nonalloc-no-warn.test would need a change if we don't apply
#94519.