[MachO] Fix dead-stripping __eh_frame

[smeenai]

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.

[smeenai]

Clear lint warning

[thevinster]

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]

@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

9–10

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?

[thevinster]

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

[smeenai]

In 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, @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(), 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

9–10

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]

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

9–10

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

[smeenai]

Test both architectures and orders

lld/MachO/InputFiles.cpp

if (lsdaAddrRelocIt != isec->relocs.end()) {
           cast<ConcatInputSection>(findContainingSubsection(*sec, &lsdaAddr));
       ehRelocator.makePcRel(lsdaAddrOff, lsdaIsec, target->p2WordSize);
     }
 
     fdes[isec] = {funcLength, cie.personalitySymbol, lsdaIsec};
     funcSym->unwindEntry = isec;
     ehRelocator.commit();
   }
+
+  // __eh_frame is marked as S_ATTR_LIVE_SUPPORT in input files, because FDEs
+  // are normally required to be kept alive if they reference a live symbol.
+  // However, we've explicitly marked a dependency from a symbol to its FDE, so
+  // dead-stripping will just work as usual, and S_ATTR_LIVE_SUPPORT will just
+  // incorrectly prevent us from dead-stripping duplicate FDEs for a live symbol
+  // (e.g. if there were multiple weak copies). Remove this flag and just let
+  // dead-stripping proceed as normal.
+  ehFrameSection.flags &= ~S_ATTR_LIVE_SUPPORT;
 }
 
 std::string ObjFile::sourceFile() const {
   SmallString<261> dir(compileUnit->getCompilationDir());
   StringRef sep = sys::path::get_separator();
   // We don't use `path::append` here because we want an empty `dir` to result
   // in an absolute path. `append` would give us a relative path for that case.
   if (!dir.endswith(sep))

lld/test/MachO/eh-frame-dead-strip.s

+# REQUIRES: aarch64
+# RUN: rm -rf %t; split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos11.0 %t/strong.s -o %t/strong.o
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-macos11.0 %t/weak.s -o %t/weak.o
+# RUN: %lld -arch arm64 -dylib -dead_strip %t/strong.o %t/weak.o -o %t/libstrongweak.dylib
+# RUN: llvm-dwarfdump --eh-frame %t/libstrongweak.dylib | FileCheck %s
+
+## 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.
+
+# CHECK: .eh_frame contents:
+# CHECK: 00000000 00000010 00000000 CIE
+# CHECK: 00000014 00000020 00000018 FDE cie=00000000
+# CHECK-NOT: CIE
+# CHECK-NOT: FDE
+
+#--- strong.s
+.globl _fun
+_fun:
+  .cfi_startproc
+  .cfi_def_cfa_offset 0
+  ## cfi_escape cannot be encoded in compact unwind
+  .cfi_escape 0x2e, 0x10
+  ret
+  .cfi_endproc
+
+#--- weak.s
+.globl _fun
+.weak_definition _fun
+_fun:
+  .cfi_startproc
+  .cfi_def_cfa_offset 0
+  ## cfi_escape cannot be encoded in compact unwind
+  .cfi_escape 0x2e, 0x10
+  ret
+  .cfi_endproc