[lld-macho] Refactor segment/section creation, sorting, and merging

[int3]

Summary

There were a few issues with the previous setup:

1. The section sorting comparator used a declarative map of section names to determine the correct order, but it turns out we need to match on more than just names -- in particular, an upcoming diff will sort based on whether the S_ZERO_FILL flag is set. This diff changes the sorter to a more imperative but flexible form.

2. We were sorting OutputSections stored in a MapVector, which left the MapVector in an inconsistent state -- the wrong keys map to the wrong values! In practice, we weren't doing key lookups (only container iteration) after the sort, so this was fine, but it was still a dubious state of affairs. This diff copies the OutputSections to a vector before sorting them.

3. We were adding unneeded OutputSections to OutputSegments and then filtering them out later, which meant that we had to remember whether an OutputSegment was in a pre- or post-filtered state. This diff only adds the sections to the segments if they are needed.

In addition to those major changes, two minor ones worth noting:

1. I renamed all OutputSection variable names to osec, to parallel isec. Previously we were using some inconsistent combination of osec, os, and section.

2. I added a check (and a test) for InputSections with names that clashed with those of our synthetic OutputSections.

[int3]

format commit msg

[int3]

update

[compnerd]

Overall, I'm liking this change. I feel like its simpler.

lld/MachO/OutputSegment.cpp

41–42

Nit: section instead of osec would be nicer IMO.

47–49

Similar

lld/MachO/Writer.cpp

137–138

Similar

347

I think that this might be easier to read as a llvm::StringSwitch.

364

Why does the section ordering check the segment name? This deserves a comment at least.

382

s/osec/section/?

438

Minor micro-opt? swap the condition as we expect the conflict to be rare, and that makes it easier to follow too IMO.

454–459

s/osec/section/?

[int3]

lld/MachO/OutputSegment.cpp

41–42

in addition to paralleling isec, it's also the naming convention used by lld-ELF...

lld/MachO/Writer.cpp

347

TIL about StringSwitch, thanks

[int3]

update

[Ktwu]

This is nice; thanks for the cleanup!

lld/MachO/Writer.cpp

337

Why move the ordering functions back into the writer?

355

Why use std::numeric_limits instead of constants like -3, -2, -1, 0? Magic numbers suck but std::numeric_limits<int>::min() seems sort of verbose.

Nit: cache std::numeric_limits<int>::min() in a temp variable?

[MaskRay]

LGTM.

lld/MachO/Writer.cpp

454–459

osec looks fine. I appreciate that in some places of ELF/ we use osec to emphasize that it is an OutputSection, not an InputSection.

[int3]

lld/MachO/OutputSegment.h

20–24

for future reference: the change from a pointer to an array was required so that these constants could be used in StringSwitch.

lld/MachO/Writer.cpp

337

I thought it was nice for all the sorting code to live together, so I brought the ordering functions next to sortSegmentsAndSections. Also while looking up the ordering functions I always forgot if they lived in OutputSegment.cpp or OutputSection.cpp...

I guess another way to look at it is an OO vs functional style of code organization -- I prefer the latter, especially since lld is mostly performing complex operations on relatively simple objects

355

fair enough, I switched to -3, .. 0 instead

lld/MachO/MergedOutputSection.h

 //===- OutputSection.h ------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_MACHO_MERGED_OUTPUT_SECTION_H
 #define LLD_MACHO_MERGED_OUTPUT_SECTION_H
 
 #include "InputSection.h"
 #include "OutputSection.h"
 #include "lld/Common/LLVM.h"
