[PATCH] D54483: [ELF] Use lower_bound to compute relative CU index

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: ELF/SyntheticSections.cpp:2430-2435
+      uint32_t I =
+          std::lower_bound(CUs.begin(), CUs.end(), Set.Offset,
+                           [](GdbIndexSection::CuEntry CU, uint32_t Offset) {
+                             return CU.CuOffset < Offset;
+                           }) -
+          CUs.begin();
----------------
I get where @ruiu is coming from in this being a bit harder to read - and if he'd prefer it to be a linear search (N^2, ultimately where N is the number of pub tables in the file - which would usually be small, except for LTO situations), that's OK.

I'd prefer a binary search - though perhaps there are more legible ways to write it, maybe having a nicer general algorithm, something like:

  auto I = llvm::BinarySearch(CUs, [](GDBIndexSection::CuEntry CU) {
    return CU.CuOffset == Offset ? 0 : CU.CuOffset < Offset ? -1 : 1;
  });
  if (I == CUs.end()) {
    /* invalid debug info, a pubnames seciton with a CU offset that's not the offset of any CU in the input */
  }
  ... *I ...

Might be nice - open to ideas on naming "BinarySearch" so that it's not confusing with other similar APIs, etc. Though I don't think it could be misused terribly easily - if someone mistook it for std::binary_search it'd only successfully compile if the object being searched for also happened to have an operator() that matched this usage, et.c


================
Comment at: test/ELF/gdb-index-multiple-cu.s:62
 
-# .debug_gnu_pubnames has just one set, associated with .Lcu_begin1 (CuIndex: 1)
+# The two sets of .debug_gnu_pubnames are deliberately swapped.
 .section .debug_gnu_pubnames,"", at progbits
----------------
Probably worth explaining why they're swapped - what this is intending to test for (that it's testing the case where the pubnames are not in the same order as the CUs), something like, perhaps:

  # Swap the pubname sections to test the case where pubnames are in a different order than the CUs they refer to


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54483