[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

Wed Aug 3 05:58:20 PDT 2022

labath added a comment.

In D130796#3692243 <https://reviews.llvm.org/D130796#3692243>, @zequanwu wrote:

> In D130796#3691227 <https://reviews.llvm.org/D130796#3691227>, @zequanwu wrote:
>
>>> If you find yourself needing to do extra work to work its limitations, we should fix that algorithm instead.
>>
>> That makes sense. I'll work on fixing `RangeVectorData`.
>
> After thinking it again, not just `RangeVectorData` need to be written but also `RangeVector`, although they are similar. It requires rewrite the whole file `RangeMap.h` and all other places uses its APIs.

Why is that? I don't think we need to change the API -- just the way it is implemented. And the RangeVectorData class already contains the upper_bound argument, which should make it possible to find the element containing a value quickly <https://en.wikipedia.org/wiki/Interval_tree#Java_example:_Searching_a_point_or_an_interval_in_the_tree>. That basically means taking the algorithm in FindEntryIndexesThatContain, but making it stop at the first element found.

And you can just ignore the RangeVector class. That problem (two separate classes) can be handled separately and holistically.

Alternatively, if we don't want to/need to support overlaps, we could switch to some completely different class, which implements searches correctly. llvm::IntervalMap, for example.

> Can we just use `RangeVectorData` as it is right now and fix it later? So, this patch is not blocked. Otherwise, this local variables functionality is almost unusable for NativePDB plugin when multiple ranges exist.

Maybe.. However I don't want to end up with  two broken overlap-detection algorithms (see inline comment) instead of one good one.

================
Comment at: lldb/source/Expression/DWARFExpressionList.cpp:37-39
+  if (m_exprs.FindEntryThatContains(base) ||
+      m_exprs.FindEntryThatContains(end - 1))
+    return false;
----------------
zequanwu wrote:
> labath wrote:
> > What is this attempting to check? That the list does not contain an overlapping entry? That hardly seems like a correct algorithm...
> `RangeDataVector::Append` just append the new entry to the end of vector. It has no idea if overlaps happens or not. 
I get that. My point is that this is not the right way to check for overlaps. It won't catch `(1,10)` and `(4,6)` for instance (depending on the order in which you insert them).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130796/new/

https://reviews.llvm.org/D130796