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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

just a partial review for now... will come back and do a deeper reading next week after I understand a bit more about the CU format.



================
Comment at: lld/MachO/OutputSegment.cpp:54
+  for (const OutputSection *osec : sections) {
+    if (name.equals(osec->name))
+      return osec;
----------------
nit: StringRefs can be compared using `==`


================
Comment at: lld/MachO/UnwindInfo.cpp:1
+#include "UnwindInfo.h"
+#include "Config.h"
----------------
missing license header


================
Comment at: lld/MachO/UnwindInfo.cpp:58
+
+bool UnwindInfoSection::isHidden() const {
+  if (checkedCompactUnwindInputs)
----------------
I think this should be `isNeeded()`, not `isHidden()` -- "hidden" indicates that the section header is absent but that the section data itself should still be emitted


================
Comment at: lld/MachO/UnwindInfo.cpp:93
+    for (const Reloc &r : isec->relocs) {
+      if (auto *risec = r.target.dyn_cast<InputSection *>()) {
+        if (risec->getVA() == 0) {
----------------
nit: `targetIsec` would be clearer


================
Comment at: lld/MachO/UnwindInfo.cpp:139
+              if (a.second == b.second)
+                // When freqnecies match, secondarily sort on encoding
+                // to maintain parity with validate-unwind-info.py
----------------
frequencies


================
Comment at: lld/MachO/UnwindInfo.h:51-66
+  mutable std::map<uint32_t, size_t> commonEncodingIndexes;
+  mutable std::vector<std::pair<compact_unwind_encoding_t, size_t>>
+      commonEncodings;
+  mutable std::vector<uint32_t> personalities;
+  mutable std::vector<unwind_info_section_header_lsda_entry> lsdaEntries;
+  mutable std::vector<compact_unwind_entry> cuVector;
+  mutable std::vector<compact_unwind_entry *> cuPtrVector;
----------------
why the need to mark everything as mutable?


================
Comment at: lld/MachO/Writer.cpp:413-414
+      .Case(segment_names::linkEdit, 100)
+      // __LD,__compact_unwind isn't an output segment, so shove it past the end
+      .Case(segment_names::ld, 101)
       .Default(0);
----------------
This seems a bit hacky. Do we really have to put the `__compact_unwind` sections in an OutputSegment? If we're not going to output them, could we just put all these InputSections in a vector contained in `UnwindInfoSection`? 

Also, we may want to support emitting object files at some point via `-r`, in which case we'll actually want to emit the `__LD` segment at a valid position.


================
Comment at: lld/test/MachO/tools/validate-unwind-info.py:89
+
+  return sys.exit()
+
----------------
this seems unnecessary given that we'll be exiting anyway when this function returns


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86805