[lld-macho][nfc] Remove unnecessary parameterization of section sort

[int3]

Summary

As @alexshap pointed out here,
it's a bit confusing to have the option to sort OutputSections with any
comparator when in practice we only use one.

[thakis]

Once we handle N_COLD_FUNC this will also affect ordering. I don't know if that'd go in the one comparator or if it'd use a different comparator. If it's the latter, maybe we shouldn't change this?

[int3]

I think it'd use/extend the same comparator, since much of the ordering defined in the current comparator is needed for correctness.

[thakis]

Sounds good :)

lld/MachO/OutputSegment.cpp

98

wrap to 80

[int3]

lld/MachO/OutputSegment.cpp

98

it's already wrapped at 80 :)

[alexander-shaposhnikov]

lld/MachO/OutputSegment.cpp

65

static auto compareByOrder(...) {
   return ...
}

lld/MachO/OutputSegment.h

 class InputSection;
 
 class OutputSegment {
 public:
   const OutputSection *firstSection() const { return sections.front(); }
   const OutputSection *lastSection() const { return sections.back(); }
 
   void addOutputSection(OutputSection *os);
-  void sortOutputSections(
-      llvm::function_ref<bool(OutputSection *, OutputSection *)> comparator) {
-    llvm::sort(sections, comparator);
-  }
+  void sortOutputSections();
 
   const std::vector<OutputSection *> &getSections() const { return sections; }
   size_t numNonHiddenSections() const;
 
   uint64_t fileOff = 0;
   uint64_t fileSize = 0;
   uint64_t vmSize = 0;
   int inputOrder = std::numeric_limits<int>::max();
   StringRef name;
   uint32_t maxProt = 0;
   uint32_t initProt = 0;
   uint8_t index;
 
 private:
   std::vector<OutputSection *> sections;
 };
 
 extern std::vector<OutputSegment *> outputSegments;
 
