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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/MergedOutputSection.cpp:20
+void MergedOutputSection::mergeInput(InputSection *input) {
+  if (this->inputs.empty()) {
+    this->align = input->align;
----------------
bikeshed: Do we want to prefix all member accesses with `this->`? lld-ELF seems to use it inconsistently, lld-COFF in just a few places... I have a slight preference towards omitting it, but we should be consistent either way


================
Comment at: lld/MachO/OutputSection.cpp:25-28
+void OutputSection::mergeInput(InputSection *input) {
+  error("Section " + this->name + " in segment " + parent->name +
+        " cannot be merged");
+}
----------------
can we just define this method on MergedOutputSection?


================
Comment at: lld/MachO/OutputSection.h:20
+
+class OutputSection {
+public:
----------------
How does this class hierarchy compare to that in the ELF implementation? I know they have `SectionBase`, `InputSectionBase`, and `MergeInputSection`... was planning to dig into it eventually but curious if you have looked


================
Comment at: lld/MachO/OutputSegment.h:49
+  OutputSection *getOrCreateOutputSection(StringRef name);
+  void setOutputSection(OutputSection *os);
 
----------------
`addOutputSection` seems like a more fitting name


================
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;
----------------
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


================
Comment at: lld/MachO/OutputSegment.h:32
 public:
-  InputSection *firstSection() const { return sections.front().second.at(0); }
+  typedef llvm::MapVector<StringRef, OutputSection *> SectionMap;
 
----------------
nit: I think `using SectionMap = ...` is the more modern C++ way (and is the method favored by lld-ELF/COFF)


================
Comment at: lld/MachO/OutputSegment.h:40
+      return true;
+    for (auto i : sections) {
+      if (i.second->isNeeded()) {
----------------
@ruiu will object to the `auto` :) I personally hate typing out an `std::pair` type, but we should at least unpack `i.second` into a var with a named type

nit 2: use `auto &` here

alternatively we could replace the loop with `std::any_of`


================
Comment at: lld/MachO/SyntheticSections.h:43
+
+  const char *segname;
 
----------------
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?


================
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.
----------------
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?


================
Comment at: lld/MachO/Writer.cpp:486
   for (OutputSegment *seg : outputSegments)
-    assignAddresses(seg);
+    if (seg != linkEditSegment)
+      assignAddresses(seg);
----------------
Thanks, I think it's clearer this way


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893