[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 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:
> 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
Sorry I didn't notice that `llvm::BinarySearch` does not take the needle parameter. Yeah it indeed looks better than `std::binary_search`!

> (this seemed to be Rui's cited complaint - and it is a bit awkward/unnecessary)

`std::binary_search` has reserved the third parameter as the needle with a generic type. As it cannot overload on that parameter to express a comparator, when both the needle and the comparator appear, it does look a bit redundant when we pass a lambda as a comparator.

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

That bug has been fixed and is no longer relevant. These revisions are just polishing the interface and fixing some corner cases that people may not notice. They are good from a quality of implementation view, though.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54483