+void sortOutputSegments();
+
 OutputSegment *getOrCreateOutputSegment(StringRef name);
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/OutputSegment.cpp

 //===- OutputSegment.cpp --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "OutputSegment.h"
 #include "ConcatOutputSection.h"
 #include "InputSection.h"
 #include "SyntheticSections.h"
 
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/MachO.h"
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace lld;
 using namespace lld::macho;
 
 static uint32_t initProt(StringRef name) {
void OutputSegment::addOutputSection(OutputSection *osec) {
   osec->parent = this;
   sections.push_back(osec);
 
   for (const SectionAlign &sectAlign : config->sectionAlignments)
     if (sectAlign.segName == name && sectAlign.sectName == osec->name)
       osec->align = sectAlign.align;
 }
 
+template <typename T, typename F> static auto compareByOrder(F ord) {
+  return [=](T a, T b) { return ord(a) < ord(b); };
+}
+
+static int segmentOrder(OutputSegment *seg) {
+  return StringSwitch<int>(seg->name)
+      .Case(segment_names::pageZero, -4)
+      .Case(segment_names::text, -3)
+      .Case(segment_names::dataConst, -2)
+      .Case(segment_names::data, -1)
+      .Case(segment_names::llvm, std::numeric_limits<int>::max() - 1)
+      // Make sure __LINKEDIT is the last segment (i.e. all its hidden
+      // sections must be ordered after other sections).
+      .Case(segment_names::linkEdit, std::numeric_limits<int>::max())
+      .Default(seg->inputOrder);
+}
+
+static int sectionOrder(OutputSection *osec) {
+  StringRef segname = osec->parent->name;
+  // Sections are uniquely identified by their segment + section name.
+  if (segname == segment_names::text) {
+    return StringSwitch<int>(osec->name)
+        .Case(section_names::header, -4)
+        .Case(section_names::text, -3)
+        .Case(section_names::stubs, -2)
+        .Case(section_names::stubHelper, -1)
+        .Case(section_names::unwindInfo, std::numeric_limits<int>::max() - 1)
+        .Case(section_names::ehFrame, std::numeric_limits<int>::max())
+        .Default(osec->inputOrder);
+  } else if (segname == segment_names::data ||
+             segname == segment_names::dataConst) {
+    // For each thread spawned, dyld will initialize its TLVs by copying the
+    // address range from the start of the first thread-local data section to
+    // the end of the last one. We therefore arrange these sections contiguously
+    // to minimize the amount of memory used. Additionally, since zerofill
+    // sections must be at the end of their segments, and since TLV data
+    // sections can be zerofills, we end up putting all TLV data sections at the
+    // end of the segment.
+    switch (sectionType(osec->flags)) {
+    case S_THREAD_LOCAL_REGULAR:
+      return std::numeric_limits<int>::max() - 2;
+    case S_THREAD_LOCAL_ZEROFILL:
+      return std::numeric_limits<int>::max() - 1;
+    case S_ZEROFILL:
+      return std::numeric_limits<int>::max();
+    default:
+      return StringSwitch<int>(osec->name)
+          .Case(section_names::got, -3)
+          .Case(section_names::lazySymbolPtr, -2)
+          .Case(section_names::const_, -1)
+          .Default(osec->inputOrder);
+    }
+  } else if (segname == segment_names::linkEdit) {
+    return StringSwitch<int>(osec->name)
+        .Case(section_names::rebase, -9)
+        .Case(section_names::binding, -8)
+        .Case(section_names::weakBinding, -7)
+        .Case(section_names::lazyBinding, -6)
+        .Case(section_names::export_, -5)
+        .Case(section_names::functionStarts, -4)
+        .Case(section_names::symbolTable, -3)
+        .Case(section_names::indirectSymbolTable, -2)
+        .Case(section_names::stringTable, -1)
+        .Case(section_names::codeSignature, std::numeric_limits<int>::max())
+        .Default(osec->inputOrder);
+  }
+  // ZeroFill sections must always be the at the end of their segments,
+  // otherwise subsequent sections may get overwritten with zeroes at runtime.
+  if (sectionType(osec->flags) == S_ZEROFILL)
+    return std::numeric_limits<int>::max();
+  return osec->inputOrder;
+}
+
+void OutputSegment::sortOutputSections() {
+  llvm::sort(sections, compareByOrder<OutputSection *>(sectionOrder));
+}
+
+void macho::sortOutputSegments() {
+  llvm::sort(outputSegments, compareByOrder<OutputSegment *>(segmentOrder));
+}
+
 static DenseMap<StringRef, OutputSegment *> nameToOutputSegment;
 std::vector<OutputSegment *> macho::outputSegments;
 
 OutputSegment *macho::getOrCreateOutputSegment(StringRef name) {
   OutputSegment *&segRef = nameToOutputSegment[name];
   if (segRef)
     return segRef;
 
   segRef = make<OutputSegment>();
   segRef->name = name;
   segRef->maxProt = maxProt(name);
   segRef->initProt = initProt(name);
 
   outputSegments.push_back(segRef);
   return segRef;
 }

lld/MachO/Writer.cpp

if (isa<ObjFile>(file))
       for (Symbol *sym : file->symbols)
         if (auto *d = dyn_cast<Defined>(sym))
           addSym(*d);
   }
 
   return sectionPriorities;
 }
 
