[PATCH] D76839: [lld-macho] Extend SyntheticSections to cover all segment load commands

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/MachO/OutputSegment.cpp:20
 
+namespace {
+
----------------
I believe the LLVM coding style recommends making functions private using `static` instead of surrounding a function definition with an anonymous namespace.


================
Comment at: lld/MachO/OutputSegment.cpp:40
+
+void macho::OutputSegment::addSection(InputSection *isec) {
+  isec->parent = this;
----------------
Since you have `using namespace lld::macho`, I guess you can omit `macho::`?


================
Comment at: lld/MachO/OutputSegment.cpp:43
+  std::vector<InputSection *> &vec = sections[isec->name];
+  if (vec.size() == 0 && !isec->isHidden()) {
+    ++numNonHiddenSections;
----------------
!empty()


================
Comment at: lld/MachO/OutputSegment.cpp:61
 
   auto *os = make<OutputSegment>();
   os->name = name;
----------------
Looks like you can directly assign to `segRef`.


================
Comment at: lld/MachO/OutputSegment.h:35
+  bool isNeeded() const {
+    return sections.size() != 0 || name == segment_names::linkEdit;
+  }
----------------
nit: `!sections.empty()` is more conventional.


================
Comment at: lld/MachO/SyntheticSections.cpp:157-159
+  uint32_t strx = contents.size();
+  os << str << '\0';
+  return strx;
----------------
This function makes a copy of a string which is then copied to the output buffer, which isn't very efficient. You can avoid the extra string copy by eliminating the output stream as well as SmallVector, and instead storing strings as `std::vector<StringRef>`.


================
Comment at: lld/MachO/SyntheticSections.h:48-49
+
+// A hidden section that exists solely for the purpose of creating the
+// __PAGEZERO segment, which is used to catch null pointer dereferences.
+class PageZeroSection : public InputSection {
----------------
Thank you for the comment, that's very helpful.


================
Comment at: lld/MachO/SyntheticSections.h:71
 
+  bool isNeeded() const override { return entries.size() != 0; }
+
----------------
!empty()


================
Comment at: lld/MachO/Writer.cpp:114
+
+    if (seg->getSections().size() == 0)
+      return;
----------------
!empty()


================
Comment at: lld/MachO/Writer.cpp:122
 
-    for (auto &p : seg->sections) {
+    for (auto &p : seg->getSections()) {
       StringRef s = p.first;
----------------
Use a concrete type instead of `auto`.


================
Comment at: lld/MachO/Writer.cpp:124
       StringRef s = p.first;
-      std::vector<InputSection *> &sections = p.second;
+      const std::vector<InputSection *> &sections = p.second;
+      for (auto *isec : sections)
----------------
We usually use `ArrayRef<T>` instead of `const std::vector<T>`.


================
Comment at: lld/MachO/Writer.cpp:125
+      const std::vector<InputSection *> &sections = p.second;
+      for (auto *isec : sections)
+        c->filesize += isec->getFileSize();
----------------
Expand auto.


================
Comment at: lld/MachO/Writer.cpp:236
+
+static void sortByOrder(MutableArrayRef<InputSection *> in,
+                        llvm::function_ref<uint32_t(InputSection *s)> order) {
----------------
Can you add a function comment?


================
Comment at: lld/MachO/Writer.cpp:237
+static void sortByOrder(MutableArrayRef<InputSection *> in,
+                        llvm::function_ref<uint32_t(InputSection *s)> order) {
+  std::vector<std::pair<uint32_t, InputSection *>> v;
----------------
You are always passing `sectionOrder` function as the second argument, so it looks like you can eliminate the second parameter and always using `sectionOrder`.


================
Comment at: lld/MachO/Writer.cpp:256
+    return 0;
+  else if (isec->name == section_names::header)
+    return 1;
----------------
nit: omit `else` after `return`.


================
Comment at: lld/MachO/Writer.cpp:259-261
+    return linkEditOffset + 1;
+  else
+    return linkEditOffset;
----------------
So it looks like the function returns only 4 possible values -- 0, 1, linkEditOffset, or linkEditOffset+1. Can't you use 0, 1, 2, 3 instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76839