[PATCH] D69027: [llvm-dwarfdump][Statistics] Fix calculation of OffsetToFirstDefinition

[Kristina Bessonova via Phabricator via llvm-commits]

krisb added a comment.

In D69027#1718400 <https://reviews.llvm.org/D69027#1718400>, @avl wrote:

> Speaking of variable's enclosing scope: it is necessary to take into account not only OffsetFromFirstDefinition, which points into the start of the variable's enclosing scope. The end of the variable's address ranges should also be encountered.


I have my patch based on two assumptions:

1. We only care about lexically (or statically) scoped languages. Otherwise detecting a variable's scope is very problematic.
2. In a lexically scoped language the end of a variable's scope should match the end of the enclosing lexical scope.

That's why I a bit confused about why would we need to make some adjustments to the end of a range. Could you kindly clarify what did you mean?

In D69027#1718400 <https://reviews.llvm.org/D69027#1718400>, @avl wrote:

> So it is probably OK to have coverage for variable against overall scope(which allows us to see above problem) and to see coverage for variable against it`s enclosing scope, assuming start and end of address ranges of the variable are correct.


If the other metric is valuable, I suggest implementing it in a separate patch. But could you give an example when calculating coverage against full scope provides additional knowledge which does not follow from currently existing metrics?



================
Comment at: tools/llvm-dwarfdump/Statistics.cpp:297
             uint64_t FirstDef = List->Entries[0].Begin;
-            uint64_t UnitOfs = UnitLowPC; 
-            // Ranges sometimes start before the lexical scope.
-            if (UnitOfs + FirstDef >= ScopeLowPC)
-              OffsetToFirstDefinition = UnitOfs + FirstDef - ScopeLowPC;
-            // Or even after it. Count that as a failure.
-            if (OffsetToFirstDefinition > BytesInScope)
-              OffsetToFirstDefinition = 0;
+            BytesInScope = adjustScopeToFirstDef(BytesInScope, FirstDef,
+                                                 ScopeDie, UnitLowPC);
----------------
avl wrote:
> adjustScopeToFirstDef() returns BytesInScope with already subtracted OffsetToFirstDefinition. That BytesInScope value later passed into collectLocStats(). Previously it was passed without subtracting OffsetToFirstDefinition. Is that correct?
>From my point of view, yes, that's correct. 
I think, we should either apply OffsetToFirstDefinition every time we calculating any 'coverage' metrics or do not apply it at all. Or as another option - explicitly say which metrics are calculated against adjusted scope, and which against the full one. Currently, we have a mix, which hardens interpreting the statistics. 
In that patch, I chose the first option and applied OffsetToFirstDefinition everywhere.  


================
Comment at: tools/llvm-dwarfdump/Statistics.cpp:532
   /// version.
   unsigned Version = 3;
   unsigned VarParamTotal = 0;
----------------
aprantl wrote:
> Since this is going to change the results in an incomparable way, can you please also bump the version number?
Forgot about this. Done, thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69027