[ELF] - Split processSectionCommands().

[grimar]

Summary

It was suggested in D38582 thread and I think reasonable
cleanup itself.

processSectionCommands() uses createInputSectionList() to construct the
list of input sections. In this patch I changed createInputSectionList() to return
Optional<>, so that now it filters out sections that does not satisfy ONLY_IF_RO/ONLY_IF_RW
constraints and sections that are discarded using /DISCARD/ on its side. That allows
to reduce loop in processSectionCommands() and I think looks more natural.

[ruiu]

Still not sure if this is an improvement. When I ask for splitting a loop, it doesn't ask for moving code from a loop to a new function, but actually split a loop. For example, assume this loop:

for (...) {
  A;
  B;
}

When you split the loop, you'll get this:

for (...)
  A;
for (...)
  B;

This is I think an improvement because it is now clear that A and B doesn't depend each other. B might depend on A, but it is still better than mutually-dependent code. But if you just move code outside of the loop like this:

for (...) {
  doA();
  doB();
}

func doA() { A; }
func doB() { B; }

the complexity of the new code is the same as before. It doesn't make things logically simpler.

[grimar]

In D39418#912510, @ruiu wrote:

Still not sure if this is an improvement. When I ask for splitting a loop, it doesn't ask for moving code from a loop to a new function, but actually split a loop. For example, assume this loop:

for (...) {
  A;
  B;
}

When you split the loop, you'll get this:

for (...)
  A;
for (...)
  B;

This is I think an improvement because it is now clear that A and B doesn't depend each other. B might depend on A, but it is still better than mutually-dependent code. But if you just move code outside of the loop like this:

I see what you mean. Issue here that A and B are actually dependent. This loop simplified representation is:

for (...) {
 createSymbol();
 createSection();
}

If I would split it to:

for (...) {
 createSymbol();
}
for (...) {
 createSection();
}

It would break absolute2.s which script is SECTIONS { .text : { *(.text) } foo = ABSOLUTE(_start) + _start; };
And error would be: unable to evaluate expression: input section .text has no output section assigned
because assignment tries to access symbol in .text section wich is not yet created.

If I would split it to reversed order:

for (...) {
 createSection();
}
for (...) {
 createSymbol();
}

Then it would break early-assign-symbol.s case which script is SECTIONS { aaa = foo | 1; .text : { *(.text*) } }
And it checks that we report "{{.*}}.script:1: unable to evaluate expression: input section .text has no output section assigned" here.

These experiments does not take into account that we also create symbols inside sections, what might add additional issues
when trying to split this logic out. And generally I think such dependency is correct and does not feel like it worth splitting as it not simple.

What we can do here is to reduce the loop itself and that is what I tried to do in this patch.

ELF/LinkerScript.h

class LinkerScript final {
   void addSymbol(SymbolAssignment *Cmd);
   void assignSymbol(SymbolAssignment *Cmd, bool InSec);
   void setDot(Expr E, const Twine &Loc, bool InSec);
 
   std::vector<InputSection *>
   computeInputSections(const InputSectionDescription *,
                        const llvm::DenseMap<SectionBase *, int> &Order);
 
-  std::vector<InputSection *>
+  llvm::Optional<std::vector<InputSection *>>
   createInputSectionList(OutputSection &Cmd,
                          const llvm::DenseMap<SectionBase *, int> &Order);
 
   std::vector<size_t> getPhdrIndices(OutputSection *Sec);
 
   MemoryRegion *findMemoryRegion(OutputSection *Sec);
 
   void switchTo(OutputSection *Sec);

ELF/LinkerScript.cpp

if (S == InX::ShStrTab || S == InX::Dynamic || S == InX::DynSymTab ||
       error("discarding " + S->Name + " section is not allowed");
 
     S->Assigned = false;
     S->Live = false;
     discard(S->DependentSections);
   }
 }
 
-std::vector<InputSection *> LinkerScript::createInputSectionList(
+// Method is used to build properly sorted input sections list for given output
+// section command. Returns the list constructed or None in case if sections are
+// discarded with /DISCARD/ or does not satisfy ONLY_IF_R[O|W] constraints.
+Optional<std::vector<InputSection *>> LinkerScript::createInputSectionList(
     OutputSection &OutCmd, const DenseMap<SectionBase *, int> &Order) {
+  // Create a list of input sections matching sections descriptions.
   std::vector<InputSection *> Ret;
-
   for (BaseCommand *Base : OutCmd.SectionCommands) {
     if (auto *Cmd = dyn_cast<InputSectionDescription>(Base)) {
       Cmd->Sections = computeInputSections(Cmd, Order);
       Ret.insert(Ret.end(), Cmd->Sections.begin(), Cmd->Sections.end());
     }
   }
+
+  // The output section name `/DISCARD/' is special.
+  // Any input section assigned to it is discarded.
+  if (OutCmd.Name == "/DISCARD/") {
+    discard(Ret);
+    return None;
+  }
+
+  // This is for ONLY_IF_RO and ONLY_IF_RW. An output section directive
+  // ".foo : ONLY_IF_R[OW] { ... }" is handled only if all member input
+  // sections satisfy a given constraint.
+  if (!matchConstraints(Ret, OutCmd.Constraint)) {
+    for (InputSectionBase *S : Ret)
+      S->Assigned = false;
+    return None;
+  }
+
   return Ret;
 }
 
 void LinkerScript::processSectionCommands() {
   // 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
void LinkerScript::processSectionCommands() {
   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])) {
-      std::vector<InputSection *> V = createInputSectionList(*Sec, Order);
-
-      // The output section name `/DISCARD/' is special.
-      // Any input section assigned to it is discarded.
-      if (Sec->Name == "/DISCARD/") {
-        discard(V);
-        continue;
-      }
-
-      // This is for ONLY_IF_RO and ONLY_IF_RW. An output section directive
-      // ".foo : ONLY_IF_R[OW] { ... }" is handled only if all member input
-      // sections satisfy a given constraint. If not, a directive is handled
-      // as if it wasn't present from the beginning.
-      //
-      // Because we'll iterate over SectionCommands many more times, the easiest
-      // way to "make it as if it wasn't present" is to just remove it.
-      if (!matchConstraints(V, Sec->Constraint)) {
-        for (InputSectionBase *S : V)
-          S->Assigned = false;
+      // We want to build a list of input sections to work with. If list is
+      // absent that means all sections are either discarded or does not satisfy
+      // given output command constraints. In that case we want to ban such
+      // command because will iterate over SectionCommands many more times. The
+      // easiest way to "make it as if it wasn't present" is to just remove it.
+      Optional<std::vector<InputSection *>> V = createInputSectionList(*Sec, Order);
+      if (!V) {
         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)
+        for (InputSectionBase *S : *V)
           S->Alignment = Subalign;
       }
 
       // Add input sections to an output section.
-      for (InputSection *S : V)
+      for (InputSection *S : *V)
         Sec->addSection(S);
     }
   }
   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) {