[PATCH] D86805: [lld-macho] create __TEXT,__unwind_info from __LD,__compact_unwind

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.


================
Comment at: lld/MachO/OutputSegment.cpp:73-74
 
-  outputSegments.push_back(segRef);
+  if (name != segment_names::ld) // TODO(gkm): only when no -r
+    outputSegments.push_back(segRef);
   return segRef;
----------------
Instead of filtering it out here, how about checking for `__LD,__compact_unwind` inside `createOutputSections()` while looping over `mergedOutputSections`, and then doing something like `unwindInfoSection->setCompactUnwind(...)`? Benefits: no need for the loop in `UnwindInfoSection::isNeeded()`, no need for `getOutputSection`, and no need to create an outputSegment to be discarded later.

That said, the current setup is definitely much cleaner than what we had before, and I'm happy to have this refinement in a follow-up diff


================
Comment at: lld/MachO/UnwindInfo.cpp:201
+
+// All inputs are relocated and output adddress known, so write!
+
----------------
s/adddress/addresses are/


================
Comment at: lld/MachO/UnwindInfo.cpp:288
+  }
+  assert(getSize() == (size_t)(reinterpret_cast<uint8_t *>(p2p) - buf));
+}
----------------
nit: `static_cast<size_t>`


================
Comment at: lld/MachO/UnwindInfo.h:14-17
+#include "llvm/ADT/DenseMap.h"
+#include <vector>
+
+#include "mach-o/compact_unwind_encoding.h"
----------------
nit: group includes with LLD headers first, then LLVM headers, then system headers


================
Comment at: lld/MachO/Writer.cpp:430-431
+        .Case(section_names::header, -1)
+        .Case(section_names::unwindInfo, 100)
+        .Case(section_names::ehFrame, 101)
+        .Default(0);
----------------
how about using `INT_MAX - 1` here instead, to cover the unlikely case where we have more than 100 sections? ld64 seems to do that


================
Comment at: lld/MachO/Writer.cpp:638
   in.imageLoaderCache = make<ImageLoaderCacheSection>();
+  in.unwindInfo = make<UnwindInfoSection>();
 }
----------------
nit: Synthetic sections that only need to be used within `Writer` -- like `SymtabSection` -- can be a member of `Writer` instead of `InStruct`.


================
Comment at: lld/test/MachO/compact-unwind.test:17
+
+# RUN: %python %S/tools/generate-cfi-funcs.py --seed=facebook >%t.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 -o %t.o %t.s
----------------
maybe change this seed to something that's not company-specific


================
Comment at: lld/test/MachO/tools/validate-unwind-info.py:36
+    for symbol, encoding, _, _ in object_encodings_list}
+  if (not object_encodings_map):
+    sys.exit("no object encodings found in input")
----------------
JFYI, the parens are unnecessary, but I'm fine if you prefer them


================
Comment at: lld/test/MachO/tools/validate-unwind-info.py:68
+      del object_encodings_map[symbol]
+    if args.debug and False:
+      print(f"{'delete' if fold else 'retain'} {symbol} with {encoding}")
----------------
I don't suppose you meant to leave `and False` in?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86805