[lld] 65226d3 - [lld-macho] Fix bug in CUE folding that resulted in wrong unwind table.

[Vy Nguyen via llvm-commits]

Author: Vy Nguyen
Date: 2022-11-22T10:39:51-05:00
New Revision: 65226d3f1f533914a5fcc2d94bb1ff95b0d55d5d

URL: https://github.com/llvm/llvm-project/commit/65226d3f1f533914a5fcc2d94bb1ff95b0d55d5d
DIFF: https://github.com/llvm/llvm-project/commit/65226d3f1f533914a5fcc2d94bb1ff95b0d55d5d.diff

LOG: [lld-macho] Fix bug in CUE folding that resulted in wrong unwind table.

PR/59070

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

Added: 
    lld/test/MachO/compact-unwind-foldings.s

Modified: 
    lld/MachO/UnwindInfoSection.cpp

Removed: 
    


################################################################################
diff  --git a/lld/MachO/UnwindInfoSection.cpp b/lld/MachO/UnwindInfoSection.cpp
index b83aa896b1527..6a58b5129478b 100644
--- a/lld/MachO/UnwindInfoSection.cpp
+++ b/lld/MachO/UnwindInfoSection.cpp
@@ -179,6 +179,9 @@ class UnwindInfoSectionImpl final : public UnwindInfoSection {
   DenseMap<size_t, uint32_t> lsdaIndex;
   std::vector<SecondLevelPage> secondLevelPages;
   uint64_t level2PagesOffset = 0;
+  // The highest-address function plus its size. The unwinder needs this to
+  // determine the address range that is covered by unwind info.
+  uint64_t cueEndBoundary = 0;
 };
 
 UnwindInfoSection::UnwindInfoSection()
@@ -458,6 +461,10 @@ void UnwindInfoSectionImpl::finalize() {
     return cuEntries[a].functionAddress < cuEntries[b].functionAddress;
   });
 
+  // Record the ending boundary before we fold the entries.
+  cueEndBoundary = cuEntries[cuIndices.back()].functionAddress +
+                   cuEntries[cuIndices.back()].functionLength;
+
   // Fold adjacent entries with matching encoding+personality and without LSDA
   // We use three iterators on the same cuIndices to fold in-situ:
   // (1) `foldBegin` is the first of a potential sequence of matching entries
@@ -631,6 +638,9 @@ void UnwindInfoSectionImpl::writeTo(uint8_t *buf) const {
   for (const Symbol *personality : personalities)
     *i32p++ = personality->getGotVA() - in.header->addr;
 
+  // FIXME: LD64 checks and warns aboutgaps or overlapse in cuEntries address
+  // ranges. We should do the same too
+
   // Level-1 index
   uint32_t lsdaOffset =
       uip->indexSectionOffset +
@@ -648,9 +658,10 @@ void UnwindInfoSectionImpl::writeTo(uint8_t *buf) const {
     l2PagesOffset += SECOND_LEVEL_PAGE_BYTES;
   }
   // Level-1 sentinel
-  const CompactUnwindEntry &cuEnd = cuEntries[cuIndices.back()];
-  iep->functionOffset =
-      cuEnd.functionAddress - in.header->addr + cuEnd.functionLength;
+  // XXX(vyng): Note that LD64 adds +1 here.
+  // Unsure whether it's a bug or it's their workaround for something else.
+  // See comments from https://reviews.llvm.org/D138320.
+  iep->functionOffset = cueEndBoundary - in.header->addr;
   iep->secondLevelPagesSectionOffset = 0;
   iep->lsdaIndexArraySectionOffset =
       lsdaOffset + entriesWithLsda.size() *

diff  --git a/lld/test/MachO/compact-unwind-foldings.s b/lld/test/MachO/compact-unwind-foldings.s
new file mode 100644
index 0000000000000..18511da772144
--- /dev/null
+++ b/lld/test/MachO/compact-unwind-foldings.s
@@ -0,0 +1,104 @@
+# REQUIRES: x86
+# Tests to verify that
+#    - the folded CUEs covered up to the last entries, even if they were removed/folded.
+#    - only entries that *should* be in eh_frame are actually included. (**)
+# (**) This is where LD64 does 
diff erently.
+
+# RUN: rm -rf %t; split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/fold-tail.s -o %t/fold-tail.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/cues-range-gap.s -o %t/cues-range-gap.o
+
+# RUN: %lld -o %t/fold-tail.out %t/fold-tail.o
+# RUN: llvm-objdump --macho --syms --unwind-info %t/fold-tail.out | FileCheck %s 
+
+# RUN: %lld -o %t/cues-range-gap.out %t/cues-range-gap.o
+
+# CHECK-LABEL: SYMBOL TABLE:
+# CHECK:       [[#%x,A_ADDR:]]  l     F __TEXT,__text _a
+# CHECK:       [[#%x,B_ADDR:]]  l     F __TEXT,__text _b
+# CHECK:       [[#%x,C_ADDR:]]  l     F __TEXT,__text _c
+# CHECK:       [[#%x,D_ADDR:]]  l     F __TEXT,__text _d
+# CHECK:       [[#%x,MAIN_ADDR:]]  g     F __TEXT,__text _main
+
+## Check that [1] offset starts at c's address + 3 (its length).
+# CHECK-LABEL: Contents of __unwind_info section:
+# CHECK:  Top level indices: (count = 2)
+# CHECK-NEXT : [0]: function offset=[[#%#.7x,MAIN_ADDR]]
+# CHECK-NEXT : [1]: function offset=[[#%#.7x,C_ADDR + 3]]
+
+#--- fold-tail.s
+        .text
+        .globl  _main
+        .p2align        4, 0x90
+_main:
+        .cfi_startproc
+        callq   _a
+        retq
+        .cfi_endproc
+
+        .p2align        4, 0x90
+_a:
+        .cfi_startproc
+        callq   _b
+        retq
+        .cfi_endproc
+
+        .p2align        4, 0x90
+_b:
+        .cfi_startproc
+        callq   _c
+        retq
+        .cfi_endproc
+
+        .p2align        4, 0x90
+_c:
+        .cfi_startproc
+        retq
+        .cfi_endproc
+
+// _d should NOT have an entry in .eh_frame
+// So it shouldn't be covered by the unwind table.
+_d:
+        xorl %eax, %eax
+        ret
+
+.subsections_via_symbols
+
+#--- cues-range-gap.s
+        .text
+        .globl  _main
+        .p2align        4, 0x90
+_main:
+        .cfi_startproc
+        callq   _a
+        retq
+        .cfi_endproc
+
+        .p2align        4, 0x90
+_a:
+        .cfi_startproc
+        callq   _b
+        retq
+        .cfi_endproc
+
+// _d is intentionally placed in between the symbols with eh_frame entries.
+// but _d should NOT have an entry in .eh_frame
+// So it shouldn't be covered by the unwind table.
+_d:
+        xorl %eax, %eax
+        ret
+
+        .p2align        4, 0x90
+_b:
+        .cfi_startproc
+        callq   _c
+        retq
+        .cfi_endproc
+
+        .p2align        4, 0x90
+_c:
+        .cfi_startproc
+        retq
+        .cfi_endproc
+
+.subsections_via_symbols