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

[Jez Ng via Phabricator via llvm-commits]

int3 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;
----------------
oontvoo wrote:
> 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` ?
sounds good!


================
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.
+
----------------
oontvoo wrote:
> 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)
sgtm!


================
Comment at: lld/test/MachO/compact-unwind-foldings.s:7
+
+# 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
----------------



================
Comment at: lld/test/MachO/compact-unwind-foldings.s:59-60
+
+// _d should NOT have an entry in .eh_frame
+// So it shouldn't be covered by the unwind table.
+        .section        __TEXT,__text,regular,no_dead_strip
----------------
this makes sense to me, and I think we can keep our deviation from ld64's behavior here


================
Comment at: lld/test/MachO/compact-unwind-foldings.s:61
+// So it shouldn't be covered by the unwind table.
+        .section        __TEXT,__text,regular,no_dead_strip
+_d:
----------------
is this directive necessary?


================
Comment at: lld/test/MachO/compact-unwind-foldings.s:85-86
+
+// _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.
----------------
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


================
Comment at: lld/test/MachO/compact-unwind-foldings.s:88
+// So it shouldn't be covered by the unwind table.
+        .section        __TEXT,__text,regular,no_dead_strip
+_d:
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138320