[PATCH] D109217: [llvm-dwarfdump] Fix unsigned overflow when calculating stats
Djordje Todorovic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 20 08:15:17 PDT 2021
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
More information about the llvm-commits
mailing list