Refactor how we decide which sections to sort

[• espindola]

Summary

This is a bit more verbose, but it has a few advantages.

The logic on what to do with special sections like .init_array is not duplicated. Before we would need keep isKnownNonreorderableSection in sync.

I think with this the call graph based sorting can be implemented by "just" returning a new order from buildSectionOrder.

[ruiu]

LGTM

I like the new code. That's slightly verbose but much easier to read.

[Bigcheese]

Seems fine and removes the duplication of names. My only concern is mixing --symbol-ordering-file with call graph sort in buildSectionOrder. Currently in the patch call graph sorting is applied then the order file. With this the two would need to be mixed into a single DenseMap, or the loop would need to be duplicated (which is what the patch currently does).

[• espindola]

r324744.

ELF/Writer.cpp

   return SectionOrder;
 }
 
-static bool isKnownNonreorderableSection(const OutputSection *OS) {
-  return llvm::StringSwitch<bool>(OS->Name)
-      .Cases(".init", ".fini", ".init_array", ".fini_array", ".ctors",
-             ".dtors", true)
-      .Default(false);
-}
-
-// If no layout was provided by linker script, we want to apply default
-// sorting for special input sections. This also handles --symbol-ordering-file.
-template <class ELFT> void Writer<ELFT>::sortInputSections() {
-  // Sort input sections by priority using the list provided
-  // by --symbol-ordering-file.
-  DenseMap<SectionBase *, int> Order = buildSectionOrder();
-  if (!Order.empty())
-    for (BaseCommand *Base : Script->SectionCommands)
-      if (auto *Sec = dyn_cast<OutputSection>(Base))
-        if (Sec->Live && !isKnownNonreorderableSection(Sec))
-          Sec->sort([&](InputSectionBase *S) { return Order.lookup(S); });
-
-  if (Script->HasSectionsCommand)
+static void sortSection(OutputSection *Sec,
+                        const DenseMap<SectionBase *, int> &Order) {
+  if (!Sec->Live)
     return;
+  StringRef Name = Sec->Name;
 
   // Sort input sections by section name suffixes for
   // __attribute__((init_priority(N))).
-  if (OutputSection *Sec = findSection(".init_array"))
-    Sec->sortInitFini();
-  if (OutputSection *Sec = findSection(".fini_array"))
-    Sec->sortInitFini();
+  if (Name == ".init_array" || Name == ".fini_array") {
+    if (!Script->HasSectionsCommand)
+      Sec->sortInitFini();
+    return;
+  }
 
   // Sort input sections by the special rule for .ctors and .dtors.
-  if (OutputSection *Sec = findSection(".ctors"))
-    Sec->sortCtorsDtors();
-  if (OutputSection *Sec = findSection(".dtors"))
-    Sec->sortCtorsDtors();
+  if (Name == ".ctors" || Name == ".dtors") {
+    if (!Script->HasSectionsCommand)
+      Sec->sortCtorsDtors();
+    return;
+  }
+
+  // Never sort these.
+  if (Name == ".init" || Name == ".fini")
+    return;
+
+  // Sort input sections by priority using the list provided
+  // by --symbol-ordering-file.
+  if (!Order.empty())
+    Sec->sort([&](InputSectionBase *S) { return Order.lookup(S); });
+}
+
+// If no layout was provided by linker script, we want to apply default
+// sorting for special input sections. This also handles --symbol-ordering-file.
+template <class ELFT> void Writer<ELFT>::sortInputSections() {
+  // Build the order once since it is expensive.
+  DenseMap<SectionBase *, int> Order = buildSectionOrder();
+  for (BaseCommand *Base : Script->SectionCommands)
+    if (auto *Sec = dyn_cast<OutputSection>(Base))
+      sortSection(Sec, Order);
 }
 
 template <class ELFT> void Writer<ELFT>::sortSections() {