[ELF] Orphan placement: prefer the last similar section when its rank <= orphan's rank #94099

MaskRay · 2024-06-01T06:36:04Z

findOrphanPos finds the most similar output section (that has input
sections). In the event of proximity ties, we select the first section.

However, when an orphan section's rank is equal to or larger than the
most similar sections's, it makes sense to prioritize the last similar
section. This new behavior matches GNU ld better.

// orphan placement for .bss (SHF_ALLOC|SHF_WRITE, SHT_NOBITS)

WA SHT_PROGBITS
(old behavior) <= here
A
WA SHT_PROGBITS
AX
WA (.data)
(new behavior) <= here

When the orphan section's rank is less, the current behavior
prioritizing the first section still makes sense.

// orphan with a smaller rank, e.g. .rodata

<= here
WA
AX
WA

Close #92987

Created using spr 1.3.5-bogner

llvmbot · 2024-06-01T06:36:32Z

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

findOrphanPos selects the most similar output section (that has input
sections). In case of ties, it select the first section.

However, when an orphan section has the same or larger rank than the
most similar sections, it makes sense to select the last similar
section. This new behavior matches GNU ld better.

// orphan with the same or larger rank than .data, e.g. .bss

WA (data like section)
(old behavior) &lt;= here
A
WA (data like section)
AX
WA (.data)
(new behavior) &lt;= here

Close #92987

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

4 Files Affected:

(modified) lld/ELF/Writer.cpp (+7-3)
(modified) lld/test/ELF/linkerscript/orphan.s (+22)
(modified) lld/test/ELF/linkerscript/sections-nonalloc.s (+17-16)
(modified) lld/test/ELF/linkerscript/sections.s (+4-4)

diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 58ae20720a177..685ac0352f1fb 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -935,14 +935,18 @@ findOrphanPos(SmallVectorImpl<SectionCommand *>::iterator b,
       return i;
   }
 
-  // Find the first element that has as close a rank as possible.
   if (b == e)
     return e;
