[ELF] Consider that NOLOAD sections should be placed in a PT_LOAD segment

[kschwarz]

Summary

During PHDR creation, the case where an output section does not require a
PT_LOAD header but still occupies memory in the current VMA region was not handled.

If such an output section interleaves two output sections that have the same
VMA and LMA regions set, we would previously re-use the existing PT_LOAD header
for the second output section.
However, since the memory region is not contiguous, we need to start a new PT_LOAD
segment.

This fixes https://bugs.llvm.org/show_bug.cgi?id=50558

[MaskRay]

During PHDR creation, the case where an output section does not require a PT_LOAD header but still occupies memory in the current VMA region was not handled.

I think our needsPtLoad may not match GNU ld. "does not require a PT_LOAD" may be incorrect.

The latest GNU ld code has a comment from
https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=ld/ldlang.c;h=9038ebfff3b40298572b3eaf10eb0531e105d4fa;hp=798d7a1109712033d7764e2ef8e59351476d89b2;hb=f4eaaf7fceed097ed62542f9940986b1de01aa48;hpb=7cdfa31841aea9665682cadb03aaade2821721b1

The semantics appear to be different from what is documented here https://sourceware.org/binutils/docs/ld/Output-Section-Type.html

lld/test/ELF/linkerscript/phdrs-noload.test

38

