[LLD][ELF] - Don't keep empty output sections which have explicit program headers

[grimar]

Summary

This reverts a side effect introduced in the code cleanup patch D43571:
LLD started to emit empty output sections that are explicitly assigned to a segment.

This patch fixes the issue by removing the !sec.phdrs.empty() special case from
isDiscardable. As compensation, we add an early phdrs propagation step (see the inline comment).
This is similar to one that we do in adjustSectionsAfterSorting.

[jhenderson]

It makes me happy that doing this doesn't seem to be all that complex. Thanks for looking into it!

lld/ELF/LinkerScript.cpp

989–990

Nit: delete extra blank line too.

1015

1081

1082

lld/test/ELF/linkerscript/implicit-program-header.test

10–12

Unrelated to this patch, but it looks incorrect that .foo is assigned to this program header? It should be in ph_exec, and only ph_exec, by my understanding.

21

Is it worth extending this test case to show that ALL program headers are propagated, e.g. if .bar was inside : ph_exec : ph_other then .foo is too?

[grimar]

• Addressed review comments.

lld/test/ELF/linkerscript/implicit-program-header.test

10–12

This happens because .foo is placed between .dynsym and .dynamic which belongs to "W" segment:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg L
k Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00
0   0  0
  [ 1] .dynsym           DYNSYM          0000000000000000 001000 000018 18   A
3   1  8
  [ 2] .hash             HASH            0000000000000018 001018 000010 04   A
1   0  4
  [ 3] .dynstr           STRTAB          0000000000000028 001028 000001 00   A
0   0  1
  [ 4] .foo              PROGBITS        0000000000000029 001029 000008 00  AX
0   0  1
  [ 5] .text             PROGBITS        0000000000000034 001034 000008 00  AX
0   0  4
  [ 6] .dynamic          DYNAMIC         0000000000000040 001040 000060 10  WA
3   0  8
  [ 7] .comment          PROGBITS        0000000000000000 0010a0 000008 01  MS
0   0  1
  [ 8] .symtab           SYMTAB          0000000000000000 0010a8 000030 18     1
0   2  8
  [ 9] .shstrtab         STRTAB          0000000000000000 0010d8 00004e 00
0   0  1
  [10] .strtab           STRTAB          0000000000000000 001126 00000a 00
0   0  1

Program Headers:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz
  Flg Align
  LOAD           0x001000 0x0000000000000000 0x0000000000000000 0x0000a0 0x0000a
0  W  0x1000
  LOAD           0x001029 0x0000000000000029 0x0000000000000029 0x000008 0x00000
8   E 0x1000

 Section to Segment mapping:
  Segment Sections...
   00     .dynsym .hash .dynstr .foo .text .dynamic
   01     .foo

The output might looks strange, but I am not sure this is not correct. Sections are sorted by their flags as expected it seems.
I guess the linker script should be more precise about orphpans first of all for having a better and more stable behavior.

21

Done.

[jhenderson]

LGTM. Probably worth waiting for another reviewer too though.

lld/test/ELF/linkerscript/implicit-program-header.test

17

[grimar]

• Addressed review comment.

[MaskRay]

Suggest a rewrite of the description:

This reverts a side effect introduced in the code cleanup patch D43571: LLD started to emit empty output sections that are explicitly assigned to a segment.

This patch fixes the issue by removing the !sec.phdrs.empty() special case from isDiscardable. As compensation, we add an early phdrs propagation step (see the inline comment).
This is similar to one that we do in adjustSectionsAfterSorting.

lld/ELF/LinkerScript.cpp

1073

Suggest:

// The code below may remove empty output sections. We should save the specified program headers (if exist) and propagate them to subsequent sections which do not specify program headers.
// An example of such linker script is:
// SECTIONS { .empty : { *(.empty) } :rw
// .foo : { *(.foo) } }
// Note: at this point the order of output sections has not been finalized, because orphans have not been inserted into their expected positions. We will handle them in adjustSectionsAfterSorting().

[MaskRay]

lld/ELF/LinkerScript.cpp

1073

such linker script -> such a linker script

lld/ELF/LinkerScript.cpp

