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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/stats-scope-bytes-covered.yaml:17-23
+##     DW_TAG_lexical_block
+##       DW_AT_low_pc	(0x0000000000000005)
+##       DW_AT_high_pc	(0x000000000000000a)
+##
+##       DW_TAG_variable
+##         DW_AT_location	(0x00000000:
+##            [0x0000000000000000, 0x0000000000000005): DW_OP_reg5 RDI)
----------------
dblaikie wrote:
> Might be worth making this test case a bit more interesting by having /some/ overlap (and some non-overlap) regions.
Ping on this.


================
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
----------------
Orlando wrote:
> 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.
Yeah, I looked at/considered `IntervalMap` but I'm not sure it provides an easy way to get the intersection of one interval with any number of intervals in the map, for instance - or intersectionally reassign the value (if it could do that (eg: given a map with `[1, 5) -> 0, [7, 10) -> 0` and an interval `[3, 8) -> 1`, it could produce `[1, 3) -> 0, [3, 5) -> 1, [7, 8) -> 1, [8, 10) -> 0`, then you could use it to do this work & do a linear scan over the map summing up all ranges that -> 1. Maybe it's viable to add that functionality to IntervalMap, I'm not sure.)

But I'm OK with that being a follow-up/future work.


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

https://reviews.llvm.org/D85636