[ELF] --orphan-handling=: don't warn/error for unused synthesized sections

[MaskRay]

Summary

This makes --orphan-handling= less noisy.
This change also improves our compatibility with GNU ld.

GNU ld special cases .symtab, .strtab and .shstrtab . We need output section
descriptions for .symtab, .strtab and .shstrtab to suppress:

<internal>:(.symtab) is being placed in '.symtab'
<internal>:(.shstrtab) is being placed in '.shstrtab'
<internal>:(.strtab) is being placed in '.strtab'

With --strip-all, .symtab and .strtab can be omitted (--strip-all is not
compatible with --emit-relocs).

[MaskRay]

Improve tests

[nickdesaulniers]

Thanks for the patch, just minor drive by comments, mostly aesthetic.

Is there anywhere we can document what the "synthetic sections" are?

lld/ELF/LinkerScript.cpp

731

Bikeshed: needs a better name. DiagnoseOrphanHandling or EnforceOrphanHandlingPolicy or such. process doesn't make any modifications.

732

Consider returning early if the loop invariant config->orphanHandling != OrphanHandlingPolicy::Error && config->orphanHandling != OrphanHandlingPolicy::Warn.

lld/ELF/LinkerScript.h

285

can this method be marked const?

lld/ELF/Writer.cpp

1680–1681

s/SS/ss/?

[kees]

On my orphan checking kernel series, I'm left with only .rela_* and .rela.* getting reported, along with:

(.shstrtab) is being placed in '.shstrtab'
(.strtab) is being placed in '.strtab'
(.symtab) is being placed in '.symtab'

The diff before and after this patch is:

$ diff -u /tmp/before.log /tmp/after.log 
--- /tmp/before.log     2020-02-25 15:56:05.906258196 -0800
+++ /tmp/after.log      2020-02-25 15:53:28.144706255 -0800
@@ -1,6 +1,3 @@
-(.bss.rel.ro) is being placed in '.bss.rel.ro'
-(.iplt) is being placed in '.iplt'
-(.plt) is being placed in '.plt'
 (.rela.altinstr_aux) is being placed in '.rela.altinstr_aux'
 (.rela.altinstr_replacement) is being placed in '.rela.altinstr_replacement'
 (.rela.altinstructions) is being placed in '.rela.altinstructions'
@@ -10939,4 +10936,3 @@
 (.shstrtab) is being placed in '.shstrtab'
 (.strtab) is being placed in '.strtab'
 (.symtab) is being placed in '.symtab'
-(.symtab_shndx) is being placed in '.symtab_shndx'

[MaskRay]

Address review comments

lld/ELF/LinkerScript.cpp

731

Thanks for the suggestions. I'll go with diagnoseOrphanHandling. (I considered handleOrphanHandling and thought it is silly.)

732

I think the current code is more readable. (config->orphanHandling != OrphanHandlingPolicy::Error && config->orphanHandling != OrphanHandlingPolicy::Warn adds two extra lines.)

if ... else if makes it clear that there is the third choice (no diagnostics).

[grimar]

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

18

There is an excessive space after ".symtab". Pehaps just add the word "sections"?

37

Use --implicit-check-not=<internal> in tests?
In the current way seems the test case accepts the situation when for a COMMON case
there is an internal section between .comment and .shstrtab

[MaskRay]

Improve tests

[MaskRay]

Update description

lld/ELF/LinkerScript.h

public:
 
   bool hasPhdrsCommands() { return !phdrsCommands.empty(); }
   uint64_t getDot() { return dot; }
   void discard(InputSectionBase *s);
 
   ExprValue getSymbolValue(StringRef name, const Twine &loc);
 
   void addOrphanSections();
+  void diagnoseOrphanHandling() const;
   void adjustSectionsBeforeSorting();
   void adjustSectionsAfterSorting();
 
   std::vector<PhdrEntry *> createPhdrs();
   bool needsInterpSection();
 
   bool shouldKeep(InputSectionBase *s);
   const Defined *assignAddresses();
public:
 
   // A list of symbols referenced by the script.
   std::vector<llvm::StringRef> referencedSymbols;
 
   // Used to implement INSERT [AFTER|BEFORE]. Contains output sections that need
   // to be reordered.
   std::vector<InsertCommand> insertCommands;
 
+  // Sections that will be warned/errored by --orphan-handling.
+  std::vector<const InputSectionBase *> orphanSections;
+
   // Sections whose addresses are not equal to their addrExpr values.
   std::vector<std::pair<const OutputSection *, uint64_t>>
       changedSectionAddresses;
 };
 
 extern LinkerScript *script;
 
 } // end namespace elf
 } // end namespace lld
 
 #endif // LLD_ELF_LINKER_SCRIPT_H

