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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

LGTM with the comments addressed. @ruiu, what do you think?



================
Comment at: lld/MachO/OutputSegment.cpp:40
+  if (vec.empty() && !isec->isHidden()) {
+    ++numNonHiddenSections;
+  }
----------------
int3 wrote:
> smeenai wrote:
> > Hmm, what's up with the `vec.empty()` condition? Doesn't that mean `numNonHiddenSections` will only ever be 0 or 1, depending on if the first section `addSection` was called on for a particular segment was hidden or not?
> This is to handle merged sections (well, aside from the fact that the rest of the code doesn't really do that yet). `vec.empty()` checks to see if we're creating a new output section or adding to an existing one.
Got it. Flagging this for @Ktwu to adjust as part of the section merging, if she isn't already.


================
Comment at: lld/MachO/SyntheticSections.cpp:85
+
+bool BindingSection::isNeeded() const { return in.got->isNeeded(); }
+
----------------
Any reason to have this one-liner not be part of the header?


================
Comment at: lld/MachO/SyntheticSections.h:97
+// Stores the strings referenced by the symbol table.
+class StringPoolSection : public InputSection {
+public:
----------------
I'd prefer StringTable over StringPool, since string table is the standard terminology used in the Mach-O reference.


================
Comment at: lld/MachO/Writer.cpp:235
+    // list for their given segment.
+    std::vector<std::pair<StringRef, std::vector<StringRef>>> ordering{
+        {segment_names::pageZero, {}},
----------------
Hmm, I'm not sure if I found this or the if-ladder clearer. On the one hand, this is more declarative, but on the other, you have to parse the code below to understand what this is doing. It should be easy enough to change in a follow-up if we want though, so I don't care too much right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76839