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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked 2 inline comments as done.
oontvoo added inline comments.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:183
+
+  // The final ending address (this should covered folded entries as well)
+  uint64_t finalEndingAddress = 0;
----------------
int3 wrote:
> 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?
changed to `cueEndBoundary` ?


================
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;
----------------
int3 wrote:
> 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.
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)


================
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.
+
----------------
int3 wrote:
> 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.
How about just `compact-unwind-folding.s`? 
(added two additional test cases, where we behave slightly differently from LD64 ... LMK what you think)


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





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138320