[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

37 ↗

(On Diff #350290)

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.

72 ↗

(On Diff #350290)

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–23

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

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–23

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

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

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 (!isValidCIdentifier(s))
     return;
   addOptionalRegular(saver.save("__start_" + s), sec, 0,
                      config->zStartStopVisibility);
   addOptionalRegular(saver.save("__stop_" + s), sec, -1,
                      config->zStartStopVisibility);
 }
 
 static bool needsPtLoad(OutputSection *sec) {
-  if (!(sec->flags & SHF_ALLOC) || sec->noload)
+  if (!(sec->flags & SHF_ALLOC))
     return false;
 
   // Don't allocate VA space for TLS NOBITS sections. The PT_TLS PHDR is
   // responsible for allocating space for them, not the PT_LOAD that
   // contains the TLS initialization image.
   if ((sec->flags & SHF_TLS) && sec->type == SHT_NOBITS)
     return false;
   return true;

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

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: echo "SECTIONS { \
 # RUN:         .sec1 (NOLOAD) : { . += 1; } \
 # RUN:         .bss           : { *(.bss) } \
 # RUN:       };" > %t.script
 # RUN: ld.lld %t.o -T %t.script -o %t
 # RUN: llvm-readelf -S -l %t | FileCheck %s
 
 ## If a SHT_NOBITS section is the only section of a PT_LOAD segment,
 ## p_offset will be set to the sh_offset field of the section. Check we align
 ## sh_offset to sh_addr modulo max-page-size, so that p_vaddr=p_offset (mod
 ## p_align).
 
 # CHECK:      Name  Type     Address          Off     Size
 # CHECK-NEXT:       NULL     0000000000000000 000000  000000
-# CHECK-NEXT: .text PROGBITS 0000000000000000 000158  000000
-# CHECK-NEXT: .sec1 NOBITS   0000000000000000 000158  000001
-# CHECK-NEXT: .bss  NOBITS   0000000000000400 000400  000001
+# CHECK-NEXT: .text PROGBITS 0000000000000000 000190  000000
+# CHECK-NEXT: .sec1 NOBITS   0000000000000000 001000  000001
+# CHECK-NEXT: .bss  NOBITS   0000000000000400 001400  000001
 
 # CHECK:      Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
-# CHECK-NEXT: LOAD 0x000400 0x0000000000000400 0x0000000000000400 0x000000 0x000001 RW  0x1000
+# CHECK-NEXT: LOAD 0x001000 0x0000000000000000 0x0000000000000000 0x000000 0x000001 R   0x1000
+# CHECK-NEXT: LOAD 0x001400 0x0000000000000400 0x0000000000000400 0x000000 0x000001 RW  0x1000
 
-# CHECK:      00 .bss {{$}}
+# CHECK:      00 .sec1 {{$}}
+# CHECK:      01 .bss {{$}}
 
 .bss
 .p2align 10
 .byte 0

lld/test/ELF/linkerscript/noload.s

 # REQUIRES: x86
 # RUN: split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %t/asm -o %t.o
 # RUN: ld.lld --script %t/lds %t.o -o %t/out
 # RUN: llvm-readelf -S -l %t/out | FileCheck %s
 
 # CHECK:      Name                 Type   Address          Off               Size
 # CHECK:      .data_noload_a       NOBITS 0000000000000000 [[OFF:[0-9a-f]+]] 001000
 # CHECK-NEXT: .data_noload_b       NOBITS 0000000000010000 [[OFF]]           001000
 # CHECK-NEXT: .no_input_sec_noload NOBITS 0000000000011000 [[OFF]]           000001
 
 # CHECK:      Type Offset   VirtAddr           PhysAddr
+# CHECK-NEXT: LOAD 0x001000 0x0000000000000000 0x0000000000000000
 # CHECK-NEXT: LOAD 0x001000 0x0000000000020000 0x0000000000020000
 
+# CHECK:      00 .data_noload_a .data_noload_b .no_input_sec_noload {{$}}
+# CHECK:      01 .text {{$}}
+
 #--- asm
 .section .text,"ax",@progbits
   nop
 
 .section .data_noload_a,"aw",@progbits
 .zero 4096
 
 .section .data_noload_b,"aw",@progbits