[PATCH] D54361: [ELF] .gdb_index: fix CuOff when a .debug_info section contains more than 1 CU

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lld/trunk/ELF/SyntheticSections.cpp:2432
+      //
+      // We assume both CUs[*].CuOff and Set.Offset are increasing.
+      while (I < CUs.size() && CUs[I].CuOffset < Set.Offset)
----------------
MaskRay wrote:
> dblaikie wrote:
> > Could you fix this? This assumption seems pretty subtle/likely to break valid DWARF in surprising/hard to diagnose ways.
> > 
> > Moving the declaration of I into the for(Set) loop (ie: putting it just before this while loop) should suffice - and a test case with 2 pubnames in the reverse order to CUs would demonstrate the fix?
> > 
> > (would be nice to do a binary search rather than a linear one on the CU offsets, since they are naturally sorted by offset)
> > 
> > 
> I used `std::lower_bound(CUs.begin(), CUs.end(), ...) - CUs.begin())` in a former version but  @ruiu thought it was too complicated...
*nod* std::lower_bound is a somewhat awkward API, but the general goal of using a binary search still seems achievable with readable/not-so-awkward code.

Having a convenient binary search API that takes a lambda/functor without also taking the comparison object, instead relying on the functor to have that parameter hardcoded/captured, seems useful. Not sure what to name it (naming it llvm::binary_search would be likely to get mixed up with the interface to std::binary_search, which has the similar API quirks to std::lower_bound, unfortunately) though.


Repository:
  rL LLVM

https://reviews.llvm.org/D54361