[ELF] - Get rid of LinkerScript::adjustSectionsBeforeSorting().

[grimar]

Summary

I suggest to get rid of LinkerScript::adjustSectionsBeforeSorting() method.
It is used to force output sections that does not have any input sections
to be live when they have data bytes commands or assignments inside.

Probably there is no reason to delay that and do separatelly, I believe it
is simpler to set this flag when we do linker script commands proccessing,
as at that place we already know everything we need to do that.

[grimar]

• Better version.

[ruiu]

ELF/LinkerScript.cpp

441–444

I don't like to do more things in a single for-loop. Previously, this loop was just three lines. Now it does more things. It is hard to understand now. Please try to split it into smaller pieces.

449–457

I really do not understand how this comment explains the following code.

[grimar]

• Addressed review comments.

[grimar]

• Cosmetic changes.

[grimar]

Ping.

• Rebased, fixed comment.

[ruiu]

Hmm, does this actually make things easier to understand? I'm not sure about that. At least, when I ask you to split the loop, I was talking about the outermost loop. You created a small function for the innermost part, but that's not what I meant.

[grimar]

I created D39418 to split the loop.
I'll rebase this patch later if/when we land above one.

Also it may worth to land D39094 to simplify it a bit too.

ELF/LinkerScript.h

public:
   uint64_t getDot() { return Dot; }
   void discard(ArrayRef<InputSection *> V);
 
   ExprValue getSymbolValue(StringRef Name, const Twine &Loc);
 
   void fabricateDefaultCommands();
   void addOrphanSections(OutputSectionFactory &Factory);
   void removeEmptyCommands();
-  void adjustSectionsBeforeSorting();
   void adjustSectionsAfterSorting();
 
   std::vector<PhdrEntry *> createPhdrs();
   bool needsInterpSection();
 
   bool shouldKeep(InputSectionBase *S);
   void assignAddresses();
   void allocateHeaders(std::vector<PhdrEntry *> &Phdrs);

ELF/LinkerScript.cpp

for (BaseCommand *Base : OutCmd.SectionCommands) {
     if (auto *Cmd = dyn_cast<InputSectionDescription>(Base)) {
       Cmd->Sections = computeInputSections(Cmd);
       Ret.insert(Ret.end(), Cmd->Sections.begin(), Cmd->Sections.end());
     }
   }
   return Ret;
 }
 
