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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Thanks for tracking this down!!



================
Comment at: lld/MachO/UnwindInfoSection.cpp:182
   uint64_t level2PagesOffset = 0;
+
+  // The final ending address (this should covered folded entries as well)
----------------
nit: rm blank line


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


================
Comment at: lld/MachO/UnwindInfoSection.cpp:464
 
+  // Remember the ending boundary before we fold the entries.
+  finalEndingAddress = cuEntries[cuIndices.back()].functionAddress +
----------------
"remember" usually means "recall", rather than record/save.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:658
   // Level-1 sentinel
   const CompactUnwindEntry &cuEnd = cuEntries[cuIndices.back()];
+  iep->functionOffset = finalEndingAddress - in.header->addr;
----------------
this is now unused


================
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;
----------------
I'm not entirely sure why, but this is what ld64 does


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138320