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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:71
+    }
+    assert(Value != UINT64_MAX && "Stat field overflow");
+  }
----------------
dblaikie wrote:
> Personally, I think once we've defined the behavior on overflow, we shouldn't assert that overflow doesn't happen - this undermines the concept of having defined behavior on overflow (& makes it somewhat harder to test - since that behavior can now only be tested in a non-asserts build (well, I guess since assert isn't UB-if-false, and the assert is after the warning, that's not the case, but it's a bit subtle)).
> 
> Also, might it make more sense to do the warning/etc on the final use/printing out of the statistic instead? (I guess that's difficult because some statistics are derived from others? - so catching it the moment it overflows means it'll always be diagnosed only once, rather than multiple times due to multiple uses?)
Thanks for the suggestions. I totally agree.

I guess that it makes sense to report the warning when printing the overflowed value, since we can point to the specific field.


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

https://reviews.llvm.org/D109217