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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

Thanks!



================
Comment at: lld/MachO/UnwindInfoSection.cpp:641
 
+  // FIXME: LD64 checks and warns aboutgaps or overlapse in cuEntries address
+  // ranges. We should do the same too
----------------



================
Comment at: lld/MachO/UnwindInfoSection.cpp:183
+
+  // The final ending address (this should covered folded entries as well)
+  uint64_t finalEndingAddress = 0;
----------------
int3 wrote:
> 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.
thanks for changing the comments!

thoughts on the variable rename?


================
Comment at: lld/MachO/UnwindInfoSection.cpp:659
   const CompactUnwindEntry &cuEnd = cuEntries[cuIndices.back()];
-  iep->functionOffset =
-      cuEnd.functionAddress - in.header->addr + cuEnd.functionLength;
+  iep->functionOffset = finalEndingAddress - in.header->addr;
   iep->secondLevelPagesSectionOffset = 0;
----------------
jyknight wrote:
> oontvoo wrote:
> > int3 wrote:
> > > jyknight wrote:
> > > > int3 wrote:
> > > > > I'm not entirely sure why, but this is what ld64 does
> > > > 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.
> > > > 
> > > That was my initial thought too, but shouldn't adding the length already make it an exclusive bound?
> > hmm yeah, looks like LD64 does add 1. (here and a couple of lines up in the secondLevelPages loop too).
> > 
> @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.
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.


================
Comment at: lld/test/MachO/compact-unwind-folding-bug.s:2
+# REQUIRES: x86
+# Test to verify that the folded CUEs covered up to the last entries, even if they were removed/folded.
+
----------------
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.


================
Comment at: lld/test/MachO/compact-unwind-folding-bug.s:19
+# CHECK-NEXT : [1]: function offset=[[#%#.7x,C_ADDR + 4]]
+        .section        __TEXT,__text,regular,pure_instructions
+        .globl  _main
----------------
(alternatively you could omit it entirely, MC emits stuff into `.text` by default)


================
Comment at: lld/test/MachO/compact-unwind-folding-bug.s:24-27
+        pushq   %rbp
+        callq   _a
+        popq    %rbp
+        retq
----------------
can we remove more boilerplate? (likewise for the other functions)


================
Comment at: lld/test/MachO/compact-unwind-folding-bug.s:56-59
+        .section        __DATA,__objc_imageinfo,regular,no_dead_strip
+L_OBJC_IMAGE_INFO:
+        .long   0
+        .long   64
----------------
why do we need this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138320/new/

https://reviews.llvm.org/D138320