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

[oontvoo]

Summary

PR/59070

[jyknight]

Code change looks good to me!

[int3]

Thanks for tracking this down!!

lld/MachO/UnwindInfoSection.cpp

182

nit: rm blank line

183

nit 1: I think "largest" is more descriptive than "final" (plus "final" often has temporal-ordering connotations, e.g. InputSection::isFinal). Also maybe unwindAddressBoundary or cuAddressBoundary might be a better name for the variable, for similar reasons?

nit 2: I don't think "this should covered folded entries as well" is a super clear explanation; I thought about how to elaborate on it, but then I think maybe it's better to let the reader figure that out by reading the comments in the implementation. I guess it's okay if you want to keep that line though.

464

"remember" usually means "recall", rather than record/save.

658

this is now unused

659

I'm not entirely sure why, but this is what ld64 does

lld/test/MachO/compact-unwind-folding-bug.s

2

just to be clear, 'remove boilerplate' will include removing all the LSDA stuff, yes? we don't need to have a file that repros the actual no-catch bug, just something that checks that the last address of a bunch of folded CUEs is correct

[jyknight]

lld/MachO/UnwindInfoSection.cpp

659

Adding 1 does make sense. This value is the start of the "next function", so it's an exclusive bound, and you want the final byte of the function to actually be included in the range.

[int3]

lld/MachO/UnwindInfoSection.cpp

659

That was my initial thought too, but shouldn't adding the length already make it an exclusive bound?

[oontvoo]

lld/MachO/UnwindInfoSection.cpp

659

hmm yeah, looks like LD64 does add 1. (here and a couple of lines up in the secondLevelPages loop too).

[oontvoo]

addressed review comments + simplified test.

Thanks!

[oontvoo]

fixed test + off-by-one error

[jyknight]

lld/MachO/UnwindInfoSection.cpp

659

@int3: oh, good point! ld64's code is actually making it too large by one byte. I suspect that's by accident, and a mistake which we should not replicate.

[int3]

Thanks!

lld/MachO/UnwindInfoSection.cpp

183

thanks for changing the comments!

thoughts on the variable rename?

641

659

or possibly to work around a mistake in the runtime unwinder... hard to say without carefully reading its implementation.

@oontvoo I'll leave it up to you whether you want to leave the +1 in. We could certainly omit it and see if it causes any problems in practice.

lld/test/MachO/compact-unwind-folding-bug.s

3

How about

This is a regression test. for verifying that the level-1 index sentinel entry has the correct address value. In particular, we used to emit the wrong value when the last N compact unwind entries were folded together.

I'm also wondering if the test file name can have something more specific than "folding bug"... maybe compact-unwind-tail-folding.s? Because it's not a bug in folding in general, only when the fold happens at the tail entries of the CUE array.

20

(alternatively you could omit it entirely, MC emits stuff into .text by default)

25–28

can we remove more boilerplate? (likewise for the other functions)

57–60

why do we need this?

[oontvoo]

lld/MachO/UnwindInfoSection.cpp

183

changed to cueEndBoundary ?

659

I'll remove the +1 since it seems wrong (ie., if there happens to be stuff after the last entry taht *shouldn't* be in it, then the table would still include it.)

Also speaking of "strange", there seems to be another edge case here. I've added the test case below.

Should we replicate this or not? (my guess is no, but /shrug)

lld/test/MachO/compact-unwind-folding-bug.s

3

How about just compact-unwind-folding.s?
(added two additional test cases, where we behave slightly differently from LD64 ... LMK what you think)

25–28

We probably could - but slightly prefer that these symbols refer to each other (so that they don't look like they're all unused)
(I did remove the push/pop pairs to make the test smaller)

[oontvoo]

updated tests and remove the off-by-one "bug"

[int3]

lld/MachO/UnwindInfoSection.cpp

183

sounds good!

lld/test/MachO/compact-unwind-folding-bug.s

3

sgtm!

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

7 ↗