void LinkerScript::assignOffsets(OutputSection *sec) {
   // as they are not part of the process image.
   if (!(sec->flags & SHF_ALLOC))
     dot = savedDot;
 }
 
 static bool isDiscardable(OutputSection &sec) {
   if (sec.name == "/DISCARD/")
     return true;
 
-  // We do not remove empty sections that are explicitly
-  // assigned to any segment.
-  if (!sec.phdrs.empty())
-    return false;
-
   // We do not want to remove OutputSections with expressions that reference
   // symbols even if the OutputSection is empty. We want to ensure that the
   // expressions can be evaluated and report an error if they cannot.
   if (sec.expressionsUseSymbols)
     return false;
 
   // OutputSections may be referenced by name in ADDR and LOADADDR expressions,
   // as an empty Section can has a valid VMA and LMA we keep the OutputSection
   // to maintain the integrity of the other Expression.
   if (sec.usedInExpression)
     return false;
 
   for (BaseCommand *base : sec.sectionCommands) {
     if (auto cmd = dyn_cast<SymbolAssignment>(base))
       // Don't create empty output sections just for unreferenced PROVIDE
       // symbols.
       if (cmd->name != "." && !cmd->sym)
         continue;
 
     if (!isa<InputSectionDescription>(*base))
       return false;
   }
   return true;
 }
 
+static void maybePropagatePhdrs(OutputSection &sec,
+                                std::vector<StringRef> &phdrs) {
+  if (sec.phdrs.empty()) {
+    // To match the bfd linker script behaviour, only propagate program
+    // headers to sections that are allocated.
+    if (sec.flags & SHF_ALLOC)
+      sec.phdrs = phdrs;
+  } else {
+    phdrs = sec.phdrs;
+  }
+}
+
 void LinkerScript::adjustSectionsBeforeSorting() {
   // If the output section contains only symbol assignments, create a
   // corresponding output section. The issue is what to do with linker script
   // like ".foo : { symbol = 42; }". One option would be to convert it to
   // "symbol = 42;". That is, move the symbol out of the empty section
   // description. That seems to be what bfd does for this simple case. The
   // problem is that this is not completely general. bfd will give up and
   // create a dummy section too if there is a ". = . + 1" inside the section
   // for example.
   // Given that we want to create the section, we have to worry what impact
   // it will have on the link. For example, if we just create a section with
   // 0 for flags, it would change which PT_LOADs are created.
   // We could remember that particular section is dummy and ignore it in
   // other parts of the linker, but unfortunately there are quite a few places
   // that would need to change:
   //   * The program header creation.
   //   * The orphan section placement.
   //   * The address assignment.
   // The other option is to pick flags that minimize the impact the section
   // will have on the rest of the linker. That is why we copy the flags from
   // the previous sections. Only a few flags are needed to keep the impact low.
   uint64_t flags = SHF_ALLOC;
 
+  std::vector<StringRef> defPhdrs;
   for (BaseCommand *&cmd : sectionCommands) {
     auto *sec = dyn_cast<OutputSection>(cmd);
     if (!sec)
       continue;
 
     // Handle align (e.g. ".foo : ALIGN(16) { ... }").
     if (sec->alignExpr)
       sec->alignment =
           std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue());
 
     // The input section might have been removed (if it was an empty synthetic
     // section), but we at least know the flags.
     if (sec->hasInputSections)
       flags = sec->flags;
 
     // We do not want to keep any special flags for output section
     // in case it is empty.
     bool isEmpty = (getFirstInputSection(sec) == nullptr);
     if (isEmpty)
       sec->flags = flags & ((sec->nonAlloc ? 0 : (uint64_t)SHF_ALLOC) |
                             SHF_WRITE | SHF_EXECINSTR);
 
+    // The code below may remove empty output sections. We should save the
+    // specified program headers (if exist) and propagate them to subsequent
+    // sections which do not specify program headers.
+    // An example of such a linker script is:
+    // SECTIONS { .empty : { *(.empty) } :rw
+    //            .foo : { *(.foo) } }
+    // Note: at this point the order of output sections has not been finalized,
+    // because orphans have not been inserted into their expected positions. We
+    // will handle them in adjustSectionsAfterSorting().
+    if (sec->sectionIndex != UINT32_MAX)
+      maybePropagatePhdrs(*sec, defPhdrs);
+
     if (isEmpty && isDiscardable(*sec)) {
       sec->markDead();
       cmd = nullptr;
     }
   }
 
   // It is common practice to use very generic linker scripts. So for any
   // given run some of the output sections in the script will be empty.
