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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

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



================
Comment at: lld/MachO/OutputSegment.cpp:41
   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);
----------------
Nit: `section` instead of `osec` would be nicer IMO.


================
Comment at: lld/MachO/OutputSegment.cpp:47
 
-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;
----------------
Similar


================
Comment at: lld/MachO/Writer.cpp:137
 
-    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();
----------------
Similar


================
Comment at: lld/MachO/Writer.cpp:347
+  }
+  return 0;
+}
----------------
I think that this might be easier to read as a `llvm::StringSwitch`.


================
Comment at: lld/MachO/Writer.cpp:364
+      return std::numeric_limits<int>::min() + 3;
+  }
+  return 0;
----------------
Why does the section ordering check the segment name?  This deserves a comment at least.


================
Comment at: lld/MachO/Writer.cpp:386
+    seg->sortOutputSections(compareByOrder<OutputSection *>(sectionOrder));
+    for (auto *osec : seg->getSections()) {
       // Now that the output sections are sorted, assign the final
----------------
s/osec/section/?


================
Comment at: lld/MachO/Writer.cpp:442
+    auto it = mergedOutputSections.find({ssec->segname, ssec->name});
+    if (it != mergedOutputSections.end()) {
+      error("section from " + it->second->firstSection()->file->getName() +
----------------
Minor micro-opt? swap the condition as we expect the conflict to be rare, and that makes it easier to follow too IMO.


================
Comment at: lld/MachO/Writer.cpp:458
 
-  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);
----------------
s/osec/section/?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81887/new/

https://reviews.llvm.org/D81887