[lld-macho][nfc] Add more tests + comments around ICF + unwind info interaction

While working on {D129830}, I realized that our handling of ICF +
eh_frame combined was untested. Additionally I realized that the comment
explaining why we were safely slicing away the functionAddress reloc
from our compact unwind entries was... insufficient and slightly
misleading. I've tried to clarify it.

Reviewed By: #lld-macho, thevinster

Differential Revision: https://reviews.llvm.org/D129894

2 files changed, 61 insertions, 6 deletions

diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index fda6900..225a712 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -1240,11 +1240,23 @@ void ObjFile::registerCompactUnwind(Section &compactUnwindSection) {
         continue;
       }
       d->unwindEntry = isec;
-      // Since we've sliced away the functionAddress, we should remove the
-      // corresponding relocation too. Given that clang emits relocations in
-      // reverse order of address, this relocation should be at the end of the
-      // vector for most of our input object files, so this is typically an O(1)
-      // operation.
+      // Now that the symbol points to the unwind entry, we can remove the reloc
+      // that points from the unwind entry back to the symbol.
+      //
+      // First, the symbol keeps the unwind entry alive (and not vice versa), so
+      // this keeps dead-stripping simple.
+      //
+      // Moreover, it reduces the work that ICF needs to do to figure out if
+      // functions with unwind info are foldable.
+      //
+      // However, this does make it possible for ICF to fold CUEs that point to
+      // distinct functions (if the CUEs are otherwise identical).
+      // UnwindInfoSection takes care of this by re-duplicating the CUEs so that
+      // each one can hold a distinct functionAddress value.
+      //
+      // Given that clang emits relocations in reverse order of address, this
+      // relocation should be at the end of the vector for most of our input
+      // object files, so this erase() is typically an O(1) operation.
       it = isec->relocs.erase(it);
     }
   }
diff --git a/lld/test/MachO/icf.s b/lld/test/MachO/icf.s
index c368b86..417549f 100644
--- a/lld/test/MachO/icf.s
+++ b/lld/test/MachO/icf.s
@@ -10,7 +10,7 @@
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 %t/main.s -o %t/main.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 %t/abs.s -o %t/abs.o
 # RUN: %lld -lSystem --icf=all -o %t/main %t/main.o %t/abs.o
-# RUN: llvm-objdump -d --syms %t/main | FileCheck %s
+# RUN: llvm-objdump -d --syms --dwarf=frames %t/main | FileCheck %s
 
 # CHECK-LABEL: SYMBOL TABLE:
 # CHECK: [[#%x,ABS1B_REF:]]                 l     F __TEXT,__text _abs1a_ref
@@ -37,6 +37,9 @@
 # CHECK: [[#%x,HAS_UNWIND_2:]]              l     F __TEXT,__text _has_unwind_1
 # CHECK: [[#%x,HAS_UNWIND_2]]               l     F __TEXT,__text _has_unwind_2
 # CHECK: [[#%x,HAS_UNWIND_3:]]              l     F __TEXT,__text _has_unwind_3
+# CHECK: [[#%x,HAS_EH_FRAME_1:]]            l     F __TEXT,__text _has_eh_frame_1
+# CHECK: [[#%x,HAS_EH_FRAME_2:]]            l     F __TEXT,__text _has_eh_frame_2
+# CHECK: [[#%x,HAS_EH_FRAME_3:]]            l     F __TEXT,__text _has_eh_frame_3
 # CHECK: [[#%x,MUTALLY_RECURSIVE_2:]]       l     F __TEXT,__text _mutually_recursive_1
 # CHECK: [[#%x,MUTALLY_RECURSIVE_2]]        l     F __TEXT,__text _mutually_recursive_2
 # CHECK: [[#%x,INIT_2:]]                    l     F __TEXT,__text _init_1
@@ -46,6 +49,11 @@
 # COM:   [[#%x,ASYMMETRIC_RECURSIVE_2:]]    l   F __TEXT,__text _asymmetric_recursive_1
 # COM:   [[#%x,ASYMMETRIC_RECURSIVE_2]]     l   F __TEXT,__text _asymmetric_recursive_2
 
+## Check that we don't accidentally dedup distinct EH frames.
+# CHECK: FDE {{.*}} pc=[[#%x,HAS_EH_FRAME_1]]
+# CHECK: FDE {{.*}} pc=[[#%x,HAS_EH_FRAME_2]]
+# CHECK: FDE {{.*}} pc=[[#%x,HAS_EH_FRAME_3]]
+
 # CHECK-LABEL: Disassembly of section __TEXT,__text:
 # CHECK:        <_main>:
 # CHECK: callq 0x[[#%x,ABS1B_REF]]                 <_abs1b_ref>
@@ -72,6 +80,9 @@
 # CHECK: callq 0x[[#%x,HAS_UNWIND_2]]              <_has_unwind_2>
 # CHECK: callq 0x[[#%x,HAS_UNWIND_2]]              <_has_unwind_2>
 # CHECK: callq 0x[[#%x,HAS_UNWIND_3]]              <_has_unwind_3>
+# CHECK: callq 0x[[#%x,HAS_EH_FRAME_1]]            <_has_eh_frame_1>
+# CHECK: callq 0x[[#%x,HAS_EH_FRAME_2]]            <_has_eh_frame_2>
+# CHECK: callq 0x[[#%x,HAS_EH_FRAME_3]]            <_has_eh_frame_3>
 # CHECK: callq 0x[[#%x,MUTALLY_RECURSIVE_2]]       <_mutually_recursive_2>
 # CHECK: callq 0x[[#%x,MUTALLY_RECURSIVE_2]]       <_mutually_recursive_2>
 ## FIXME Mutually-recursive functions with identical bodies (see below)
@@ -209,6 +220,35 @@ _has_unwind_3:
   ret
   .cfi_endproc
 
+## In theory _has_eh_frame_{1, 2} can be dedup'ed, but we don't support this
+## yet.
+_has_eh_frame_1:
+  .cfi_startproc
+  .cfi_def_cfa_offset 8
+  ## cfi_escape cannot be encoded in compact unwind
+  .cfi_escape 0x2e, 0x10
+  ret
+  .cfi_endproc
+
+_has_eh_frame_2:
+  .cfi_startproc
+  .cfi_def_cfa_offset 8
+  ## cfi_escape cannot be encoded in compact unwind
+  .cfi_escape 0x2e, 0x10
+  ret
+  .cfi_endproc
+
+## The nop in this function body means that it cannot be folded with the
+## previous two, even though the unwind info is otherwise identical.
+_has_eh_frame_3:
+  .cfi_startproc
+  .cfi_def_cfa_offset 8
+  ## cfi_escape cannot be encoded in compact unwind
+  .cfi_escape 0x2e, 0x10
+  nop
+  ret
+  .cfi_endproc
+
 ## Fold: Mutually-recursive functions with symmetric bodies
 _mutually_recursive_1:
   callq _mutually_recursive_1 # call myself
@@ -279,6 +319,9 @@ _main:
   callq _has_unwind_1
   callq _has_unwind_2
   callq _has_unwind_3
+  callq _has_eh_frame_1
+  callq _has_eh_frame_2
+  callq _has_eh_frame_3
   callq _mutually_recursive_1
   callq _mutually_recursive_2
   callq _asymmetric_recursive_1