[PATCH] D85636: [llvm-dwarfdump] Fix misleading scope byte coverage statistics

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:296-312
         for (auto Entry : *Loc) {
-          uint64_t BytesEntryCovered = Entry.Range->HighPC - Entry.Range->LowPC;
-          BytesCovered += BytesEntryCovered;
+          TotalBytesCovered += Entry.Range->HighPC - Entry.Range->LowPC;
+          uint64_t ScopeBytesCoveredByEntry = 0;
+          // Calculate how many bytes of the parent scope this entry covers.
+          // FIXME: In section 2.6.2 of the DWARFv5 spec it says that "The
+          // address ranges defined by the bounded location descriptions of a
+          // location list may overlap". So in theory a variable can have
----------------
dblaikie wrote:
> Little concerned about the N^2 complexity of this approach, and wondering if an alternative approach might also address the limitation regarding overlapping locations.
> 
> Start with a range of intervals (without overlaps) for the enclosing scope. Then intersect any location ranges with that collection of intervals and store those in another non-overlapping interval range.
> 
> A small data structure that lets you build a non-overlapping list of intervals (merging intervals, etc), keeping them sorted - then it'll be easier to find other overlapping regions.
> 
> eg:
> ```
> Intervals Scope;
> for (address ranges in scope)
>   Scope.insert(R);
> Intervals Loc;
> for (address ranges in location) {
>   R' = Scope.intersection(R);
>   Loc.insert(R'); 
> }
> scope bytes covered = Loc.total_coverage/size/something
> ```
> 
> But, yeah, maybe the implementation of that's more complicated than is justified for this for now.
Yeah, that does look like a much better solution. If we want to go in that direction I might need to come back to this after some higher priority work (though I'd be happy to do so!).

A middle ground option might be to use `IntervalMap`. It's not perfect for this case though because it maps intervals to values, and only coalesces intervals if they have the same value. We don't have 'values' to map here - we only care about the intervals themselves. So we'd have to fudge it slightly by giving every interval some constant value (e.g. 0). I'm not really a fan of this idea, but I thought it'd be at least worth bringing up.


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

https://reviews.llvm.org/D85636