This style is too verbose. { can be at the line end.

If the input section descriptions are short, you may even consider place the whole thing on one line.

52

No newline at end of file

[kschwarz]

Test formatting

[kschwarz]

Thanks for your comments! I've applied the suggested formatting changes.

Indeed the comment you referenced

[...] ELF gets a .bss style noload, alloc, no contents section. [...]

seems to disagree with the documentation, where a NOLOAD section can have contents.
I'm not too familiar with the GNU ld internals, so I'm not sure what the effect of clearing SEC_HAS_CONTENTS is.

What would be your suggestion how to proceed?

[MaskRay]

Remove the noload check from needsPtLoad

[kschwarz]

https://reviews.llvm.org/D45264 specifically introduced the check for sec->noload, stating that this conforms to GNU ld 2.29

I checked on my local system with GNU ld 2.34, and it does create PT_LOAD segments for NOLOAD output sections.
So, is the only effect of NOLOAD on an output section to mark it as SHT_NOBITS?

[kschwarz]

Remove the check for noload, adapt existing tests

[kschwarz]

lld/test/ELF/linkerscript/nobits-offset.s

22 ↗

(On Diff #350886)

Independent from this change, GNU ld assigns WA section flags to output sections defined as

.sec1 (NOLOAD) : { . += 1; }

, while ld.lld assigns A. Is this intended?

This has also an effect on the number of program headers we create. In the test, the bss section has WA
flags, thus getting its own header.

lld/test/ELF/linkerscript/noload.s

13 ↗

(On Diff #350886)

GNU ld creates the following program headers:

LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x001000 RW  0x1000
LOAD           0x000000 0x0000000000010000 0x0000000000010000 0x000000 0x001001 RW  0x1000
LOAD           0x001000 0x0000000000020000 0x0000000000020000 0x000001 0x000001 R E 0x1000

Should we create a new header whenever a section has an addrExpr set, similar to our condition for lmaExpr?

GNU ld will only create a new header if the current section is placed at a discontiguous address.

[MaskRay]

lld/test/ELF/linkerscript/nobits-offset.s

22 ↗

(On Diff #350886)

I think our behavior is intended.

The GNU ld behavior (I don't know what the current state is) was not ideal: https://sourceware.org/bugzilla/show_bug.cgi?id=26378#c8

[MaskRay]

lld/test/ELF/linkerscript/noload.s

13 ↗

(On Diff #350886)

Try merging PT_LOAD with same flags and contiguous address ranges.

Doing this rigidly can make the code more complex. In such cases we may trade the merged behavior for simpler implementation.

[kschwarz]

lld/test/ELF/linkerscript/noload.s

13 ↗

(On Diff #350886)

I had a look when/where the merging of PT_LOAD segments with contiguous address ranges should occur.
If no addrExprs are used, the VMAs will usually be contiguous and we can keep the current behavior, i.e. check whether the memregions are identical.

However if addrExprs are used, we cannot know whether two output sections will have contiguous address ranges until after the final layout is fixed. In that case, we would need a final "merge pass", maybe at the same time we remove empty PT_LOAD segments.

If you agree, I'd like to keep these changes separate and first commit this change (creating PT_LOAD headers for NOLOAD sections).

Creating a new PT_LOAD header for every section that uses an addrExpr breaks quite some tests in the test suite, so that definitely needs a closer look.

[MaskRay]

LGTM, but the subject and description need to be updated.

We drop the special case that (NOLOAD) is not placed in a PT_LOAD. The new logic aligns with modern GNU ld.

If merging PT_LOAD will add more complexity to sameLMARegion, I'd suggest we don't do that.

[kschwarz]

In D103815#2820179, @MaskRay wrote:

LGTM, but the subject and description need to be updated.

Thanks! I updated the commit message locally, but wasn't aware that arc land will fetch the existing description in Phabricator, so that got overwritten again.. sorry for that.

[MaskRay]

After modifying the commit description, use arc diff --verbatim --head=HEAD 'HEAD^' to update the Phabricator subject/summary.

lld/ELF/Writer.cpp

if (isRelroSection(sec)) {
         error("section: " + sec->name + " is not contiguous with other relro" +
               " sections");
     } else if (inRelroPhdr) {
       inRelroPhdr = false;
       relroEnd = sec;
     }
   }
 
+  bool nonContiguousVMA = false;
   for (OutputSection *sec : outputSections) {
-    if (!needsPtLoad(sec))
+    if (!needsPtLoad(sec)) {
+      if (sec->memRegion && sec->memRegion == load->firstSec->memRegion)
+        nonContiguousVMA = true;
       continue;
+    }
 
     // Normally, sections in partitions other than the current partition are
     // ignored. But partition number 255 is a special case: it contains the
     // partition end marker (.part.end). It needs to be added to the main
     // partition so that a segment is created for it in the main partition,
     // which will cause the dynamic loader to reserve space for the other
     // partitions.
     if (sec->partition != partNo) {
       if (isMain && sec->partition == 255)
         addHdr(PT_LOAD, computeFlags(sec->getPhdrFlags()))->add(sec);
       continue;
     }
 
     // Segments are contiguous memory regions that has the same attributes
     // (e.g. executable or writable). There is one phdr for each segment.
     // Therefore, we need to create a new phdr when the next section has
     // different flags or is loaded at a discontiguous address or memory
     // region using AT or AT> linker script command, respectively. At the same
     // time, we don't want to create a separate load segment for the headers,
     // even if the first output section has an AT or AT> attribute.
     uint64_t newFlags = computeFlags(sec->getPhdrFlags());
     bool sameLMARegion =
         load && !sec->lmaExpr && sec->lmaRegion == load->firstSec->lmaRegion;
-    if (!(load && newFlags == flags && sec != relroEnd &&
+    if (nonContiguousVMA ||
+        !(load && newFlags == flags && sec != relroEnd &&
           sec->memRegion == load->firstSec->memRegion &&
           (sameLMARegion || load->lastSec == Out::programHeaders))) {
       load = addHdr(PT_LOAD, newFlags);
       flags = newFlags;
+      nonContiguousVMA = false;
     }
 
     load->add(sec);
   }
 
   // Add a TLS segment if any.
   PhdrEntry *tlsHdr = make<PhdrEntry>(PT_TLS, PF_R);
   for (OutputSection *sec : outputSections)

lld/test/ELF/linkerscript/phdrs-noload.test

+# REQUIRES: x86
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/main.s -o %t.o
+
+## NOLOAD sections do not require a PT_LOAD segment, but they can still occupy
+## memory in the executable image.
+## If two output sections with the same VMA and LMA regions are interleaved by
+## a NOLOAD output section, two separate PT_LOAD segments are required.
+# RUN: ld.lld -T %t/link.ld  %t.o -o %ta
+# RUN: llvm-readelf -S -l %ta | FileCheck %s
+
+# CHECK:       [Nr] Name      Type     Address          Off    Size   ES Flg Lk
+# CHECK-NEXT:  [ 0]           NULL     0000000000000000 000000 000000 00      0
+# CHECK-NEXT:  [ 1] .text     PROGBITS 0000000008000000 001000 000001 00  AX  0
+# CHECK-NEXT:  [ 2] .data     PROGBITS 0000000020000400 001400 000004 00  WA  0
+# CHECK-NEXT:  [ 3] .data2    NOBITS   0000000020000404 001404 000004 00  WA  0
+# CHECK-NEXT:  [ 4] .data3    PROGBITS 0000000020000408 001408 000004 00  WA  0
+# CHECK-NEXT:  [ 5] .data4    PROGBITS 0000000008000009 002009 00000c 00  WA  0
+
+
+# CHECK:       Type       Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK-NEXT:  LOAD       0x001000 0x0000000008000000 0x0000000008000000 0x000001 0x000001 R E 0x1000
+# CHECK-NEXT:  LOAD       0x001400 0x0000000020000400 0x0000000008000001 0x000004 0x000004 RW  0x1000
+# CHECK-NEXT:  LOAD       0x001408 0x0000000020000408 0x0000000008000005 0x000004 0x000004 RW  0x1000
+# CHECK-NEXT:  LOAD       0x002009 0x0000000008000009 0x0000000008000009 0x00000c 0x00000c RW  0x1000
+# CHECK-NEXT:  GNU_STACK  0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0x0
+
+
+#--- link.ld
+MEMORY {
+    CODE (rx) : ORIGIN = 0x08000000, LENGTH = 512K
+    DATA (rw) : ORIGIN = 0x20000400, LENGTH = 512K
+}
+
+SECTIONS {
+    .text : { *(.text) } > CODE
+    .data : { *(.a.data)} > DATA AT> CODE
+    .data2 (NOLOAD) : { . += 4; } > DATA
+    .data3 : { *(.b.data) } > DATA AT> CODE
+    .data4 : { LONG(-1); LONG(-1); LONG(-1); } > CODE
+}
+
+#--- main.s
+.globl _start
+_start: nop
+
+.section .a.data, "aw"
+.long 0xabcdef01
+
+.section .b.data, "aw"
+.long 0xbcdefa12