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

[Greg McGary via Phabricator via libcxx-commits]

gkm added inline comments.


================
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);
----------------
gkm wrote:
> int3 wrote:
> > gkm wrote:
> > > 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`.
> > I think it might be worthwhile to duplicate some of the code in `InputSection::writeTo` to specialize it for the compact unwind case. The current `writeTo` code assumes that both the source and target sections referenced by a relocation have their final output addresses determined. But this is not true for the compact unwind section -- it doesn't have a valid output address. It just happens to work because there are no pcrel relocations involved. We should really error out if we see a pcrel relocation while relocating `__compact_unwind`.
> > 
> > Creating a separate compact unwind relocating method would free us from having to create `Output{Segments, Sections}` that we discard later. I'm hoping we can retain the invariant where we add OutputSections to OutputSegments only after we have determined that they are needed.
> I still owe you an answer for this one ...
I believe this latest rev works well and isn't so ugly. I retain the normal flow for gathering input sections and attaching them to `MergedOutputSection` for `__LD`, but I exclude it from `outputSegments` (without `-r`) so further downstream processing doesn't happen. I do insert `__LD` into `nameToOutputSegment[]` so that the synthetic `UnwindInfoSection::finalize()` can find it for processing. That seems minimally invasive and doesn't need a subclass.

All evidence I have seen shows that reloc types in `__LD,__compact_unwind` are always absolute and never PC-relative. The only output address that must be determined is `lld`'s VMA. `OutputSection::writeTo()` accepts a pointer to a buffer, which is normally mapped to the linker's output file. However, when relocating `__compact_unwind`, the buffer is the internal `std::vector<CompactUnwindEntry64>::data()` rather than the output file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86805