+  // Select the most similar output section. In case of ties, select the first
+  // section with a rank > the orphan's rank, or the last one with a rank <= the
+  // orphan's rank.
   int proximity = getRankProximity(sec, *b);
   auto i = b;
   for (auto j = b; ++j != e;) {
-    int p = getRankProximity(sec, *j);
-    if (p > proximity) {
+    auto p = getRankProximity(sec, *j);
+    if (p > proximity ||
+        (p == proximity && proximity >= 0 &&
+         cast<OutputDesc>(*j)->osec.sortRank <= sec->sortRank)) {
       proximity = p;
       i = j;
     }
diff --git a/lld/test/ELF/linkerscript/orphan.s b/lld/test/ELF/linkerscript/orphan.s
index 4f01b181d0416..dc1b49042a987 100644
--- a/lld/test/ELF/linkerscript/orphan.s
+++ b/lld/test/ELF/linkerscript/orphan.s
@@ -65,6 +65,18 @@
 # RW-TEXT-NEXT: .text     PROGBITS 0000000000001{{...}} 0
 # RW-TEXT-NEXT: .mytext   PROGBITS 0000000000001{{...}} 0
 
+# RUN: ld.lld a.o -T rw-text-rw.lds -o rw-text-rw
+# RUN: llvm-readelf -S rw-text-rw | FileCheck %s --check-prefix=RW-TEXT-RW
+# RW-TEXT-RW:      .jcr      PROGBITS 00000000000002{{..}} 0
+# RW-TEXT-RW-NEXT: .rw1      PROGBITS 00000000000002{{..}} 0
+# RW-TEXT-RW-NEXT: .interp   PROGBITS 00000000000002{{..}} 0
+# RW-TEXT-RW-NEXT: .note.my  NOTE     00000000000002{{..}} 0
+# RW-TEXT-RW-NEXT: .text     PROGBITS 0000000000001{{...}} 0
+# RW-TEXT-RW-NEXT: .mytext   PROGBITS 0000000000001{{...}} 0
+# RW-TEXT-RW-NEXT: .rw2      PROGBITS 0000000000002{{...}} 0
+# RW-TEXT-RW-NEXT: .rw3      PROGBITS 0000000000002{{...}} 0
+# RW-TEXT-RW-NEXT: .bss      NOBITS   0000000000002{{...}} 0
+
 #--- a.s
 .section .rw1, "aw"; .byte 0
 .section .rw2, "aw"; .byte 0
@@ -112,3 +124,13 @@ SECTIONS {
   . = ALIGN(CONSTANT(MAXPAGESIZE));
   .text : { *(.text) }
 }
+
+#--- rw-text-rw.lds
+SECTIONS {
+  . = SIZEOF_HEADERS;
+  .rw1 : { *(.rw1) }
+  . = ALIGN(CONSTANT(MAXPAGESIZE));
+  .text : { *(.text) }
+  . = ALIGN(CONSTANT(MAXPAGESIZE));
+  .rw2 : { *(.rw2) }
+}
diff --git a/lld/test/ELF/linkerscript/sections-nonalloc.s b/lld/test/ELF/linkerscript/sections-nonalloc.s
index a0669f701d8c9..79765d32dfffe 100644
--- a/lld/test/ELF/linkerscript/sections-nonalloc.s
+++ b/lld/test/ELF/linkerscript/sections-nonalloc.s
@@ -14,15 +14,16 @@
 # CHECK-NEXT:  [ 0]           NULL     0000000000000000 000000 000000 00      0
 # CHECK-NEXT:  [ 1] .bss      NOBITS   0000000000000000 001000 000001 00  WA  0
 # CHECK-NEXT:  [ 2] data1     PROGBITS 0000000000000001 001001 000001 00  WA  0
-# CHECK-NEXT:  [ 3] data3     PROGBITS 0000000000000002 001002 000001 00  WA  0
-# CHECK-NEXT:  [ 4] other1    PROGBITS 0000000000000000 001008 000001 00      0
-# CHECK-NEXT:  [ 5] other2    PROGBITS 0000000000000000 001010 000001 00      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:  [ 6] other3    PROGBITS 0000000000000000 001020 000001 00      0
-# CHECK-NEXT:  [ 7] .symtab   SYMTAB   0000000000000000 001028 000030 18      9
-# CHECK-NEXT:  [ 8] .shstrtab STRTAB   0000000000000000 001058 00004d 00      0
-# CHECK-NEXT:  [ 9] .strtab   STRTAB   0000000000000000 0010a5 000008 00      0
-# CHECK-NEXT:  [10] data2     PROGBITS 0000000000000003 001003 000001 00  WA  0
+# 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
+## 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:       Type       Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
@@ -38,14 +39,14 @@
 # CHECK1-NEXT: [ 1] .text     PROGBITS 00000000000000b0 0000b0 000001 00  AX  0
 # CHECK1-NEXT: [ 2] .bss      NOBITS   00000000000000b1 0000b1 000001 00  WA  0
 # CHECK1-NEXT: [ 3] data1     PROGBITS 00000000000000b2 0000b2 000001 00  WA  0
-# CHECK1-NEXT: [ 4] data3     PROGBITS 00000000000000b3 0000b3 000001 00  WA  0
-# CHECK1-NEXT: [ 5] other1    PROGBITS 0000000000000000 0000b8 000001 00      0
-# CHECK1-NEXT: [ 6] other2    PROGBITS 0000000000000000 0000c0 000001 00      0
-# CHECK1-NEXT: [ 7] other3    PROGBITS 0000000000000000 0000d0 000001 00      0
-# CHECK1-NEXT: [ 8] .symtab   SYMTAB   0000000000000000 0000d8 000030 18     10
-# CHECK1-NEXT: [ 9] .shstrtab STRTAB   0000000000000000 000108 00004d 00      0
-# CHECK1-NEXT: [10] .strtab   STRTAB   0000000000000000 000155 000008 00      0
-# CHECK1-NEXT: [11] data2     PROGBITS 00000000000000b4 0000b4 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:      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 539aa9c170588..5d6cc1f3bd0df 100644
--- a/lld/test/ELF/linkerscript/sections.s
+++ b/lld/test/ELF/linkerscript/sections.s
@@ -78,13 +78,13 @@
 # SEP-BY-NONALLOC:      [Nr] Name      Type     Address          Off    Size   ES Flg
 # 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] .bss      NOBITS   000000000000002e 00102e 000002 00  WA
-# SEP-BY-NONALLOC-NEXT: [ 4] .comment  PROGBITS 0000000000000000 001033 000008 01  MS
-# SEP-BY-NONALLOC:      [ 8] other     PROGBITS 0000000000000030 001030 000003 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:      Type      Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # SEP-BY-NONALLOC-NEXT: LOAD      0x001000 0x0000000000000000 0x0000000000000000 0x00000e 0x00000e R E 0x1000
-# SEP-BY-NONALLOC-NEXT: LOAD      0x00100e 0x000000000000000e 0x000000000000000e 0x000025 0x000025 RW  0x1000
+# SEP-BY-NONALLOC-NEXT: LOAD      0x00100e 0x000000000000000e 0x000000000000000e 0x000023 0x000025 RW  0x1000
 # SEP-BY-NONALLOC-NEXT: GNU_STACK 0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0
 
 # Input section pattern contains additional semicolon.

smithp35

A few comments based on reading the code.

smithp35 · 2024-06-03T14:44:23Z

lld/ELF/Writer.cpp

  if (b == e)
    return e;
+  // Select the most similar output section. In case of ties, select the first
+  // section with a rank > the orphan's rank, or the last one with a rank <= the
+  // orphan's rank.
  int proximity = getRankProximity(sec, *b);


The implementation getRankProximity needs to be understood before this part can be understood. Perhaps worth mentioning in the comment.

Rank Proximity is a value in the range [-1, 32] with -1 for SectionCommands with no input sections leaving [0, 32] to describe the proximity with 0 most different and 32 identical.

Perhaps worth using highestProximity or maxProximity to make the purpose a bit clearer. Could even shorten to maxP.

Thanks for the comment and variable name suggestion. Adopted.

The comment is slightly tuned.

lld/ELF/Writer.cpp

smithp35 · 2024-06-03T15:09:12Z

lld/ELF/Writer.cpp

-    int p = getRankProximity(sec, *j);
-    if (p > proximity) {
+    auto p = getRankProximity(sec, *j);
+    if (p > proximity ||


I'm trying to match this up with the comment:

In case of ties, select the first section with a rank > the orphan's rank, or the last one with a rank <= the orphan's rank.

I can see the "or the last one with a rank <= orphan's rank" explicit in the condition. I think the first part is implicit because we already have a section selected "first section" and we don't update it.

Not entirely sure how to make this clearer though. Perhaps rewording the original comment to:

In the event of proximity ties, we select the first section, unless a later section with the same proximity has a rank <= the orphan's rank.

I used a variant of the comment that suggests the intention, but not replicate the "less" or "greater" behavior described by the code.

// In the event of proximity ties, we select the first or last section // depending on whether the orphan's rank is smaller.

Created using spr 1.3.5-bogner

github-actions · 2024-06-03T18:22:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Created using spr 1.3.5-bogner

MaskRay · 2024-06-03T18:27:55Z

I have captured lld's algorithm in https://maskray.me/blog/2024-06-02-understanding-orphan-sections , which is hopefully useful.

igorkudrin

LGTM

smithp35

Thanks for the update. Changes have made this easier to understand.

[𝘀𝗽𝗿] initial version

899c740

Created using spr 1.3.5-bogner

llvmbot added lld lld:ELF labels Jun 1, 2024

MaskRay requested review from igorkudrin, nga888 and smithp35 June 1, 2024 06:41

This was referenced Jun 1, 2024

[ELF] Orphan placement: remove hasInputSections condition #93761

Open

lld: Questionable heuristics for placing orphaned input sections #92987

Closed

smithp35 reviewed Jun 3, 2024

View reviewed changes

improve comments

70fe99e

Created using spr 1.3.5-bogner

simplify

9d41116

Created using spr 1.3.5-bogner

igorkudrin approved these changes Jun 4, 2024

View reviewed changes

smithp35 approved these changes Jun 4, 2024

View reviewed changes

MaskRay merged commit 7b34635 into main Jun 4, 2024
7 checks passed

MaskRay deleted the users/MaskRay/spr/elf-orphan-placement-prefer-the-last-similar-section-when-its-rank-orphans-rank branch June 4, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ELF] Orphan placement: prefer the last similar section when its rank <= orphan's rank #94099

[ELF] Orphan placement: prefer the last similar section when its rank <= orphan's rank #94099

Uh oh!

MaskRay commented Jun 1, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 1, 2024 •

edited

Loading

Uh oh!

smithp35 left a comment

Uh oh!

smithp35 Jun 3, 2024

Uh oh!

MaskRay Jun 3, 2024

Uh oh!

Uh oh!

smithp35 Jun 3, 2024

Uh oh!

MaskRay Jun 3, 2024

Uh oh!

github-actions bot commented Jun 3, 2024 •

edited

Loading

Uh oh!

MaskRay commented Jun 3, 2024

Uh oh!

igorkudrin left a comment

Uh oh!

smithp35 left a comment

Uh oh!

Uh oh!

Uh oh!

[ELF] Orphan placement: prefer the last similar section when its rank <= orphan's rank #94099

[ELF] Orphan placement: prefer the last similar section when its rank <= orphan's rank #94099

Uh oh!

Conversation

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

Uh oh!

Uh oh!

llvmbot commented Jun 1, 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 3, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smithp35 Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaskRay commented Jun 3, 2024

Uh oh!

igorkudrin left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MaskRay commented Jun 1, 2024 •

edited

Loading

llvmbot commented Jun 1, 2024 •

edited

Loading

github-actions bot commented Jun 3, 2024 •

edited

Loading