[ELF] - Change how we deal with unused synthetic sections.

[grimar]

Summary

It turns out our code that handles unused synthetic sections is
a bit overcomplicated.
I think we set output section Live bit too early and do things in
removeUnusedSyntheticSections that we can skip.

I suggest to set Live bit of output section early only if it is
known that it is live. Then we can update this flag after we get
final information about synthetic section status (empty/non-empty).
For input sections that are empty we could just disable them,
setting their Live flag to false.

Approach I am suggesting is implemented in this patch.

[davide]

I'm not sure how I feel about this.
It's true that reduces the number of lines to implement part of this feature.
OTOH, it scatters details that were in a single place into multiple (three, to be precise).
In some sense,it makes following this code harder to read.

[grimar]

In D38613#891337, @davide wrote:

I'm not sure how I feel about this.
It's true that reduces the number of lines to implement part of this feature.
OTOH, it scatters details that were in a single place into multiple (three, to be precise).
In some sense,it makes following this code harder to read.

I understand your concerns.

It was suggested in D38393 thread to design something
different than removeUnusedSynteticSections.
Also there was also discussion about behavior of this function in D37520 thread.

Designing something different anyways requires touching code outside of this method it seems.
We normally rely on Live flag when trying to control whether input or output section be in output.
In this patch I am just trying to be consistent with the rest LLD code.

I have no better ideas how to rework it at this moment. Initially I posted D37520
to fix silly issue we have currently. We could probably just land it for now.

[ruiu]

I think Live bit is abused in the output sections. That flag was introduced for gc, and originally only input sections have the flag. After that, a new class was introduced as a parent of both input sections and output sections, and at some point, we started using Live bit for output sections to indicate whether we want to emit it or not. But as far as I know, it was not discussed if that is a good approach.

I feel like we should stop using Live bit for output sections. Input sections need that flag because they are gc'ed, but for output sections, we shouldn't create useless output sections in the first place. I feel like the current output sections have too many states and that makes writing code harder.

ELF/MapFile.cpp

for (OutputSection *OSec : OutputSections) {
     OS << OSec->Name << '\n';
 
     // Dump symbols for each input section.
     for (BaseCommand *Base : OSec->Commands) {
       auto *ISD = dyn_cast<InputSectionDescription>(Base);
       if (!ISD)
         continue;
       for (InputSection *IS : ISD->Sections) {
+        if (!IS->Live)
+          continue;
         writeHeader(OS, OSec->Addr + IS->OutSecOff, IS->getSize(),
                     IS->Alignment);
         OS << indent(1) << toString(IS) << '\n';
         for (Defined *Sym : SectionSyms[IS])
           OS << SymStr[Sym] << '\n';
       }
     }
   }
 }
 
 template void elf::writeMapFile<ELF32LE>();
 template void elf::writeMapFile<ELF32BE>();
 template void elf::writeMapFile<ELF64LE>();
 template void elf::writeMapFile<ELF64BE>();

ELF/OutputSections.cpp

 static uint64_t updateOffset(uint64_t Off, InputSection *S) {
   Off = alignTo(Off, S->Alignment);
   S->OutSecOff = Off;
   return Off + S->getSize();
 }
 
 void OutputSection::addSection(InputSection *S) {
   assert(S->Live);
+
+  // Adding regular sections should trigger live bit, so that we enable emiting
+  // this output section. Synthetic sections are exception, because we do not
+  // know their final status here yet. So we set Live flag here only if we
+  // definetely know we are going to keep this synthetic section.
+  if (SyntheticSection *SS = dyn_cast<SyntheticSection>(S))
+    Live |= !SS->empty();
+  else
     Live = true;
+
   S->Parent = this;
   this->updateAlignment(S->Alignment);
 
   // The actual offsets will be computed by assignAddresses. For now, use
   // crude approximation so that it is at least easy for other code to know the
   // section order. It is also used to calculate the output section size early
   // for compressed debug sections.
   this->Size = updateOffset(Size, S);

ELF/Writer.cpp

 // even before we make decisions whether they are needed. This allows
 // users to write scripts like this: ".mygot : { .got }".
 //
 // Doing it has an unintended side effects. If it turns out that we
 // don't need a .got (for example) at all because there's no
 // relocation that needs a .got, we don't want to emit .got.
 //
 // To deal with the above problem, this function is called after
-// scanRelocations is called to remove synthetic sections that turn
-// out to be empty.
-static void removeUnusedSyntheticSections() {
-  // All input synthetic sections that can be empty are placed after
-  // all regular ones. We iterate over them all and exit at first
-  // non-synthetic.
-  for (InputSectionBase *S : llvm::reverse(InputSections)) {
-    SyntheticSection *SS = dyn_cast<SyntheticSection>(S);
-    if (!SS)
-      return;
-    OutputSection *OS = SS->getParent();
-    if (!SS->empty() || !OS)
-      continue;
-
-    std::vector<BaseCommand *>::iterator Empty = OS->Commands.end();
-    for (auto I = OS->Commands.begin(), E = OS->Commands.end(); I != E; ++I) {
-      BaseCommand *B = *I;
-      if (auto *ISD = dyn_cast<InputSectionDescription>(B)) {
-        llvm::erase_if(ISD->Sections,
-                       [=](InputSection *IS) { return IS == SS; });
-        if (ISD->Sections.empty())
-          Empty = I;
-      }
-    }
-    if (Empty != OS->Commands.end())
-      OS->Commands.erase(Empty);
-
-    // If there are no other sections in the output section, remove it from the
-    // output.
-    if (OS->Commands.empty())
-      OS->Live = false;
+// scanRelocations to disable synthetic sections that turn out to be
+// empty. If we find that section should be kept, we must update output
+// section live bit to ensure it is emited.
+static void disableUnusedSyntheticSections() {
+  for (InputSectionBase *S : InputSections) {
+    if (SyntheticSection *SS = dyn_cast<SyntheticSection>(S)) {
+      SS->Live = !SS->empty();
+      if (OutputSection *OS = SS->getParent())
+        OS->Live |= SS->Live;
+    }
   }
 }
 
 // Returns true if a symbol can be replaced at load-time by a symbol
 // with the same name defined in other ELF executable or DSO.
 static bool computeIsPreemptible(const SymbolBody &B) {
   assert(!B.isLocal());
   // Only symbols that appear in dynsym can be preempted.
for (Symbol *S : Symtab->getSymbols()) {
     }
   }
 
   // Do not proceed if there was an undefined symbol.
   if (ErrorCount)
     return;
 
   addPredefinedSections();
-  removeUnusedSyntheticSections();
+  disableUnusedSyntheticSections();
 
   sortSections();
   Script->removeEmptyCommands();
 
   // Now that we have the final list, create a list of all the
   // OutputSections for convenience.
   for (BaseCommand *Base : Script->Opt.Commands)
     if (auto *Sec = dyn_cast<OutputSection>(Base))