[ELF] Keep non-alloc orphan sections at the end #94519

MaskRay · 2024-06-05T18:34:58Z

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.

Created using spr 1.3.5-bogner

llvmbot · 2024-06-05T18:35:30Z

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

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.

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

4 Files Affected:

(modified) lld/ELF/Writer.cpp (+5-1)
(modified) lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test (+11-11)
(modified) lld/test/ELF/linkerscript/sections-nonalloc.s (+14-14)
(modified) lld/test/ELF/linkerscript/sections.s (+2-2)

diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 0295a656b0705..ff6d9beb3731e 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -921,7 +921,11 @@ static bool shouldSkip(SectionCommand *cmd) {
 static SmallVectorImpl<SectionCommand *>::iterator
 findOrphanPos(SmallVectorImpl<SectionCommand *>::iterator b,
               SmallVectorImpl<SectionCommand *>::iterator e) {
+  // Place non-alloc orphan sections at the end. This matches how we assign file
+  // offsets to non-alloc sections.
   OutputSection *sec = &cast<OutputDesc>(*e)->osec;
+  if (!(sec->flags & SHF_ALLOC))
+    return e;
 
   // As a special case, place .relro_padding before the SymbolAssignment using
   // DATA_SEGMENT_RELRO_END, if present.
@@ -1316,8 +1320,8 @@ template <class ELFT> void Writer<ELFT>::sortOrphanSections() {
   i = firstSectionOrDotAssignment;
 
   while (nonScriptI != e) {
-    auto pos = findOrphanPos(i, nonScriptI);
     OutputSection *orphan = &cast<OutputDesc>(*nonScriptI)->osec;
+    auto pos = findOrphanPos(i, nonScriptI);
 
     // As an optimization, find all sections with the same sort rank
     // and insert them with one rotate.
diff --git a/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test b/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
index 2dcd0f8d6ce2f..eabdf75fcf935 100644
--- a/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
+++ b/lld/test/ELF/linkerscript/memory-nonalloc-no-warn.test
@@ -16,20 +16,20 @@
 ## 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: [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
+# 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] .intvec3_out PROGBITS  00000000803fe060 001060 000004 00 AX  0  0   1
+# CHECK-NEXT: [ 7] .text        PROGBITS  00000000803fe064 001064 000000 00 AX  0  0   4
+# CHECK-NEXT: [ 8] .comment     PROGBITS  0000000000000000 {{.*}} {{.*}} 01 MS  0  0   1
+# CHECK-NEXT: [ 9] .symtab      SYMTAB    0000000000000000 {{.*}} {{.*}} 18     11 1   8
+# CHECK-NEXT: [10] .shstrtab    STRTAB    0000000000000000 {{.*}} {{.*}} 00     0  0   1
+# CHECK-NEXT: [11] .strtab      STRTAB    0000000000000000 {{.*}} {{.*}} 00     0  0   1
 
 
 #--- a.s
diff --git a/lld/test/ELF/linkerscript/sections-nonalloc.s b/lld/test/ELF/linkerscript/sections-nonalloc.s
index 79765d32dfffe..b4fab8c24cfb4 100644
--- a/lld/test/ELF/linkerscript/sections-nonalloc.s
+++ b/lld/test/ELF/linkerscript/sections-nonalloc.s
@@ -16,15 +16,15 @@
 # CHECK-NEXT:  [ 2] data1     PROGBITS 0000000000000001 001001 000001 00  WA  0
 # CHECK-NEXT:  [ 3] other1    PROGBITS 0000000000000000 001008 000001 00      0
 # CHECK-NEXT:  [ 4] other2    PROGBITS 0000000000000000 001010 000001 00      0
-## Orphan placement places other3, .symtab, .shstrtab and .strtab after other2.
-# CHECK-NEXT:  [ 5] other3    PROGBITS 0000000000000000 001020 000001 00      0
-# CHECK-NEXT:  [ 6] .symtab   SYMTAB   0000000000000000 001028 000030 18      8
-# CHECK-NEXT:  [ 7] .shstrtab STRTAB   0000000000000000 001058 00004d 00      0
-# CHECK-NEXT:  [ 8] .strtab   STRTAB   0000000000000000 0010a5 000008 00      0
-# CHECK-NEXT:  [ 9] data2     PROGBITS 0000000000000002 001002 000001 00  WA  0
+# CHECK-NEXT:  [ 5] data2     PROGBITS 0000000000000002 001002 000001 00  WA  0
 ## max{sortRank(data1),sortRank(data2)} <= sortRank(data3). data3 is placed after the latter.
-# CHECK-NEXT:  [10] data3     PROGBITS 0000000000000003 001003 000001 00  WA  0
-# CHECK-NEXT:  [11] .text     PROGBITS 0000000000000004 001004 000001 00  AX  0
+# CHECK-NEXT:  [ 6] data3     PROGBITS 0000000000000003 001003 000001 00  WA  0
+# CHECK-NEXT:  [ 7] .text     PROGBITS 0000000000000004 001004 000001 00  AX  0
+## Non-alloc orphan sections other3, .symtab, .shstrtab and .strtab are placed at the end.
+# CHECK-NEXT:  [ 8] other3    PROGBITS 0000000000000000 001020 000001 00      0
+# CHECK-NEXT:  [ 9] .symtab   SYMTAB   0000000000000000 001028 000030 18     11
+# CHECK-NEXT:  [10] .shstrtab STRTAB   0000000000000000 001058 00004d 00      0
+# CHECK-NEXT:  [11] .strtab   STRTAB   0000000000000000 0010a5 000008 00      0
 
 # CHECK:       Type       Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # CHECK-NEXT:  LOAD       0x001000 0x0000000000000000 0x0000000000000000 0x000004 0x000004 RW  0x1000
@@ -41,12 +41,12 @@
 # CHECK1-NEXT: [ 3] data1     PROGBITS 00000000000000b2 0000b2 000001 00  WA  0
 # CHECK1-NEXT: [ 4] other1    PROGBITS 0000000000000000 0000b8 000001 00      0
 # CHECK1-NEXT: [ 5] other2    PROGBITS 0000000000000000 0000c0 000001 00      0
-# CHECK1-NEXT: [ 6] other3    PROGBITS 0000000000000000 0000d0 000001 00      0
-# CHECK1-NEXT: [ 7] .symtab   SYMTAB   0000000000000000 0000d8 000030 18      9
-# CHECK1-NEXT: [ 8] .shstrtab STRTAB   0000000000000000 000108 00004d 00      0
-# CHECK1-NEXT: [ 9] .strtab   STRTAB   0000000000000000 000155 000008 00      0
-# CHECK1-NEXT: [10] data2     PROGBITS 00000000000000b3 0000b3 000001 00  WA  0
-# CHECK1-NEXT: [11] data3     PROGBITS 00000000000000b4 0000b4 000001 00  WA  0
+# CHECK1-NEXT: [ 6] data2     PROGBITS 00000000000000b3 0000b3 000001 00  WA  0
+# CHECK1-NEXT: [ 7] data3     PROGBITS 00000000000000b4 0000b4 000001 00  WA  0
+# CHECK1-NEXT: [ 8] other3    PROGBITS 0000000000000000 0000d0 000001 00      0
+# CHECK1-NEXT: [ 9] .symtab   SYMTAB   0000000000000000 0000d8 000030 18     11
+# CHECK1-NEXT: [10] .shstrtab STRTAB   0000000000000000 000108 00004d 00      0
+# CHECK1-NEXT: [11] .strtab   STRTAB   0000000000000000 000155 000008 00      0
 # CHECK1:      Type       Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # CHECK1-NEXT: LOAD       0x000000 0x0000000000000000 0x0000000000000000 0x0000b5 0x0000b5 RWE 0x1000
 # CHECK1-NEXT: 0x60000000 0x0000b8 0x0000000000000000 0x0000000000000000 0x000009 0x000001     0x8
diff --git a/lld/test/ELF/linkerscript/sections.s b/lld/test/ELF/linkerscript/sections.s
index 5d6cc1f3bd0df..fc03af8402dfe 100644
--- a/lld/test/ELF/linkerscript/sections.s
+++ b/lld/test/ELF/linkerscript/sections.s
@@ -79,8 +79,8 @@
 # SEP-BY-NONALLOC:      [ 1] .text     PROGBITS 0000000000000000 001000 00000e 00  AX
 # SEP-BY-NONALLOC-NEXT: [ 2] .data     PROGBITS 000000000000000e 00100e 000020 00  WA
 # SEP-BY-NONALLOC-NEXT: [ 3] .comment  PROGBITS 0000000000000000 001031 000008 01  MS
-# SEP-BY-NONALLOC:      [ 7] other     PROGBITS 000000000000002e 00102e 000003 00  WA
-# SEP-BY-NONALLOC-NEXT: [ 8] .bss      NOBITS   0000000000000031 001031 000002 00  WA
+# SEP-BY-NONALLOC:      [ 4] other     PROGBITS 000000000000002e 00102e 000003 00  WA
+# SEP-BY-NONALLOC-NEXT: [ 5] .bss      NOBITS   0000000000000031 001031 000002 00  WA
 
 # SEP-BY-NONALLOC:      Type      Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # SEP-BY-NONALLOC-NEXT: LOAD      0x001000 0x0000000000000000 0x0000000000000000 0x00000e 0x00000e R E 0x1000

Created using spr 1.3.5-bogner

smithp35

LGTM thanks, may be worth waiting to see if any of the other reviewers have any concerns.

I agree that the difference from GNU ld is cosmetic. As while it may be at the expected section index, its address and file offset are if it was placed at the end.

Output for reference from GNU ld output for sections-nonalloc.s.tmpb

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        00000000000000b0 0000b0 000001 00  AX  0   0  4
  [ 2] .bss              NOBITS          00000000000000b1 0000b1 000001 00  WA  0   0  1
  [ 3] data1             PROGBITS        00000000000000b2 0000b2 000001 00  WA  0   0  1
  [ 4] other1            PROGBITS        00000000000000b4 0000b8 000001 00      0   0  4
  [ 5] other2            PROGBITS        00000000000000b8 0000c0 000001 00      0   0  8
  [ 6] other3            PROGBITS        0000000000000000 0000d0 000001 00      0   0 16
  [ 7] data2             PROGBITS        00000000000000b3 0000b3 000001 00  WA  0   0  1
  [ 8] data3             PROGBITS        00000000000000b4 0000b4 000001 00  WA  0   0  1
  [ 9] .symtab           SYMTAB          0000000000000000 0000d8 000030 18     10   1  8
  [10] .strtab           STRTAB          0000000000000000 000108 000008 00      0   0  1
  [11] .shstrtab         STRTAB          0000000000000000 000110 00004d 00      0   0  1

smithp35 · 2024-06-06T10:45:30Z

lld/ELF/Writer.cpp

    OutputSection *orphan = &cast<OutputDesc>(*nonScriptI)->osec;
+    auto pos = findOrphanPos(i, nonScriptI);


Although not part of this patch, I'm wondering whether nonScriptI would be better called getNonScriptI to make it a bit clearer that it is a lambda. It's current name implies that it is a variable.

Thx. I'll make a separate NFC commit to rename this and reorder the two statements.

Hmm, nonScriptI is an iterator, not a lambda. So its current name seems fine.

nga888

LGTM.

Created using spr 1.3.5-bogner

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

MaskRay · 2024-06-06T20:10:21Z

Merged as 9ad0175. Forgot to use spr land

[𝘀𝗽𝗿] initial version

0da2a7f

Created using spr 1.3.5-bogner

llvmbot added lld lld:ELF labels Jun 5, 2024

MaskRay mentioned this pull request Jun 5, 2024

[ELF] Orphan placement: remove hasInputSections condition #93761

Open

MaskRay added a commit that referenced this pull request Jun 5, 2024

rebase onto #94519

425390d

Created using spr 1.3.5-bogner

MaskRay requested review from igorkudrin, nga888 and smithp35 June 5, 2024 18:37

smithp35 approved these changes Jun 6, 2024

View reviewed changes

nga888 approved these changes Jun 6, 2024

View reviewed changes

drop unneeded change

6689e59

Created using spr 1.3.5-bogner

MaskRay closed this Jun 6, 2024

MaskRay deleted the users/MaskRay/spr/elf-keep-non-alloc-orphan-sections-at-the-end branch June 6, 2024 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ELF] Keep non-alloc orphan sections at the end #94519

[ELF] Keep non-alloc orphan sections at the end #94519

Uh oh!

MaskRay commented Jun 5, 2024

Uh oh!

llvmbot commented Jun 5, 2024 •

edited

Loading

Uh oh!

smithp35 left a comment

Uh oh!

smithp35 Jun 6, 2024

Uh oh!

MaskRay Jun 6, 2024

Uh oh!

MaskRay Jun 6, 2024

Uh oh!

nga888 left a comment

Uh oh!

MaskRay commented Jun 6, 2024

Uh oh!

Uh oh!

		OutputSection orphan = &cast<OutputDesc>(nonScriptI)->osec;
		auto pos = findOrphanPos(i, nonScriptI);

[ELF] Keep non-alloc orphan sections at the end #94519

[ELF] Keep non-alloc orphan sections at the end #94519

Uh oh!

Conversation

MaskRay commented Jun 5, 2024

Uh oh!

llvmbot commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

nga888 left a comment

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Jun 6, 2024

Uh oh!

Uh oh!

llvmbot commented Jun 5, 2024 •

edited

Loading