[PATCH] D54483: [ELF] .gdb_index: use lower_bound to compute relative CU index in an object file

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay 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();
----------------
dblaikie wrote:
> 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
Yes, the layout `clang-format` uses is not pretty. I don't know if additional error checking is worth the few more lines of code, as gold only handles the first set of `.debug_gnu_pubnames`. I'm not sure if the implementer has noticed that, but I won't be surprised if they don't as debug symbol accelerating in the partially linked object files does not seem very noticeable.

As to `llvm::BinarySearch`, I'm unclear if the `end()`-returning behavior when there is no element equal to the needle is useful. `stdlib.h bsearch` and `std::binary_search` look mostly useless to me.

It is important to get `CuIndex` of subsequent object files correct (that was the main merit of D54361)


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54483