Page MenuHomePhabricator

[MachO] Fix dead-stripping __eh_frame
ClosedPublic

Authored by smeenai on Aug 23 2022, 10:50 AM.

Details

Reviewers
int3
ributzka
Group Reviewers
Restricted Project
Commits
rGa745e47900dd: [MachO] Fix dead-stripping __eh_frame
Summary

This section is marked S_ATTR_LIVE_SUPPORT in input files, which meant
that on arm64, we were unnecessarily preserving FDEs if we e.g. had
multiple weak definitions for a function. Worse, 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. We set up associations from functions
to their FDEs, so dead-stripping will just work naturally, and we can
clear S_ATTR_LIVE_SUPPORT from our input __eh_frame sections to fix
dead-stripping.

Diff Detail

Event Timeline

smeenai created this revision.Aug 23 2022, 10:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 23 2022, 10:50 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
smeenai requested review of this revision.Aug 23 2022, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 10:50 AM
smeenai edited the summary of this revision. (Show Details)Aug 23 2022, 10:53 AM
smeenai updated this revision to Diff 454897.Aug 23 2022, 10:54 AM

Clear lint warning

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?

int3 added a subscriber: int3.Aug 25 2022, 2:24 AM

@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...

lld/test/MachO/eh-frame-dead-strip.s
10–11

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?

@int3 Thanks for the clarification! In that case, this change LGTM.

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.

@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(), which is skipped if the FDE is dead. 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 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.

lld/test/MachO/eh-frame-dead-strip.s
10–11

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.

int3 accepted this revision.Aug 26 2022, 7:42 AM

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

lld/test/MachO/eh-frame-dead-strip.s
10–11

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

This revision is now accepted and ready to land.Aug 26 2022, 7:42 AM
smeenai updated this revision to Diff 455953.Aug 26 2022, 10:32 AM

Test both architectures and orders

This revision was automatically updated to reflect the committed changes.