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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 3 inline comments as done.
gkm 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) {
----------------
int3 wrote:
> 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?
Yes, equal or fewer.


================
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.
----------------
int3 wrote:
> 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`.
Aside from the crufty helper class `size_tx`, I thought ...
* The current approach has a simpler access procedure:
  # call `.lookup()` to receive the desired `.second` of the `DenseMap`'s pair
  # which we can compare with the default constructor `size_tx()` to detect "not found"
* With the iterator there are more steps:
   # call `.find()` on to return an iterator
   # compare iterator to `.end()` for "not found"
   # if found, dereference the iterator to return a pair
   # access `.second` to get the encoding-table index

I rewrote using `.find()` and it came-out nicer than anticipated, so I dropped `size_tx` and `.lookup()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93267