+#include "llvm/ADT/MapVector.h"
 
 namespace lld {
 namespace macho {
 
 // Linking multiple files will inevitably mean resolving sections in different
 // files that are labeled with the same segment and section name. This class
 // contains all such sections and writes the data from each section sequentially
 // in the final binary.
 class MergedOutputSection : public OutputSection {
 public:
   MergedOutputSection(StringRef name) : OutputSection(MergedKind, name) {}
 
   const InputSection *firstSection() const { return inputs.front(); }
   const InputSection *lastSection() const { return inputs.back(); }
 
   // These accessors will only be valid after finalizing the section
   uint64_t getSize() const override { return size; }
   uint64_t getFileSize() const override { return fileSize; }
 
-  void mergeInput(InputSection *input) override;
+  void mergeInput(InputSection *input);
   void finalize() override;
 
   void writeTo(uint8_t *buf) const override;
 
   std::vector<InputSection *> inputs;
 
   static bool classof(const OutputSection *sec) {
     return sec->kind() == MergedKind;

lld/MachO/OutputSection.h

public:
   // as-is so their file size is the same as their address space size.
   virtual uint64_t getFileSize() const { return getSize(); }
 
   // Hidden sections omit header content, but body content may still be present.
   virtual bool isHidden() const { return false; }
   // Unneeded sections are omitted entirely (header and body).
   virtual bool isNeeded() const { return true; }
 
-  // Some sections may allow coalescing other raw input sections.
-  virtual void mergeInput(InputSection *input);
-
   // Specifically finalizes addresses and section size, not content.
   virtual void finalize() {
     // TODO investigate refactoring synthetic section finalization logic into
     // overrides of this function.
   }
 
   virtual void writeTo(uint8_t *buf) const = 0;
 
   StringRef name;
   OutputSegment *parent = nullptr;
 
   uint32_t index = 0;
   uint64_t addr = 0;
   uint64_t fileOff = 0;
   uint32_t align = 1;
   uint32_t flags = 0;
 
 private:
   Kind sectionKind;
 };
 
-class OutputSectionComparator {
-public:
-  OutputSectionComparator(uint32_t segmentOrder,
-                          const std::vector<StringRef> &sectOrdering)
-      : segmentOrder(segmentOrder) {
-    for (uint32_t j = 0, m = sectOrdering.size(); j < m; ++j)
-      sectionOrdering[sectOrdering[j]] = j;
-  }
-
-  uint32_t sectionOrder(StringRef secname) {
-    auto sectIt = sectionOrdering.find(secname);
-    if (sectIt != sectionOrdering.end())
-      return sectIt->second;
-    return sectionOrdering.size();
-  }
-
-  // Sort sections within a common segment, which stores them in
-  // a MapVector of section name -> section
-  bool operator()(const std::pair<StringRef, OutputSection *> &a,
-                  const std::pair<StringRef, OutputSection *> &b) {
-    return sectionOrder(a.first) < sectionOrder(b.first);
-  }
-
-  bool operator<(const OutputSectionComparator &b) {
-    return segmentOrder < b.segmentOrder;
-  }
-
-private:
-  uint32_t segmentOrder;
-  llvm::DenseMap<StringRef, uint32_t> sectionOrdering;
-};
-
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/OutputSection.cpp

 //===- OutputSection.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 "OutputSection.h"
 #include "OutputSegment.h"
-#include "lld/Common/ErrorHandler.h"
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::macho;
 
 uint64_t OutputSection::getSegmentOffset() const {
   return addr - parent->firstSection()->addr;
 }
-
-void OutputSection::mergeInput(InputSection *input) {
-  llvm_unreachable("Cannot merge input section into unmergable output section");
-}

lld/MachO/OutputSegment.h

 //===- OutputSegment.h ------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_MACHO_OUTPUT_SEGMENT_H
 #define LLD_MACHO_OUTPUT_SEGMENT_H
 
 #include "OutputSection.h"
 #include "lld/Common/LLVM.h"
-#include "llvm/ADT/MapVector.h"
 
 namespace lld {
 namespace macho {
 
 namespace segment_names {
 
-constexpr const char *pageZero = "__PAGEZERO";
-constexpr const char *text = "__TEXT";
-constexpr const char *data = "__DATA";
-constexpr const char *linkEdit = "__LINKEDIT";
-constexpr const char *dataConst = "__DATA_CONST";
+constexpr const char pageZero[] = "__PAGEZERO";
+constexpr const char text[] = "__TEXT";
+constexpr const char data[] = "__DATA";
+constexpr const char linkEdit[] = "__LINKEDIT";
+constexpr const char dataConst[] = "__DATA_CONST";
 
 } // namespace segment_names
 
 class OutputSection;
-class OutputSegmentComparator;
 class InputSection;
 
 class OutputSegment {
 public:
-  using SectionMap = typename llvm::MapVector<StringRef, OutputSection *>;
-  using SectionMapEntry = typename std::pair<StringRef, OutputSection *>;
+  const OutputSection *firstSection() const { return sections.front(); }
+  const OutputSection *lastSection() const { return sections.back(); }
 
-  const OutputSection *firstSection() const { return sections.front().second; }
-  const OutputSection *lastSection() const { return sections.back().second; }
-
-  bool isNeeded() const {
-    if (name == segment_names::linkEdit)
-      return true;
-    for (const SectionMapEntry &i : sections) {
-      OutputSection *os = i.second;
-      if (os->isNeeded())
-        return true;
-    }
-    return false;
-  }
-
-  OutputSection *getOrCreateOutputSection(StringRef name);
   void addOutputSection(OutputSection *os);
-  void sortOutputSections(OutputSegmentComparator *comparator);
-  void removeUnneededSections();
+  void sortOutputSections(
+      llvm::function_ref<bool(OutputSection *, OutputSection *)> comparator) {
+    llvm::stable_sort(sections, comparator);
+  }
 
-  const SectionMap &getSections() const { return sections; }
+  const std::vector<OutputSection *> &getSections() const { return sections; }
   size_t numNonHiddenSections() const;
 
   uint64_t fileOff = 0;
   StringRef name;
   uint32_t maxProt = 0;
   uint32_t initProt = 0;
   uint8_t index;
 
 private:
-  SectionMap sections;
-};
-
-class OutputSegmentComparator {
-public:
-  OutputSegmentComparator();
-
-  OutputSectionComparator *sectionComparator(const OutputSegment *os) {
-    auto it = orderMap.find(os->name);
-    if (it == orderMap.end()) {
-      return defaultPositionComparator;
-    }
-    return &it->second;
-  }
-
-  bool operator()(const OutputSegment *a, const OutputSegment *b) {
-    return *sectionComparator(a) < *sectionComparator(b);
-  }
-
-private:
-  const StringRef defaultPosition = StringRef();
-  llvm::DenseMap<StringRef, OutputSectionComparator> orderMap;
-  OutputSectionComparator *defaultPositionComparator;
+  std::vector<OutputSection *> sections;
 };
 
 extern std::vector<OutputSegment *> outputSegments;
 
-OutputSegment *getOutputSegment(StringRef name);
 OutputSegment *getOrCreateOutputSegment(StringRef name);
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/OutputSegment.cpp

 static uint32_t maxProt(StringRef name) {
   if (name == segment_names::pageZero)
     return 0;
   return VM_PROT_READ | VM_PROT_WRITE | VM_PROT_EXECUTE;
 }
 
 size_t OutputSegment::numNonHiddenSections() const {
   size_t count = 0;
-  for (const OutputSegment::SectionMapEntry &i : sections) {
-    OutputSection *os = i.second;
-    count += (!os->isHidden() ? 1 : 0);
+  for (const OutputSection *osec : sections) {
+    count += (!osec->isHidden() ? 1 : 0);
   }
   return count;
 }
 
-void OutputSegment::addOutputSection(OutputSection *os) {
-  os->parent = this;
-  std::pair<SectionMap::iterator, bool> result =
-      sections.insert(SectionMapEntry(os->name, os));
-  if (!result.second) {
-    llvm_unreachable("Attempted to set section, but a section with the same "
-                     "name already exists");
-  }
-}
-
-OutputSection *OutputSegment::getOrCreateOutputSection(StringRef name) {
-  OutputSegment::SectionMap::iterator i = sections.find(name);
-  if (i != sections.end()) {
-    return i->second;
-  }
-
-  auto *os = make<MergedOutputSection>(name);
-  addOutputSection(os);
-  return os;
-}
-
-void OutputSegment::sortOutputSections(OutputSegmentComparator *comparator) {
-  llvm::stable_sort(sections, *comparator->sectionComparator(this));
-}
-
-void OutputSegment::removeUnneededSections() {
-  sections.remove_if([](const std::pair<StringRef, OutputSection *> &p) {
-    return !p.second->isNeeded();
-  });
-}
-
-OutputSegmentComparator::OutputSegmentComparator() {
-  // This defines the order of segments and the sections within each segment.
-  // Segments that are not mentioned here will end up at defaultPosition;
-  // sections that are not mentioned will end up at the end of the section
-  // list for their given segment.
-  std::vector<std::pair<StringRef, std::vector<StringRef>>> ordering{
-      {segment_names::pageZero, {}},
-      {segment_names::text, {section_names::header}},
-      {defaultPosition, {}},
-      // Make sure __LINKEDIT is the last segment (i.e. all its hidden
-      // sections must be ordered after other sections).
-      {segment_names::linkEdit,
-       {
-           section_names::binding,
-           section_names::export_,
-           section_names::symbolTable,
-           section_names::stringTable,
-       }},
-  };
-
-  for (uint32_t i = 0, n = ordering.size(); i < n; ++i) {
-    auto &p = ordering[i];
-    StringRef segname = p.first;
-    const std::vector<StringRef> &sectOrdering = p.second;
-    orderMap.insert(std::pair<StringRef, OutputSectionComparator>(
-        segname, OutputSectionComparator(i, sectOrdering)));
-  }
-
-  // Cache the position for the default comparator since this is the likely
-  // scenario.
-  defaultPositionComparator = &orderMap.find(defaultPosition)->second;
+void OutputSegment::addOutputSection(OutputSection *osec) {
+  osec->parent = this;
+  sections.push_back(osec);
 }
 
 static llvm::DenseMap<StringRef, OutputSegment *> nameToOutputSegment;
 std::vector<OutputSegment *> macho::outputSegments;
 
-OutputSegment *macho::getOutputSegment(StringRef name) {
-  return nameToOutputSegment.lookup(name);
-}
-
 OutputSegment *macho::getOrCreateOutputSegment(StringRef name) {
   OutputSegment *&segRef = nameToOutputSegment[name];
   if (segRef != nullptr)
     return segRef;
 
   segRef = make<OutputSegment>();
   segRef->name = name;
   segRef->maxProt = maxProt(name);
   segRef->initProt = initProt(name);
 
   outputSegments.push_back(segRef);
   return segRef;
 }

lld/MachO/SyntheticSections.h

 #include "llvm/ADT/SetVector.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace lld {
 namespace macho {
 
 namespace section_names {
 
-constexpr const char *pageZero = "__pagezero";
-constexpr const char *header = "__mach_header";
-constexpr const char *binding = "__binding";
-constexpr const char *lazyBinding = "__lazy_binding";
-constexpr const char *export_ = "__export";
-constexpr const char *symbolTable = "__symbol_table";
-constexpr const char *stringTable = "__string_table";
-constexpr const char *got = "__got";
+constexpr const char pageZero[] = "__pagezero";
+constexpr const char header[] = "__mach_header";
+constexpr const char binding[] = "__binding";
+constexpr const char lazyBinding[] = "__lazy_binding";
+constexpr const char export_[] = "__export";
+constexpr const char symbolTable[] = "__symbol_table";
+constexpr const char stringTable[] = "__string_table";
+constexpr const char got[] = "__got";
 
 } // namespace section_names
 
 class DylibSymbol;
 class LoadCommand;
 
 class SyntheticSection : public OutputSection {
 public:
   SyntheticSection(const char *segname, const char *name);
   virtual ~SyntheticSection() = default;
 
   static bool classof(const OutputSection *sec) {
     return sec->kind() == SyntheticKind;
   }
+
+  const StringRef segname;
 };
 
 // The header of the Mach-O file, which must have a file offset of zero.
 class MachHeaderSection : public SyntheticSection {
 public:
   MachHeaderSection();
   void addLoadCommand(LoadCommand *);
   bool isHidden() const override { return true; }
struct InStruct {
   GotSection *got = nullptr;
   LazyPointerSection *lazyPointers = nullptr;
   StubsSection *stubs = nullptr;
   StubHelperSection *stubHelper = nullptr;
   ImageLoaderCacheSection *imageLoaderCache = nullptr;
 };
 
 extern InStruct in;
+extern std::vector<SyntheticSection *> syntheticSections;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/SyntheticSections.cpp

 using namespace llvm::MachO;
 using namespace llvm::support;
 using namespace llvm::support::endian;
 
 namespace lld {
 namespace macho {
 
 SyntheticSection::SyntheticSection(const char *segname, const char *name)
-    : OutputSection(SyntheticKind, name) {
-  // Synthetic sections always know which segment they belong to so hook
-  // them up when they're made
-  getOrCreateOutputSegment(segname)->addOutputSection(this);
+    : OutputSection(SyntheticKind, name), segname(segname) {
+  syntheticSections.push_back(this);
 }
 
 // dyld3's MachOLoaded::getSlide() assumes that the __TEXT segment starts
 // from the beginning of the file (i.e. the header).
 MachHeaderSection::MachHeaderSection()
     : SyntheticSection(segment_names::text, section_names::header) {}
 
 void MachHeaderSection::addLoadCommand(LoadCommand *lc) {
void StringTableSection::writeTo(uint8_t *buf) const {
   uint32_t off = 0;
   for (StringRef str : strings) {
     memcpy(buf + off, str.data(), str.size());
     off += str.size() + 1; // account for null terminator
   }
 }
 
 InStruct in;
+std::vector<SyntheticSection *> syntheticSections;
 
 } // namespace macho
 } // namespace lld

lld/MachO/Writer.cpp

void writeTo(uint8_t *buf) const override {
     if (seg->getSections().empty())
       return;
 
     c->vmaddr = seg->firstSection()->addr;
     c->vmsize =
         seg->lastSection()->addr + seg->lastSection()->getSize() - c->vmaddr;
     c->nsects = seg->numNonHiddenSections();
 
-    for (auto &p : seg->getSections()) {
-      StringRef s = p.first;
-      OutputSection *section = p.second;
-      c->filesize += section->getFileSize();
+    for (OutputSection *osec : seg->getSections()) {
+      c->filesize += osec->getFileSize();
 
-      if (section->isHidden())
+      if (osec->isHidden())
         continue;
 
       auto *sectHdr = reinterpret_cast<section_64 *>(buf);
       buf += sizeof(section_64);
 
-      memcpy(sectHdr->sectname, s.data(), s.size());
+      memcpy(sectHdr->sectname, osec->name.data(), osec->name.size());
       memcpy(sectHdr->segname, name.data(), name.size());
 
-      sectHdr->addr = section->addr;
-      sectHdr->offset = section->fileOff;
-      sectHdr->align = Log2_32(section->align);
-      sectHdr->flags = section->flags;
-      sectHdr->size = section->getSize();
+      sectHdr->addr = osec->addr;
+      sectHdr->offset = osec->fileOff;
+      sectHdr->align = Log2_32(osec->align);
+      sectHdr->flags = osec->flags;
+      sectHdr->size = osec->getSize();
     }
   }
 
 private:
   StringRef name;
   OutputSegment *seg;
 };
 
for (InputFile *file : inputFiles)
     if (isa<ObjFile>(file) || isa<ArchiveFile>(file))
       for (lld::macho::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, -2)
+      .Case(segment_names::text, -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(0);
+}
+
+static int sectionOrder(OutputSection *osec) {
+  StringRef segname = osec->parent->name;
+  // Sections are uniquely identified by their segment + section name.
+  if (segname == segment_names::text) {
+    if (osec->name == section_names::header)
+      return -1;
+  } else if (segname == segment_names::linkEdit) {
+    return StringSwitch<int>(osec->name)
+        .Case(section_names::binding, -4)
+        .Case(section_names::export_, -3)
+        .Case(section_names::symbolTable, -2)
+        .Case(section_names::stringTable, -1)
+        .Default(0);
+  }
+  return 0;
+}
+
+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() {
-  auto comparator = OutputSegmentComparator();
-  llvm::stable_sort(outputSegments, comparator);
+  llvm::stable_sort(outputSegments,
+                    compareByOrder<OutputSegment *>(segmentOrder));
 
   DenseMap<const InputSection *, size_t> isecPriorities =
       buildInputSectionPriorities();
 
   uint32_t sectionIndex = 0;
   for (OutputSegment *seg : outputSegments) {
-    seg->sortOutputSections(&comparator);
-    for (auto &p : seg->getSections()) {
-      OutputSection *section = p.second;
+    seg->sortOutputSections(compareByOrder<OutputSection *>(sectionOrder));
+    for (auto *osec : seg->getSections()) {
       // Now that the output sections are sorted, assign the final
       // output section indices.
-      if (!section->isHidden())
-        section->index = ++sectionIndex;
+      if (!osec->isHidden())
+        osec->index = ++sectionIndex;
 
       if (!isecPriorities.empty()) {
-        if (auto *merged = dyn_cast<MergedOutputSection>(section)) {
+        if (auto *merged = dyn_cast<MergedOutputSection>(osec)) {
           llvm::stable_sort(merged->inputs,
                             [&](InputSection *a, InputSection *b) {
                               return isecPriorities[a] > isecPriorities[b];
                             });
         }
       }
     }
   }
case MH_EXECUTE:
     make<PageZeroSection>();
     break;
   case MH_DYLIB:
     break;
   default:
     llvm_unreachable("unhandled output file type");
   }
 
-  // Then merge input sections into output sections/segments.
+  // Then merge input sections into output sections.
+  MapVector<std::pair<StringRef, StringRef>, MergedOutputSection *>
+      mergedOutputSections;
   for (InputSection *isec : inputSections) {
-    getOrCreateOutputSegment(isec->segname)
-        ->getOrCreateOutputSection(isec->name)
-        ->mergeInput(isec);
+    MergedOutputSection *&osec =
+        mergedOutputSections[{isec->segname, isec->name}];
+    if (osec == nullptr)
+      osec = make<MergedOutputSection>(isec->name);
+    osec->mergeInput(isec);
+  }
+
+  for (const auto &it : mergedOutputSections) {
+    StringRef segname = it.first.first;
+    MergedOutputSection *osec = it.second;
+    getOrCreateOutputSegment(segname)->addOutputSection(osec);
+  }
+
+  for (SyntheticSection *ssec : syntheticSections) {
+    auto it = mergedOutputSections.find({ssec->segname, ssec->name});
+    if (it == mergedOutputSections.end()) {
+      if (ssec->isNeeded())
+        getOrCreateOutputSegment(ssec->segname)->addOutputSection(ssec);
+    } else {
+      error("section from " + it->second->firstSection()->file->getName() +
+            " conflicts with synthetic section " + ssec->segname + "," +
+            ssec->name);
     }
-
-  // Remove unneeded segments and sections.
-  // TODO: Avoid creating unneeded segments in the first place
-  for (auto it = outputSegments.begin(); it != outputSegments.end();) {
-    OutputSegment *seg = *it;
-    seg->removeUnneededSections();
-    if (!seg->isNeeded())
-      it = outputSegments.erase(it);
-    else
-      ++it;
   }
 }
 
 void Writer::assignAddresses(OutputSegment *seg) {
   addr = alignTo(addr, PageSize);
   fileOff = alignTo(fileOff, PageSize);
   seg->fileOff = fileOff;
 
-  for (auto &p : seg->getSections()) {
-    OutputSection *section = p.second;
-    addr = alignTo(addr, section->align);
-    fileOff = alignTo(fileOff, section->align);
-    section->addr = addr;
-    section->fileOff = isZeroFill(section->flags) ? 0 : fileOff;
-    section->finalize();
+  for (auto *osec : seg->getSections()) {
+    addr = alignTo(addr, osec->align);
+    fileOff = alignTo(fileOff, osec->align);
+    osec->addr = addr;
+    osec->fileOff = isZeroFill(osec->flags) ? 0 : fileOff;
+    osec->finalize();
 
-    addr += section->getSize();
-    fileOff += section->getFileSize();
+    addr += osec->getSize();
+    fileOff += osec->getFileSize();
   }
 }
 
 void Writer::openFile() {
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(config->outputFile, fileOff,
                                FileOutputBuffer::F_executable);
 
   if (!bufferOrErr)
     error("failed to open " + config->outputFile + ": " +
           llvm::toString(bufferOrErr.takeError()));
   else
     buffer = std::move(*bufferOrErr);
 }
 
 void Writer::writeSections() {
   uint8_t *buf = buffer->getBufferStart();
-  for (OutputSegment *seg : outputSegments) {
-    for (auto &p : seg->getSections()) {
-      OutputSection *section = p.second;
-      section->writeTo(buf + section->fileOff);
-    }
-  }
+  for (OutputSegment *seg : outputSegments)
+    for (OutputSection *osec : seg->getSections())
+      osec->writeTo(buf + osec->fileOff);
 }
 
 void Writer::run() {
   // dyld requires __LINKEDIT segment to always exist (even if empty).
   OutputSegment *linkEditSegment =
       getOrCreateOutputSegment(segment_names::linkEdit);
 
   scanRelocations();

lld/test/MachO/invalid/reserved-section-name.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
+# RUN: not lld -flavor darwinnew -o %t %t.o 2>&1 | FileCheck %s -DFILE=%t.o
+# CHECK: error: section from [[FILE]] conflicts with synthetic section __DATA_CONST,__got
+
+.globl _main
+
+.section __DATA_CONST,__got
+.space 1
+
+.text
+_main:
+  mov $0, %rax
+  ret