[PATCH] D109217: [llvm-dwarfdump] Fix unsigned overflow when calculating stats

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

Orlando added a comment.

It seems like the down side of using asserts to detect the "overflow" (the patch's current approach) is that release-config users may still get misleading stats due to wrapping. Implementing a saturating int in and of itself doesn't seem like a full solution, since a user may not notice that the stat is the saturated value, or even know that it's special, especially if the stats are consumed by another tool.

IMO when the stat cannot be computed properly - however the detection is implemented, either with saturating ints, checks like the ones in the asserts, or something else - a good solution for users would be to print a message, and either skip printing the "bad" stats or all of them. That would be consistent for all build configurations and avoid hiding the issue. What do you think? Sorry if I'm just stating the obvious!



================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:760
   /// version.
-  unsigned Version = 8;
-  unsigned VarParamTotal = 0;
-  unsigned VarParamUnique = 0;
-  unsigned VarParamWithLoc = 0;
-  unsigned NumFunctions = 0;
-  unsigned NumInlinedFunctions = 0;
-  unsigned NumFuncsWithSrcLoc = 0;
-  unsigned NumAbstractOrigins = 0;
-  unsigned ParamTotal = 0;
-  unsigned ParamWithType = 0;
-  unsigned ParamWithLoc = 0;
-  unsigned ParamWithSrcLoc = 0;
-  unsigned LocalVarTotal = 0;
-  unsigned LocalVarWithType = 0;
-  unsigned LocalVarWithSrcLoc = 0;
-  unsigned LocalVarWithLoc = 0;
+  unsigned Version = 9;
+  uint64_t VarParamTotal = 0;
----------------
djtodoro wrote:
> Orlando wrote:
> > I'm not sure how much this matters but looking at the comment above I don't think a version bump is necessary for this patch? Any input that didn't trip the assertions previously will still have the same output with the patch applied.
> I think since this is a bug fix we should bump it -- e.g., a stat number could have been 0 (as a consequence of the bug), and now it will be 2^32 for example, right? I think that this is the purpose behind the stats version.
Sounds reasonable (I wasn't thinking about release mode when I made that comment).


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

https://reviews.llvm.org/D109217