lld/ELF/LinkerScript.cpp

 // Add sections that didn't match any sections command.
 void LinkerScript::addOrphanSections() {
   StringMap<TinyPtrVector<OutputSection *>> map;
   std::vector<OutputSection *> v;
 
   std::function<void(InputSectionBase *)> add;
   add = [&](InputSectionBase *s) {
     if (s->isLive() && !s->parent) {
-      StringRef name = getOutputSectionName(s);
-
-      if (config->orphanHandling == OrphanHandlingPolicy::Error)
-        error(toString(s) + " is being placed in '" + name + "'");
-      else if (config->orphanHandling == OrphanHandlingPolicy::Warn)
-        warn(toString(s) + " is being placed in '" + name + "'");
+      orphanSections.push_back(s);
 
+      StringRef name = getOutputSectionName(s);
       if (OutputSection *sec = findByName(sectionCommands, name)) {
         sec->recordSection(s);
       } else {
         if (OutputSection *os = addInputSec(map, s, name))
           v.push_back(os);
         assert(isa<MergeInputSection>(s) ||
                s->getOutputSection()->sectionIndex == UINT32_MAX);
       }
void LinkerScript::addOrphanSections() {
   // for example: "foo = ABSOLUTE(ADDR(.text)));".
   // When SECTIONS command is present we just add all orphans to the end.
   if (hasSectionsCommand)
     sectionCommands.insert(sectionCommands.end(), v.begin(), v.end());
   else
     sectionCommands.insert(sectionCommands.begin(), v.begin(), v.end());
 }
 
+void LinkerScript::diagnoseOrphanHandling() const {
+  for (const InputSectionBase *sec : orphanSections) {
+    StringRef name = getOutputSectionName(sec);
+    if (config->orphanHandling == OrphanHandlingPolicy::Error)
+      error(toString(sec) + " is being placed in '" + name + "'");
+    else if (config->orphanHandling == OrphanHandlingPolicy::Warn)
+      warn(toString(sec) + " is being placed in '" + name + "'");
+  }
+}
+
 uint64_t LinkerScript::advance(uint64_t size, unsigned alignment) {
   bool isTbss =
       (ctx->outSec->flags & SHF_TLS) && ctx->outSec->type == SHT_NOBITS;
   uint64_t start = isTbss ? dot + ctx->threadBssOffset : dot;
   start = alignTo(start, alignment);
   uint64_t end = start + size;
 
   if (isTbss)

lld/ELF/Writer.cpp

static void removeUnusedSyntheticSections() {
   // non-synthetic.
   for (InputSectionBase *s : llvm::reverse(inputSections)) {
     SyntheticSection *ss = dyn_cast<SyntheticSection>(s);
     if (!ss)
       return;
     OutputSection *os = ss->getParent();
     if (!os || ss->isNeeded())
       continue;
 
-    // If we reach here, then SS is an unused synthetic section and we want to
-    // remove it from corresponding input section description of output section.
+    // If we reach here, then ss is an unused synthetic section and we want to
+    // remove it from the corresponding input section description, and
+    // orphanSections.
     for (BaseCommand *b : os->sectionCommands)
       if (auto *isd = dyn_cast<InputSectionDescription>(b))
         llvm::erase_if(isd->sections,
                        [=](InputSection *isec) { return isec == ss; });
+    llvm::erase_if(script->orphanSections,
+                   [=](const InputSectionBase *isec) { return isec == ss; });
   }
 }
 
 // Create output section objects and add them to OutputSections.
 template <class ELFT> void Writer<ELFT>::finalizeSections() {
   Out::preinitArray = findSection(".preinit_array");
   Out::initArray = findSection(".init_array");
   Out::finiArray = findSection(".fini_array");
template <class ELFT> void Writer<ELFT>::finalizeSections() {
   // Do not proceed if there was an undefined symbol.
   if (errorCount())
     return;
 
   if (in.mipsGot)
     in.mipsGot->build();
 
   removeUnusedSyntheticSections();
+  script->diagnoseOrphanHandling();
 
   sortSections();
 
   // Now that we have the final list, create a list of all the
   // OutputSections for convenience.
   for (BaseCommand *base : script->sectionCommands)
     if (auto *sec = dyn_cast<OutputSection>(base))
       outputSections.push_back(sec);

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

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: echo "SECTIONS { .text : { *(.text.1) } }" > %t.script
 
 ## Check we do not report orphans by default even with -verbose.
 # RUN: ld.lld -shared -o %t.out --script %t.script %t.o 2>&1 -verbose \
 # RUN:   | FileCheck %s --check-prefix=DEFAULT
 # DEFAULT-NOT: placed
 
 ## Check --orphan-handling=place has the same behavior as default.
 # RUN: ld.lld -shared --orphan-handling=place -o %t.out --script %t.script \
 # RUN:   %t.o 2>&1 -verbose  -error-limit=0 | FileCheck %s --check-prefix=DEFAULT
 
 ## Check --orphan-handling=error reports errors about orphans.
-# RUN: not ld.lld -shared --orphan-handling=error -o /dev/null --script %t.script \
-# RUN:   %t.o 2>&1 -verbose  -error-limit=0 | FileCheck %s --check-prefix=REPORT
-# REPORT:      {{.*}}.o:(.text) is being placed in '.text'
-# REPORT-NEXT: {{.*}}.o:(.text.2) is being placed in '.text'
-# REPORT-NEXT: <internal>:(.comment) is being placed in '.comment'
-# REPORT-NEXT: <internal>:(.bss) is being placed in '.bss'
-# REPORT-NEXT: <internal>:(.bss.rel.ro) is being placed in '.bss.rel.ro'
-# REPORT-NEXT: <internal>:(.dynsym) is being placed in '.dynsym'
-# REPORT-NEXT: <internal>:(.gnu.version) is being placed in '.gnu.version'
-# REPORT-NEXT: <internal>:(.gnu.version_r) is being placed in '.gnu.version_r'
-# REPORT-NEXT: <internal>:(.gnu.hash) is being placed in '.gnu.hash'
-# REPORT-NEXT: <internal>:(.hash) is being placed in '.hash'
-# REPORT-NEXT: <internal>:(.dynamic) is being placed in '.dynamic'
-# REPORT-NEXT: <internal>:(.dynstr) is being placed in '.dynstr'
-# REPORT-NEXT: <internal>:(.rela.dyn) is being placed in '.rela.dyn'
-# REPORT-NEXT: <internal>:(.eh_frame) is being placed in '.eh_frame'
-# REPORT-NEXT: <internal>:(.got) is being placed in '.got'
-# REPORT-NEXT: <internal>:(.got.plt) is being placed in '.got.plt'
-# REPORT-NEXT: <internal>:(.got.plt) is being placed in '.got.plt'
-# REPORT-NEXT: <internal>:(.rela.plt) is being placed in '.rela.plt'
-# REPORT-NEXT: <internal>:(.rela.dyn) is being placed in '.rela.dyn'
-# REPORT-NEXT: <internal>:(.plt) is being placed in '.plt'
-# REPORT-NEXT: <internal>:(.iplt) is being placed in '.iplt'
-# REPORT-NEXT: <internal>:(.symtab) is being placed in '.symtab'
-# REPORT-NEXT: <internal>:(.symtab_shndx) is being placed in '.symtab_shndx'
-# REPORT-NEXT: <internal>:(.shstrtab) is being placed in '.shstrtab'
-# REPORT-NEXT: <internal>:(.strtab) is being placed in '.strtab'
+# RUN: not ld.lld --orphan-handling=error -o /dev/null -T %t.script \
+# RUN:   %t.o 2>&1 | FileCheck %s --check-prefixes=COMMON,SYMTAB
+
+## --strip-all discards .strtab and .symtab sections. Don't error about them.
+# RUN: not ld.lld --orphan-handling=error --strip-all -o /dev/null -T %t.script \
+# RUN:   %t.o 2>&1 | FileCheck %s --check-prefix=COMMON
+
+## -shared enables some .dynsym related sections.
+# RUN: not ld.lld -shared --orphan-handling=error -o /dev/null -T %t.script \
+# RUN:   %t.o 2>&1 | FileCheck %s --check-prefixes=COMMON,DYNSYM,SYMTAB
+
+# COMMON:      {{.*}}.o:(.text) is being placed in '.text'
+# COMMON-NEXT: {{.*}}.o:(.text.2) is being placed in '.text'
+# COMMON-NEXT: <internal>:(.comment) is being placed in '.comment'
+# DYNSYM-NEXT: <internal>:(.dynsym) is being placed in '.dynsym'
+# DYNSYM-NEXT: <internal>:(.gnu.hash) is being placed in '.gnu.hash'
+# DYNSYM-NEXT: <internal>:(.hash) is being placed in '.hash'
+# DYNSYM-NEXT: <internal>:(.dynamic) is being placed in '.dynamic'
+# DYNSYM-NEXT: <internal>:(.dynstr) is being placed in '.dynstr'
+# SYMTAB-NEXT: <internal>:(.symtab) is being placed in '.symtab'
+# COMMON-NEXT: <internal>:(.shstrtab) is being placed in '.shstrtab'
+# SYMTAB-NEXT: <internal>:(.strtab) is being placed in '.strtab'
+# COMMON-NOT: <internal>
 
 ## Check --orphan-handling=warn reports warnings about orphans.
-# RUN: ld.lld -shared --orphan-handling=warn -o %t.out --script %t.script \
-# RUN:   %t.o 2>&1 -verbose | FileCheck %s --check-prefix=REPORT
+# RUN: ld.lld --orphan-handling=warn -o /dev/null -T %t.script \
+# RUN:   %t.o 2>&1 | FileCheck %s --check-prefixes=COMMON,SYMTAB
 
 # RUN: not ld.lld --orphan-handling=foo -o /dev/null --script %t.script %t.o 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=UNKNOWN
 # UNKNOWN: unknown --orphan-handling mode: foo
 
 .section .text.1,"a"
  nop
 
 .section .text.2,"a"
  nop