(On Diff #476973)

59–60 ↗

(On Diff #476973)

this makes sense to me, and I think we can keep our deviation from ld64's behavior here

61 ↗

(On Diff #476973)

is this directive necessary?

85–86 ↗

(On Diff #476973)

both ld64 and LLD do include an entry (with encoding 0x0) for this, right?

I'm not sure there's a way to *not* include _d here, since if there isn't an entry with 0x0 encoding, then the unwinder just uses the CUE at the most recent preceding address. So I think ld64 is working correctly here

88 ↗

(On Diff #476973)

ditto

[oontvoo]

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

85–86 ↗

(On Diff #476973)

right - when _d is in the middle of the range, we can't exclude it.

[oontvoo]

address review comments + rebase

[oontvoo]

thanks!

lld/MachO/UnwindInfoSection.cpp

private:
   SmallDenseMap<std::pair<InputSection *, uint64_t /* addend */>, Symbol *>
       personalityTable;
   // Indices into cuEntries for CUEs with a non-null LSDA.
   std::vector<size_t> entriesWithLsda;
   // Map of cuEntries index to an index within the LSDA array.
   DenseMap<size_t, uint32_t> lsdaIndex;
   std::vector<SecondLevelPage> secondLevelPages;
   uint64_t level2PagesOffset = 0;
+
+  // The final ending address (this should covered folded entries as well)
+  uint64_t finalEndingAddress = 0;
 };
 
 UnwindInfoSection::UnwindInfoSection()
     : SyntheticSection(segment_names::text, section_names::unwindInfo) {
   align = 4;
 }
 
 // Record function symbols that may need entries emitted in __unwind_info, which
void UnwindInfoSectionImpl::finalize() {
   // Rather than sort & fold the 32-byte entries directly, we create a
   // vector of indices to entries and sort & fold that instead.
   cuIndices.resize(cuEntries.size());
   std::iota(cuIndices.begin(), cuIndices.end(), 0);
   llvm::sort(cuIndices, [&](size_t a, size_t b) {
     return cuEntries[a].functionAddress < cuEntries[b].functionAddress;
   });
 
+  // Remember the ending boundary before we fold the entries.
+  finalEndingAddress = 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
   // (2) `foldEnd` is the first non-matching entry after `foldBegin`.
   // The semi-open interval [ foldBegin .. foldEnd ) contains a range
   // entries that can be folded into a single entry and written to ...
   // (3) `foldWrite`
   auto foldWrite = cuIndices.begin();
void UnwindInfoSectionImpl::writeTo(uint8_t *buf) const {
   auto *i32p = reinterpret_cast<uint32_t *>(&uip[1]);
   for (const auto &encoding : commonEncodings)
     *i32p++ = encoding.first;
 
   // Personalities
   for (const Symbol *personality : personalities)
     *i32p++ = personality->getGotVA() - in.header->addr;
 
   // Level-1 index
   uint32_t lsdaOffset =
       uip->indexSectionOffset +
       uip->indexCount * sizeof(unwind_info_section_header_index_entry);
   uint64_t l2PagesOffset = level2PagesOffset;
   auto *iep = reinterpret_cast<unwind_info_section_header_index_entry *>(i32p);
   for (const SecondLevelPage &page : secondLevelPages) {
     size_t idx = cuIndices[page.entryIndex];
     iep->functionOffset = cuEntries[idx].functionAddress - in.header->addr;
     iep->secondLevelPagesSectionOffset = l2PagesOffset;
     iep->lsdaIndexArraySectionOffset =
         lsdaOffset + lsdaIndex.lookup(idx) *
                          sizeof(unwind_info_section_header_lsda_index_entry);
     iep++;
     l2PagesOffset += SECOND_LEVEL_PAGE_BYTES;
   }
   // Level-1 sentinel
   const CompactUnwindEntry &cuEnd = cuEntries[cuIndices.back()];
-  iep->functionOffset =
-      cuEnd.functionAddress - in.header->addr + cuEnd.functionLength;
+  iep->functionOffset = finalEndingAddress - in.header->addr;
   iep->secondLevelPagesSectionOffset = 0;
   iep->lsdaIndexArraySectionOffset =
       lsdaOffset + entriesWithLsda.size() *
                        sizeof(unwind_info_section_header_lsda_index_entry);
   iep++;
 
   // LSDAs
   auto *lep =

lld/test/MachO/compact-unwind-folding-bug.s

+# Test to verify that the folded CUEs covered up to the last entries, even if they were removed/folded.
+# TODO: fill in the assertions and remove boilerpalte
+
+        .section        __TEXT,__text,regular,pure_instructions
+        .globl  _main                           ## -- Begin function main
+        .p2align        4, 0x90
+_main:                                  ## @main
+        .cfi_startproc
+## %bb.0:
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        subq    $16, %rsp
+        movl    $0, -4(%rbp)
+        movl    $12, %edi
+        callq   _catchFunc
+        xorl    %eax, %eax
+        addq    $16, %rsp
+        popq    %rbp
+        retq
+        .cfi_endproc
+                                        ## -- End function
+        .p2align        4, 0x90                         ## -- Begin function catchFunc
+_catchFunc:                             ## @catchFunc
+Lfunc_begin0:
+        .cfi_startproc
+        .cfi_personality 155, ___objc_personality_v0
+        .cfi_lsda 16, Lexception0
+## %bb.0:
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        subq    $32, %rsp
+        movl    %edi, -4(%rbp)
+        movl    -4(%rbp), %edi
+Ltmp0:
+        callq   _middleFunc1
+Ltmp1:
+        jmp     LBB1_1
+LBB1_1:
+        jmp     LBB1_2
+LBB1_2:
+        addq    $32, %rsp
+        popq    %rbp
+        retq
+LBB1_3:
+Ltmp2:
+        movq    %rax, %rcx
+        movl    %edx, %eax
+        movq    %rcx, -16(%rbp)
+        movl    %eax, -20(%rbp)
+## %bb.4:
+        movl    -20(%rbp), %eax
+        movl    $1, %ecx
+        cmpl    %ecx, %eax
+        jne     LBB1_8
+## %bb.5:
+        movq    -16(%rbp), %rdi
+        callq   _objc_begin_catch
+        movq    %rax, %rdi
+        movq    _objc_retain@GOTPCREL(%rip), %rax
+        callq   *%rax
+        movq    %rax, -32(%rbp)
+Ltmp3:
+        leaq    L__unnamed_cfstring_(%rip), %rdi
+        xorl    %eax, %eax
+                                        ## kill: def $al killed $al killed $eax
+        callq   _NSLog
+Ltmp4:
+        jmp     LBB1_6
+LBB1_6:
+        leaq    -32(%rbp), %rdi
+        xorl    %eax, %eax
+        movl    %eax, %esi
+        callq   _objc_storeStrong
+        callq   _objc_end_catch
+        jmp     LBB1_2
+LBB1_7:
+Ltmp5:
+        movq    %rax, %rcx
+        movl    %edx, %eax
+        movq    %rcx, -16(%rbp)
+        movl    %eax, -20(%rbp)
+        callq   _objc_end_catch
+LBB1_8:
+        movq    -16(%rbp), %rdi
+        callq   __Unwind_Resume
+Lfunc_end0:
+        .cfi_endproc
+        .section        __TEXT,__gcc_except_tab
+        .p2align        2
+GCC_except_table1:
+Lexception0:
+        .byte   255                             ## @LPStart Encoding = omit
+        .byte   155                             ## @TType Encoding = indirect pcrel sdata4
+        .uleb128 Lttbase0-Lttbaseref0
+Lttbaseref0:
+        .byte   1                               ## Call site Encoding = uleb128
+        .uleb128 Lcst_end0-Lcst_begin0
+Lcst_begin0:
+        .uleb128 Ltmp0-Lfunc_begin0             ## >> Call Site 1 <<
+        .uleb128 Ltmp1-Ltmp0                    ##   Call between Ltmp0 and Ltmp1
+        .uleb128 Ltmp2-Lfunc_begin0             ##     jumps to Ltmp2
+        .byte   1                               ##   On action: 1
+        .uleb128 Ltmp1-Lfunc_begin0             ## >> Call Site 2 <<
+        .uleb128 Ltmp3-Ltmp1                    ##   Call between Ltmp1 and Ltmp3
+        .byte   0                               ##     has no landing pad
+        .byte   0                               ##   On action: cleanup
+        .uleb128 Ltmp3-Lfunc_begin0             ## >> Call Site 3 <<
+        .uleb128 Ltmp4-Ltmp3                    ##   Call between Ltmp3 and Ltmp4
+        .uleb128 Ltmp5-Lfunc_begin0             ##     jumps to Ltmp5
+        .byte   0                               ##   On action: cleanup
+        .uleb128 Ltmp4-Lfunc_begin0             ## >> Call Site 4 <<
+        .uleb128 Lfunc_end0-Ltmp4               ##   Call between Ltmp4 and Lfunc_end0
+        .byte   0                               ##     has no landing pad
+        .byte   0                               ##   On action: cleanup
+Lcst_end0:
+        .byte   1                               ## >> Action Record 1 <<
+                                        ##   Catch TypeInfo 1
+        .byte   0                               ##   No further actions
+        .p2align        2
+                                        ## >> Catch TypeInfos <<
+        .long   _OBJC_EHTYPE_$_NSException@GOTPCREL+4 ## TypeInfo 1
+Lttbase0:
+        .p2align        2
+                                        ## -- End function
+        .section        __TEXT,__text,regular,pure_instructions
+        .p2align        4, 0x90                         ## -- Begin function middleFunc1
+_middleFunc1:                           ## @middleFunc1
+        .cfi_startproc
+## %bb.0:
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        subq    $16, %rsp
+        movl    %edi, -4(%rbp)
+        movl    -4(%rbp), %edi
+        callq   _throwExcFunc
+        addq    $16, %rsp
+        popq    %rbp
+        retq
+        .cfi_endproc
+                                        ## -- End function
+        .p2align        4, 0x90                         ## -- Begin function throwExcFunc
+_throwExcFunc:                          ## @throwExcFunc
+        .cfi_startproc
+## %bb.0:
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
+        subq    $16, %rsp
+        movl    %edi, -4(%rbp)
+        movq    _OBJC_CLASSLIST_REFERENCES_$_(%rip), %rdi
+        movq    _OBJC_SELECTOR_REFERENCES_(%rip), %rsi
+        leaq    L__unnamed_cfstring_.2(%rip), %rdx
+        leaq    L__unnamed_cfstring_.4(%rip), %rcx
+        movq    _objc_msgSend@GOTPCREL(%rip), %rax
+        xorl    %r8d, %r8d
+                                        ## kill: def $r8 killed $r8d
+        callq   *%rax
+        movq    %rax, %rdi
+        callq   _objc_retainAutoreleasedReturnValue
+        movq    %rax, -16(%rbp)
+        movq    -16(%rbp), %rdi
+        callq   _objc_retainAutorelease
+        movq    %rax, %rdi
+        callq   _objc_exception_throw
+        .cfi_endproc
+                                        ## -- End function
+        .section        __TEXT,__cstring,cstring_literals
+L_.str:                                 ## @.str
+        .asciz  "^^^^caught the exception!!!"
+
+        .section        __DATA,__cfstring
+        .p2align        3                               ## @_unnamed_cfstring_
+L__unnamed_cfstring_:
+        .quad   ___CFConstantStringClassReference
+        .long   1992                            ## 0x7c8
+        .space  4
+        .quad   L_.str
+        .quad   27                              ## 0x1b
+
+        .section        __DATA,__objc_classrefs,regular,no_dead_strip
+        .p2align        3                               ## @"OBJC_CLASSLIST_REFERENCES_$_"
+_OBJC_CLASSLIST_REFERENCES_$_:
+        .quad   _OBJC_CLASS_$_NSException
+
+        .section        __TEXT,__cstring,cstring_literals
+L_.str.1:                               ## @.str.1
+        .asciz  "MyCaughtException"
+
+        .section        __DATA,__cfstring
+        .p2align        3                               ## @_unnamed_cfstring_.2
+L__unnamed_cfstring_.2:
+        .quad   ___CFConstantStringClassReference
+        .long   1992                            ## 0x7c8
+        .space  4
+        .quad   L_.str.1
+        .quad   17                              ## 0x11
+
+        .section        __TEXT,__cstring,cstring_literals
+L_.str.3:                               ## @.str.3
+        .asciz  "Fake exception thrown WITH CATCH"
+
+        .section        __DATA,__cfstring
+        .p2align        3                               ## @_unnamed_cfstring_.4
+L__unnamed_cfstring_.4:
+        .quad   ___CFConstantStringClassReference
+        .long   1992                            ## 0x7c8
+        .space  4
+        .quad   L_.str.3
+        .quad   32                              ## 0x20
+
+        .section        __TEXT,__objc_methname,cstring_literals
+L_OBJC_METH_VAR_NAME_:                  ## @OBJC_METH_VAR_NAME_
+        .asciz  "exceptionWithName:reason:userInfo:"
+
+        .section        __DATA,__objc_selrefs,literal_pointers,no_dead_strip
+        .p2align        3                               ## @OBJC_SELECTOR_REFERENCES_
+_OBJC_SELECTOR_REFERENCES_:
+        .quad   L_OBJC_METH_VAR_NAME_
+
+        .section        __DATA,__objc_imageinfo,regular,no_dead_strip
+L_OBJC_IMAGE_INFO:
+        .long   0
+        .long   96
+
+.subsections_via_symbols