-static int segmentOrder(OutputSegment *seg) {
-  return StringSwitch<int>(seg->name)
-      .Case(segment_names::pageZero, -4)
-      .Case(segment_names::text, -3)
-      .Case(segment_names::dataConst, -2)
-      .Case(segment_names::data, -1)
-      .Case(segment_names::llvm, std::numeric_limits<int>::max() - 1)
-      // Make sure __LINKEDIT is the last segment (i.e. all its hidden
-      // sections must be ordered after other sections).
-      .Case(segment_names::linkEdit, std::numeric_limits<int>::max())
-      .Default(seg->inputOrder);
-}
-
-static int sectionOrder(OutputSection *osec) {
-  StringRef segname = osec->parent->name;
-  // Sections are uniquely identified by their segment + section name.
-  if (segname == segment_names::text) {
-    return StringSwitch<int>(osec->name)
-        .Case(section_names::header, -4)
-        .Case(section_names::text, -3)
-        .Case(section_names::stubs, -2)
-        .Case(section_names::stubHelper, -1)
-        .Case(section_names::unwindInfo, std::numeric_limits<int>::max() - 1)
-        .Case(section_names::ehFrame, std::numeric_limits<int>::max())
-        .Default(osec->inputOrder);
-  } else if (segname == segment_names::data ||
-             segname == segment_names::dataConst) {
-    // For each thread spawned, dyld will initialize its TLVs by copying the
-    // address range from the start of the first thread-local data section to
-    // the end of the last one. We therefore arrange these sections contiguously
-    // to minimize the amount of memory used. Additionally, since zerofill
-    // sections must be at the end of their segments, and since TLV data
-    // sections can be zerofills, we end up putting all TLV data sections at the
-    // end of the segment.
-    switch (sectionType(osec->flags)) {
-    case S_THREAD_LOCAL_REGULAR:
-      return std::numeric_limits<int>::max() - 2;
-    case S_THREAD_LOCAL_ZEROFILL:
-      return std::numeric_limits<int>::max() - 1;
-    case S_ZEROFILL:
-      return std::numeric_limits<int>::max();
-    default:
-      return StringSwitch<int>(osec->name)
-          .Case(section_names::got, -3)
-          .Case(section_names::lazySymbolPtr, -2)
-          .Case(section_names::const_, -1)
-          .Default(osec->inputOrder);
-    }
-  } else if (segname == segment_names::linkEdit) {
-    return StringSwitch<int>(osec->name)
-        .Case(section_names::rebase, -9)
-        .Case(section_names::binding, -8)
-        .Case(section_names::weakBinding, -7)
-        .Case(section_names::lazyBinding, -6)
-        .Case(section_names::export_, -5)
-        .Case(section_names::functionStarts, -4)
-        .Case(section_names::symbolTable, -3)
-        .Case(section_names::indirectSymbolTable, -2)
-        .Case(section_names::stringTable, -1)
-        .Case(section_names::codeSignature, std::numeric_limits<int>::max())
-        .Default(osec->inputOrder);
-  }
-  // ZeroFill sections must always be the at the end of their segments,
-  // otherwise subsequent sections may get overwritten with zeroes at runtime.
-  if (sectionType(osec->flags) == S_ZEROFILL)
-    return std::numeric_limits<int>::max();
-  return osec->inputOrder;
-}
-
-template <typename T, typename F>
-static std::function<bool(T, T)> compareByOrder(F ord) {
-  return [=](T a, T b) { return ord(a) < ord(b); };
-}
-
 // Sorting only can happen once all outputs have been collected. Here we sort
 // segments, output sections within each segment, and input sections within each
 // output segment.
 static void sortSegmentsAndSections() {
   TimeTraceScope timeScope("Sort segments and sections");
-
-  llvm::sort(outputSegments, compareByOrder<OutputSegment *>(segmentOrder));
+  sortOutputSegments();
 
   DenseMap<const InputSection *, size_t> isecPriorities =
       buildInputSectionPriorities();
 
   uint32_t sectionIndex = 0;
   for (OutputSegment *seg : outputSegments) {
-    seg->sortOutputSections(compareByOrder<OutputSection *>(sectionOrder));
+    seg->sortOutputSections();
     for (OutputSection *osec : seg->getSections()) {
       // Now that the output sections are sorted, assign the final
       // output section indices.
       if (!osec->isHidden())
         osec->index = ++sectionIndex;
       if (!firstTLVDataSection && isThreadLocalData(osec->flags))
         firstTLVDataSection = osec;