+// If the output section contains only symbol assignments, we 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 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.
+static void keepSection(OutputSection *Sec, Optional<uint64_t> Flags) {
+  if (Sec->Live)
+    return;
+  Sec->Live = true;
+  Sec->Flags = SHF_ALLOC;
+  if (Flags)
+    Sec->Flags |= (*Flags & (SHF_WRITE | SHF_EXECINSTR));
+}
+
 void LinkerScript::processSectionCommands(OutputSectionFactory &Factory) {
   // A symbol can be assigned before any section is mentioned in the linker
   // script. In an DSO, the symbol values are addresses, so the only important
   // section values are:
   // * SHN_UNDEF
   // * SHN_ABS
   // * Any value meaning a regular section.
   // To handle that, create a dummy aether section that fills the void before
   // the linker scripts switches to another section. It has an index of one
   // which will map to whatever the first actual section is.
   Aether = make<OutputSection>("", 0, SHF_ALLOC);
   Aether->SectionIndex = 1;
 
   // Ctx captures the local AddressState and makes it accessible deliberately.
   // This is needed as there are some cases where we cannot just
   // thread the current state through to a lambda function created by the
   // script parser.
   Ctx = make_unique<AddressState>();
   Ctx->OutSec = Aether;
 
   // Add input sections to output sections.
+  Optional<uint64_t> PrevFlags;
   for (size_t I = 0; I < SectionCommands.size(); ++I) {
     // Handle symbol assignments outside of any output section.
     if (auto *Cmd = dyn_cast<SymbolAssignment>(SectionCommands[I])) {
       addSymbol(Cmd);
       continue;
     }
 
     if (auto *Sec = dyn_cast<OutputSection>(SectionCommands[I])) {
if (auto *Sec = dyn_cast<OutputSection>(SectionCommands[I])) {
       if (!matchConstraints(V, Sec->Constraint)) {
         for (InputSectionBase *S : V)
           S->Assigned = false;
         SectionCommands.erase(SectionCommands.begin() + I);
         --I;
         continue;
       }
 
-      // A directive may contain symbol definitions like this:
-      // ".foo : { ...; bar = .; }". Handle them.
-      for (BaseCommand *Base : Sec->SectionCommands)
-        if (auto *OutCmd = dyn_cast<SymbolAssignment>(Base))
-          addSymbol(OutCmd);
-
       // Handle subalign (e.g. ".foo : SUBALIGN(32) { ... }"). If subalign
       // is given, input sections are aligned to that value, whether the
       // given value is larger or smaller than the original section alignment.
       if (Sec->SubalignExpr) {
         uint32_t Subalign = Sec->SubalignExpr().getValue();
         for (InputSectionBase *S : V)
           S->Alignment = Subalign;
       }
 
       // Add input sections to an output section.
       for (InputSection *S : V)
         Sec->addSection(S);
+
+      // We are doing next things here:
+      // 1) Handle symbol definitions if directive contains them, e.g:
+      //    .foo : { ...; bar = .; }
+      // 2) Keep some output sections in even if them have no input
+      //    sections inside. See comment for keepSection.
+      for (BaseCommand *Base : Sec->SectionCommands) {
+        if (auto *OutCmd = dyn_cast<SymbolAssignment>(Base))
+          addSymbol(OutCmd);
+        if (!isa<InputSectionDescription>(*Base))
+          keepSection(Sec, PrevFlags);
+      }
+      if (Sec->Live)
+        PrevFlags = Sec->Flags;
     }
   }
   Ctx = nullptr;
 
   // Output sections are emitted in the exact same order as
   // appeared in SECTIONS command, so we know their section indices.
   for (size_t I = 0; I < SectionCommands.size(); ++I) {
     auto *Sec = dyn_cast<OutputSection>(SectionCommands[I]);
     if (!Sec)
       continue;
     assert(Sec->SectionIndex == INT_MAX);
     Sec->SectionIndex = I;
     if (Sec->Noload)
       Sec->Type = SHT_NOBITS;
   }
void LinkerScript::removeEmptyCommands() {
   // more aggressive at removing them.
   llvm::erase_if(SectionCommands, [&](BaseCommand *Base) {
     if (auto *Sec = dyn_cast<OutputSection>(Base))
       return !Sec->Live;
     return false;
   });
 }
 
-static bool isAllSectionDescription(const OutputSection &Cmd) {
-  for (BaseCommand *Base : Cmd.SectionCommands)
-    if (!isa<InputSectionDescription>(*Base))
-      return false;
-  return true;
-}
-
-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 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;
-
-  for (BaseCommand *Cmd : SectionCommands) {
-    auto *Sec = dyn_cast<OutputSection>(Cmd);
-    if (!Sec)
-      continue;
-    if (Sec->Live) {
-      Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
-      continue;
-    }
-
-    if (isAllSectionDescription(*Sec))
-      continue;
-
-    Sec->Live = true;
-    Sec->Flags = Flags;
-  }
-}
-
 void LinkerScript::adjustSectionsAfterSorting() {
   // Try and find an appropriate memory region to assign offsets in.
   for (BaseCommand *Base : SectionCommands) {
     if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       if (!Sec->Live)
         continue;
       Sec->MemRegion = findMemoryRegion(Sec);
       // Handle align (e.g. ".foo : ALIGN(16) { ... }").

ELF/Writer.cpp

if (NextSec == E)
     return E;
 
   while (I != E && shouldSkip(*I))
     ++I;
   return I;
 }
 
 template <class ELFT> void Writer<ELFT>::sortSections() {
-  Script->adjustSectionsBeforeSorting();
-
   // Don't sort if using -r. It is not necessary and we want to preserve the
   // relative order for SHF_LINK_ORDER sections.
   if (Config->Relocatable)
     return;
 
   for (BaseCommand *Base : Script->SectionCommands)
     if (auto *Sec = dyn_cast<OutputSection>(Base))
       Sec->SortRank = getSectionRank(Sec);