[PATCH] D77893: [lld] Merge Mach-O input sections

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 7 inline comments as done.
Ktwu added inline comments.


================
Comment at: lld/MachO/OutputSegment.h:51
 
-  const llvm::MapVector<StringRef, std::vector<InputSection *>> &
-  getSections() const {
-    return sections;
-  }
+  const SectionMap &getSections() const { return sections; }
+  size_t numNonHiddenSections() const;
----------------
int3 wrote:
> if `sections` isn't private any more, it doesn't need an accessor
> 
> edit: I see that it's non-private only because we need to sort it. How about making the sorting a method on this class?
> 
> Related thought... I see that MapVector has a `takeVector` method that clears out the map and returns the underlying vector. Maybe we could do that -- have a `getSortedSections` method that returns an empty vector until we actually sort things. That would mean that the comparator can work on an actual vector & take single elements instead of `std::pair`s. Just my 2c, might be overcomplicating things here
Yes to section sorting living here, nay to the `takeVector` idea (at least in this diff).


================
Comment at: lld/MachO/SyntheticSections.h:43
+
+  const char *segname;
 
----------------
int3 wrote:
> should this field live in the parent class?
> 
> edit: Oh I see, OutputSections don't define `segname` because that's accessed through `parent->name`, so only InputSections and synthetic sections need to define segnames. How about defining a `SyntheticSection` superclass that defines this field in its ctor, and have every class in this file inherit from it?
Yup, a base SyntheticSection class sounds ++


================
Comment at: lld/MachO/Writer.cpp:307-315
+  // Sort sections within a common segment
+  bool operator()(const std::pair<StringRef, OutputSection *> a,
+                  const std::pair<StringRef, OutputSection *> b) {
+    return order(a.second) < order(b.second);
+  }
+
+  // Sort segments using a representative output section.
----------------
int3 wrote:
> Oh I see... looks like the problem with my initial sorting scheme is that it assumed that the segments were only ever created once all the sections were sorted. But this didn't account for __LINKEDIT, though I was lucky enough to create it after the sorting, so things worked out... but explicitly sorting both the segments and sections is more reliable. I think we can simplify the `order` method a bit though -- it's currently written to support comparing sections across different segments, and we're not using that functionality any more. Can we have two separate `order` methods, one for segments and the other for sections in the same segment?
I was thinking about that, too, so I'll try it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893