[PATCH] D93267: [lld-macho] Handle overflow beyond the 127 common encodings limit

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:197
+    page.entryCount = i - page.entryIndex;
+    if (i < cuPtrVector.size() &&
+        page.entryCount < UNWIND_INFO_REGULAR_SECOND_LEVEL_ENTRIES_MAX) {
----------------
gkm wrote:
> int3 wrote:
> > why does it matter if `i < cuPtrVector.size()`?
> When `i == cuPtrVector.size()`, the final page was filled using the compact format, so it is irrelevant to check if we might fit more entries using the regular format.
ah, and if the final page can be filled with the compact format, it will always end up using fewer bytes than if it had used the regular format, right?


================
Comment at: lld/MachO/UnwindInfoSection.cpp:217
       personalities.size() * sizeof(uint32_t) +
-      pageBounds.size() * sizeof(unwind_info_section_header_index_entry) +
+      (secondLevelPages.size() + 1) *
+          sizeof(unwind_info_section_header_index_entry) +
----------------
gkm wrote:
> int3 wrote:
> > why the `+ 1`?
> The first level index contains a sentinel.
would be nice to have a conment


================
Comment at: lld/MachO/UnwindInfoSection.h:73
 
+  struct secondLevelPage {
+    uint32_t kind;
----------------
valid lint


================
Comment at: lld/MachO/UnwindInfoSection.h:42-47
+// This is a wrapper class that is otherwise like size_t, but has a
+// default constructor that yields ~0 rather than 0. I need it so that
+// lookup() of absent keys from DenseMap<..., size_tx> return ~0,
+// since 0 is a valid value.
+// TODO: If someone knows nicer way to accomplish this, please advise.
+// A simpler alternative: bias all values by 1.
----------------
gkm wrote:
> int3 wrote:
> > why not use `find()` instead of `lookup()`?
> `find()` returns an iterator at key.
> `lookup()` returns the value at key, and is simpler for my purposes.
I don't understand how it is simpler, since it looks like you always have to check if the returned value is a sentinel value -- just as you'd have to check if `find()` had returned a valid iterator. But it seems that using `find()` would remove the need for `size_tx`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93267