[PATCH] D113582: [lld-macho] Support renaming of LSDA section

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:132
+  // in symbolsVec.
+  std::vector<CompactUnwindEntry<Ptr>> cuVector;
+  // Indices into the compact unwind vector.
----------------
oontvoo wrote:
> for consistency, can we stick to one naming convention? :) (hungarian notation or not?)
> does `cuEntries`work?
this is an existing name, but yes, happy to change it. cuEntries sounds good.


================
Comment at: lld/MachO/UnwindInfoSection.cpp:134
+  // Indices into the compact unwind vector.
+  std::vector<size_t> cuIdxVector;
   // Indices of personality functions within the GOT.
----------------
oontvoo wrote:
> comment on what the significance of the entries pointed to by these indices is?
> 
> 
> P.S: from reading further, it's an indirection from the page's index -> the cuVector's index .... would be good to have clarification
there's a further comment on line 406. (It's not just an indirection from the page's index... it's an indirection used for pretty much all accesses into the cuVector.)


================
Comment at: lld/MachO/UnwindInfoSection.cpp:405-406
 
   // Rather than sort & fold the 32-byte entries directly, we create a
-  // vector of pointers to entries and sort & fold that instead.
-  cuPtrVector.reserve(cuVector.size());
-  for (CompactUnwindEntry<Ptr> &cuEntry : cuVector)
-    cuPtrVector.emplace_back(&cuEntry);
-  llvm::sort(cuPtrVector, [](const CompactUnwindEntry<Ptr> *a,
-                             const CompactUnwindEntry<Ptr> *b) {
-    return a->functionAddress < b->functionAddress;
+  // vector of indices to entries and sort & fold that instead.
+  cuIdxVector.resize(cuVector.size());
----------------
this is the comment I was referring to


================
Comment at: lld/MachO/UnwindInfoSection.cpp:552
+                      entriesWithLsda.size() *
+                          sizeof(unwind_info_section_header_lsda_index_entry);
   unwindInfoSize =
----------------
oontvoo wrote:
> shouldn't this be "size_t"? (because entriesWithLsda is storing the indices not the entries themselves?)
level2PagesOffset is the offset of the encoded values, not the intermediate structures :) so we're calculating the offsets after encoding here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113582