[PATCH] D132489: [MachO] Fix dead-stripping __eh_frame

Fri Aug 26 07:42:16 PDT 2022

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

> That relocation is created by ehRelocator.commit(), which is skipped if the FDE is dead. I don't know if that was intentional or not.

Ahh gotcha. Yeah it was intentional insofar as I was trying to avoid doing unnecessary work for a dead FDE.

> Could you remind me why pruning unused FDEs is required for correctness? I thought that the unwinder used the compact unwind page tables as the source of truth for associating a function with its unwind info, so any redundant FDEs would merely waste space and not cause actual runtime issues.

Yeah I didn't check how the actual unwinder worked. IIRC some tool (one of the `dump` ones, or maybe ld64) would parse all EH frames in the section sequentially, so unused FDEs with pointers to invalid addresses would make it unhappy

================
Comment at: lld/test/MachO/eh-frame-dead-strip.s:9-10
+## Verify that unneeded FDEs (and their CIEs) are dead-stripped even if they
+## point to a live symbol. This requires arm64 because x86_64 doesn't emit
+## a relocation for the function symbol in an FDE.
+
----------------
smeenai wrote:
> int3 wrote:
> > I was going to ask, doesn't our `registerEhFrames()` method create those relocations anyway? But then I realized that it would create a relocation pointing to the canonical symbol, instead of the coalesced-away weak symbol. But in that case... aren't the coalesced-away weak symbols not marked as live?
> I didn't look into it, but on x86-64, before my change, if you linked with the order `strong.o weak.o`, you'd only get one copy of the CIE and FDE, and if you linked with the order `weak.o strong.o`, you'd get two copies of each (so the CIE wasn't incorrectly dead-stripped, at least, but the second CIE and FDE were both unnecessarily retained). After my change, both orders result in just a single copy. I should add a test for the other order of linking, and I can also add an x86-64 test if we want to check both scenarios.
ah gotcha. was just curious, don't think we really need to check both archs, but yeah checking both orders sounds like a good idea

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132489