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

[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();
----------------
MaskRay wrote:
> 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)
Not sure I follow a few things

* Error checking seems necessary to me (but I know lld takes a different approach to error handling than I would - not sure how far that extends, whether only to invalid object files, or invalid data (like this DWARF) inside a valid object file)

* There are several things I meant to imply by the llvm::BinarySerach example - one, that it wouldn't take the needle parameter and pass it back into the comparator (this seemed to be Rui's cited complaint - and it is a bit awkward/unnecessary), returning end rather than the next element (seems more likely what most users want when they're searching for something in a list), and container (rather than begin/end) support

* stdlib.h/std::binary_search - yeah, neither look particularly helpful for this problem/making this code more readable

* Not sure I follow why any of the solutions discussed would cause/risk the CuIndex of subsequent object files to be incorrect


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54483