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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 14 inline comments as done.
gkm added inline comments.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:142-146
               if (a.second == b.second)
                 // When frequencies match, secondarily sort on encoding
                 // to maintain parity with validate-unwind-info.py
                 return a.first > b.first;
               return a.second > b.second;
----------------
int3 wrote:
> I know this is existing code, but isn't this the default behavior of `std::pair::operator>()`?
No. `std::pair::operator>()` collates primarily on `.first` and falls back on `.second`. Here, we need `.second` as the primary and `.first` as the fallback.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:149
 
-  // Split folded encodings into pages, limited by capacity of a page
-  // and the 24-bit range of function offset
-  //
-  // Record the page splits as a vector of iterators on cuPtrVector
-  // such that successive elements form a semi-open interval. E.g.,
-  // page X's bounds are thus: [ pageBounds[X] .. pageBounds[X+1] )
-  //
-  // Note that pageBounds.size() is one greater than the number of
-  // pages, and pageBounds.back() holds the sentinel cuPtrVector.cend()
-  pageBounds.push_back(cuPtrVector.cbegin());
-  // TODO(gkm): cut 1st page entries short to accommodate section headers ???
-  CompactUnwindEntry64 cuEntryKey;
-  for (size_t i = 0;;) {
-    // Limit the search to entries that can fit within a 4 KiB page.
-    const auto pageBegin = pageBounds[0] + i;
-    const auto pageMax =
-        pageBounds[0] +
-        std::min(i + UNWIND_INFO_COMPRESSED_SECOND_LEVEL_ENTRIES_MAX,
-                 cuPtrVector.size());
-    // Exclude entries with functionOffset that would overflow 24 bits
-    cuEntryKey.functionAddress = (*pageBegin)->functionAddress +
-                                 UNWIND_INFO_COMPRESSED_ENTRY_FUNC_OFFSET_MASK;
-    const auto pageBreak = std::lower_bound(
-        pageBegin, pageMax, &cuEntryKey,
-        [](const CompactUnwindEntry64 *a, const CompactUnwindEntry64 *b) {
-          return a->functionAddress < b->functionAddress;
-        });
-    pageBounds.push_back(pageBreak);
-    if (pageBreak == cuPtrVector.cend())
-      break;
-    i = pageBreak - cuPtrVector.cbegin();
+  // Truncate the vector to 127 elements.
+  // Common encoding indexes are limited to 0..126, while enconding
----------------
int3 wrote:
> depending on how large the vector is, might be a useful perf optimization to use quickselect to find the 127th largest element and just sort over those smaller than it... but we can consider that later
This is good thinkin'! The Chrome bug that motivated this diff has inputs with approx 380 common encodings. IMO, filtering with quickselect (1) offers an insignificant micro-efficiency, and (2) is possibly more expensive than sort alone.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:187
+        bytesRemaining -= sizeof(uint32_t);
+      } else if (bytesRemaining >= 2 * sizeof(uint32_t) &&
+                 n < UNWIND_INFO_COMPACT_ENCODINGS_MAX) {
----------------
int3 wrote:
> `2 * sizeof(uint32_t)` is due to the size of `unwind_info_regular_second_level_entry`, right? how about using `sizeof(unwind_info_regular_second_level_entry)` instead?
This is a valid comment for handling the regular encoding format a bit further down in the function. Here, we are still handling compact encoding, and specifically the case where we need to extend the local encoding table by 1 word, and the compact entry list by 1 word.


================
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:
> 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.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:204-205
+      page.padByteCount =
+          (UNWIND_INFO_REGULAR_SECOND_LEVEL_ENTRIES_MAX - page.entryCount) * 2 *
+          sizeof(uint32_t);
+    } else {
----------------
s/`2 * 2 * sizeof(uint32_t)`/`sizeof(unwind_info_regular_second_level_entry)`/ is appropriate here.


================
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) +
----------------
int3 wrote:
> why the `+ 1`?
The first level index contains a sentinel.


================
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:
> 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.


================
Comment at: lld/MachO/UnwindInfoSection.h:97-112
 #define UNWIND_INFO_COMMON_ENCODINGS_MAX 127
+#define UNWIND_INFO_COMPACT_ENCODINGS_MAX 256
 
 #define UNWIND_INFO_SECOND_LEVEL_PAGE_SIZE 4096
 #define UNWIND_INFO_REGULAR_SECOND_LEVEL_ENTRIES_MAX                           \
   ((UNWIND_INFO_SECOND_LEVEL_PAGE_SIZE -                                       \
     sizeof(unwind_info_regular_second_level_page_header)) /                    \
----------------
int3 wrote:
> general aesthetic suggestion: if we moved this into the .cpp file, we could do away with all the `UNWIND_INFO_` prefixes and reduce some of the (extensive) line wrapping. Probably better in a separate diff though
I did this here. Vertical space is reduced by an underwhelming 2 lines, but horizontal line density is also lower, so on balance it seems an improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93267