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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 8 inline comments as done.
gkm added inline comments.


================
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;
----------------
int3 wrote:
> why the need to mark everything as mutable?
The work of morphing `__LD,__compact_unwind` into `__TEXT,__unwind_info` happens in `uint64_t getSize() const` and `void writeTo(uint8_t *buf) const`, which can only be logically `const`, but not physically.


================
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);
----------------
int3 wrote:
> 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.
I agree that it could use some improvement. I will revisit.

Behaviorally, `__LD,__compact_unwind` is a `MergedOutputSegment`, except with different timing and destination: we relocate & write it to a temp buffer from `Writer::assignAddresses()`) rather than writing to the final link output from `Writer::writeSections()`. We need to relocate it fully in order to compute the size of the `__TEXT,__unwind_info` section when laying-out sections & binding to addresses.

You are quite right regarding `-r`, which makes `__LD,__compact_unwind` a completely normal `MergedOutputSegment`.


================
Comment at: lld/test/MachO/tools/validate-unwind-info.py:89
+
+  return sys.exit()
+
----------------
int3 wrote:
> this seems unnecessary given that we'll be exiting anyway when this function returns
True, but since I explicitly `sys.exit("... diagnostic ...")` on error, for symmetry I chose to explicitly `sys.exit()` on success. I dislike falling through the floor of main, despite language guarantees, and since `sys.exit()` accepts a string arg on error, I prefer that to using numeric `return STATUS`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86805