[ELF] - Cleanup of processSectionCommands().

[grimar]

Summary

The way we handle ONLY_IF_RO/OLY_IF_RW constraints in
processSectionCommands is a bit tricky. If input sections
does no satisfy given constraint we remove command from
commands list. It seems too complex, what we can do instead
is to make the OutputCommand empty. So that at later steps
LLD will remove it just like it deal with other empty output commands.
That allows to simplify the loop a bit.

[ruiu]

Your change to ELF/LinkerScript.cpp looks good, but the other change needs improving.

ELF/OutputSections.cpp

119–120 ↗

(On Diff #119584)

This function is addSection, which is supposed to add a section to an output section, right? Adding this piece of code which is not related to adding sections to this function is not a good idea. This is how this function organically grown (and I took quite a long time to clean it up), so when you add code to some function, please stop and think if what you are about to do is semantically correct.

[grimar]

ELF/OutputSections.cpp

119–120 ↗

(On Diff #119584)

This function not only adds section, but also initializes and changes the Type.
And that happened before my change already, so I assume it is proper place to set Type because
it probably desirable to change it in a single place of code, no ?

We can split this code (lines 97-120) to something like setSectionType, but that would be separate refactoring change.
Will such change be OK for you ?

[ruiu]

ELF/OutputSections.cpp

119–120 ↗

(On Diff #119584)

George, I knew that was the reason why you added this piece of code to this function, but this is not right. If addSection mutates its class' Type based on a given argument, it is fine, but if it does something that is not related to a given section every time you pass a section to the function, something's not correct.

[grimar]

• Addressed review comment.

ELF/OutputSections.cpp

119–120 ↗

(On Diff #119584)

I see. Fixed.

[ruiu]

LGTM

lld/trunk/ELF/LinkerScript.cpp

void LinkerScript::processSectionCommands() {
   // 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.
   auto Deleter = make_unique<AddressState>();
   Ctx = Deleter.get();
   Ctx->OutSec = Aether;
 
+  size_t I = 0;
   DenseMap<SectionBase *, int> Order = buildSectionOrder();
   // Add input sections to output sections.
-  for (size_t I = 0; I < SectionCommands.size(); ++I) {
+  for (BaseCommand *Base : SectionCommands) {
     // Handle symbol assignments outside of any output section.
-    if (auto *Cmd = dyn_cast<SymbolAssignment>(SectionCommands[I])) {
+    if (auto *Cmd = dyn_cast<SymbolAssignment>(Base)) {
       addSymbol(Cmd);
       continue;
     }
 
-    if (auto *Sec = dyn_cast<OutputSection>(SectionCommands[I])) {
+    if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       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.
+      // Because we'll iterate over SectionCommands many more times, the easy
+      // way to "make it as if it wasn't present" is to make it empty.
       if (!matchConstraints(V, Sec->Constraint)) {
         for (InputSectionBase *S : V)
           S->Assigned = false;
-        SectionCommands.erase(SectionCommands.begin() + I);
-        --I;
+        Sec->SectionCommands.clear();
         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);
-    }
-  }
-  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;
+      Sec->SectionIndex = I++;
       if (Sec->Noload)
         Sec->Type = SHT_NOBITS;
     }
   }
+  Ctx = nullptr;
+}
 
 static OutputSection *findByName(ArrayRef<BaseCommand *> Vec,
                                  StringRef Name) {
   for (BaseCommand *Base : Vec)
     if (auto *Sec = dyn_cast<OutputSection>(Base))
       if (Sec->Name == Name)
         return Sec;
   return nullptr;