void LinkerScript::adjustSectionsAfterSorting() {
   auto firstPtLoad = llvm::find_if(phdrsCommands, [](const PhdrsCommand &cmd) {
     return cmd.type == PT_LOAD;
   });
   if (firstPtLoad != phdrsCommands.end())
     defPhdrs.push_back(firstPtLoad->name);
 
   // Walk the commands and propagate the program headers to commands that don't
   // explicitly specify them.
-  for (BaseCommand *base : sectionCommands) {
-    auto *sec = dyn_cast<OutputSection>(base);
-    if (!sec)
-      continue;
-
-    if (sec->phdrs.empty()) {
-      // To match the bfd linker script behaviour, only propagate program
-      // headers to sections that are allocated.
-      if (sec->flags & SHF_ALLOC)
-        sec->phdrs = defPhdrs;
-    } else {
-      defPhdrs = sec->phdrs;
-    }
-  }
+  for (BaseCommand *base : sectionCommands)
+    if (auto *sec = dyn_cast<OutputSection>(base))
+      maybePropagatePhdrs(*sec, defPhdrs);
 }
 
 static uint64_t computeBase(uint64_t min, bool allocateHeaders) {
   // If there is no SECTIONS or if the linkerscript is explicit about program
   // headers, do our best to allocate them.
   if (!script->hasSectionsCommand || allocateHeaders)
     return 0;
   // Otherwise only allocate program headers if that would not add a page.

lld/test/ELF/linkerscript/empty-relaplt-dyntags.test

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux /dev/null -o %t.o
 # RUN: ld.lld -shared %t.o -T %s -o %t
 # RUN: llvm-readobj --dynamic-table --sections %t | FileCheck %s
 
-## In spite of .rela.plt is empty, it might have been preserved because it is
-## mentioned in the linker script. However, even in that case, we should not
-## produce DT_JMPREL and DT_PLTGOT tags because this can cause a dynamic loader
-## to write into slots in .got.plt it considers reserved to support lazy symbol
-## resolution. In fact, as .got.plt is also empty, that memory might be
-## allocated for something else.
+## Check that we remove the empty .rela.plt section even when it
+## is explicitly assigned to a program header.
+## Check that no related dynamic tags are produced.
 
-# CHECK: Sections [
-# CHECK:      Name: .rela.plt
-# CHECK-NEXT: Type: SHT_RELA
-# CHECK-NEXT: Flags [
-# CHECK-NEXT:   SHF_ALLOC
-# CHECK-NEXT: ]
-# CHECK-NEXT: Address:
-# CHECK-NEXT: Offset:
-# CHECK-NEXT: Size: 0
-# CHECK:      Name: .got.plt
-# CHECK-NEXT: Type: SHT_PROGBITS
-# CHECK-NEXT: Flags [
-# CHECK-NEXT:   SHF_ALLOC
-# CHECK-NEXT:   SHF_WRITE
-# CHECK-NEXT: ]
-# CHECK-NEXT: Address:
-# CHECK-NEXT: Offset:
-# CHECK-NEXT: Size: 0
+# CHECK-NOT: Name: .rela.plt
 
 # CHECK: DynamicSection [
 # CHECK-NOT: JMPREL
 # CHECK-NOT: PLTGOT
 
 PHDRS {
   all PT_LOAD;
   dyn PT_DYNAMIC;
 }
 SECTIONS {
   .rela.plt : { *(.rela.plt) }: all
   .dynamic : { *(.dynamic) }: all : dyn
   .got.plt : {*(.got.plt)}: all
 }

lld/test/ELF/linkerscript/implicit-program-header.test

 # REQUIRES: x86
 
 # RUN: echo '.section .text,"ax"; .quad 0' > %t.s
 # RUN: echo '.section .foo,"ax"; .quad 0' >> %t.s
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t.s -o %t.o
 # RUN: ld.lld --hash-style=sysv -o %t1 --script %s %t.o -shared
 # RUN: llvm-readelf -l %t1 | FileCheck %s
 
 # CHECK:      Segment Sections...
-# CHECK-NEXT:   00     .dynsym .hash .dynstr .bar .foo .text .dynamic
-# CHECK-NEXT:   01     .bar .foo
+# CHECK-NEXT:   00     .dynsym .hash .dynstr .foo .text .dynamic
+# CHECK-NEXT:   01     .foo
+# CHECK-NEXT:   02     .foo
 
 PHDRS {
   ph_write PT_LOAD FLAGS(2);
   ph_exec  PT_LOAD FLAGS(1);
+  ph_note  PT_NOTE;
 }
 
 SECTIONS {
-  .bar : { *(.bar) } : ph_exec
+  .bar : { *(.bar) } : ph_exec : ph_note
   .foo : { *(.foo) }
   .text : { *(.text) } : ph_write
 }

lld/test/ELF/linkerscript/orphan-phdrs.s

 # RUN: llvm-readelf -S -l %t | FileCheck %s
 
 ## Check that the orphan section is placed correctly and belongs to
 ## the correct segment.
 
 # CHECK: Section Headers
 # CHECK: .text
 # CHECK-NEXT: .orphan
-# CHECK-NEXT: .empty
 # CHECK-NEXT: .rw
 
 # CHECK: Segment Sections
 # CHECK-NEXT: .text .orphan
-# CHECK-NEXT: .empty .rw
+# CHECK-NEXT: .rw
 
 .section .text,"ax"
  ret
 
 .section .rw,"aw"
  .quad 0
 
 .section .orphan,"ax"
  ret