[PATCH] D123279: [lld-macho][nfc] Give non-text ConcatOutputSections order-independent finalization

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo accepted this revision.
oontvoo added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/ConcatOutputSection.cpp:199-209
+  auto finalizeOne = [&](ConcatInputSection *isec) {
+    isecAddr = alignTo(isecAddr, isec->align);
+    isecFileOff = alignTo(isecFileOff, isec->align);
+    isec->outSecOff = isecAddr;
+    isec->isFinal = true;
+    isecAddr += isec->getSize();
+    isecFileOff += isec->getFileSize();
----------------
Why is the lambda needed here? (It's only got one caller ...)

Why not make it the body of the for loop?


================
Comment at: lld/MachO/ConcatOutputSection.cpp:428-434
+  if (!osec) {
+    if (isec->getSegName() != segment_names::text ||
+        isec->getName() == section_names::gccExceptTab ||
+        isec->getName() == section_names::ehFrame)
+      osec = make<ConcatOutputSection>(names.second);
+    else
+      osec = make<TextOutputSection>(names.second);
----------------
nit: it seems a bit more intuitive to reverse the condition here. ie., "if segment == text", then make text-output-section, otherwise, concat-output-section.



================
Comment at: lld/MachO/ConcatOutputSection.h:65-66
+  explicit TextOutputSection(StringRef name) : ConcatOutputSection(name) {}
+  void finalizeContents() override {}
+  void finalize() override;
+  bool needsThunks() const;
----------------
could we have some comments here on finalizae() vs finalizeContents() pls?


================
Comment at: lld/MachO/Writer.cpp:976
+
+  // We could parallelize this loop, but it seems cheaper to do it all in the
+  // main thread.
----------------
less memory? 
(citations needed 😄 )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123279