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

Thu Aug 25 12:04:08 PDT 2022

smeenai added a comment.

In D132489#3744918 <https://reviews.llvm.org/D132489#3744918>, @thevinster wrote:

>> we would actually produce an invalid __eh_frame section in that case because the CIE associated with the unnecessary FDE would still get dead-stripped and we'd end up with a dangling FDE
>
> Thinking out loud here... instead of disabling `S_ATTR_LIVE_SUPPORT`, would it be better to prevent an invalid __eh_frame section by making sure the CIE is not dead-stripped even if an unnecessary FDE still references a live symbol? If we wanted to it to be "dead stripped", then enabling ICF should deduplicate the exceptions. My worry is that disabling it may cause unknown behaviors if a potential valid FDE gets dead stripped, even though it was not intended to. Is that something that can happen?

Like @int3 said, ICF is for a different case, where we have functions with different sources but identical machine code. This is for the case where we pick one copy of a definition and discard all the other weak ones, and we want to discard their FDEs as well.

`S_ATTR_LIVE_SUPPORT` to me conceptually means "this section has important metadata about this other section, but that other section has no link back to that metadata, so you have to manually check if the other section is alive and retain the metadata in that case". When we parse `__eh_frame`, we're explicitly creating the link from the function to its FDE (and from the FDE to its CIE), so we no longer need `S_ATTR_LIVE_SUPPORT`, and removing it felt correct conceptually. For the same reason, I wouldn't worry about eliminating a valid FDE; that would be a logic bug on our end and not something that could ever be legitimately hit.

In D132489#3748396 <https://reviews.llvm.org/D132489#3748396>, @int3 wrote:

> @thevinster I think removing `S_ATTR_LIVE_SUPPORT` is probably the right fix here; it seems kind of unintuitive to rely on ICF to perform what is effectively dead stripping, and FDEs should indeed be kept alive only by the function symbol that they belong to. I don't think we need to worry about valid FDEs getting stripped. But yeah I would love to better understand how the CIE is getting stripped; I would expect it not to be, given that there should be a relocation from the FDE to the CIE that prevents that...

That relocation is created by ehRelocator.commit() <https://github.com/llvm/llvm-project/blob/00e51f04e8c10d0ad3e6089c9ab490394cd69a83/lld/MachO/InputFiles.cpp#L1594>, which is skipped if the FDE is dead <https://github.com/llvm/llvm-project/blob/00e51f04e8c10d0ad3e6089c9ab490394cd69a83/lld/MachO/InputFiles.cpp#L1578>. I don't know if that was intentional or not. I debated changing that as well, but then we'd lose the ability to detect bad `__eh_frame` GCing (whereas right now `llvm-dwarfdump --eh-frame` was loudly warning us that we were getting it wrong). We could recover that ability by setting a "this should really be dead" type bit on the dead FDE section and then erroring out if we found that such a section was in fact considered live at the end, but that felt like overkill. (We could also use such a bit to override any dead-stripping decisions instead of just erroring, but that felt like a hack.) If we want to keep the current setup where we don't create the FDE relocations for a dead FDE, I'm happy to add a comment (either in this diff or separately) explaining that.

Btw, while I have you here, you have a comment <https://github.com/llvm/llvm-project/blob/00e51f04e8c10d0ad3e6089c9ab490394cd69a83/lld/MachO/InputFiles.cpp#L1575> that:

> We must prune unused FDEs for correctness, so we cannot rely on -dead_strip being enabled.

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.

================
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.
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132489