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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/locstats-bytes-overflow.yaml:24
+
+# CHECK: warning:  this field overflows
+
----------------
I think it'd be worth CHECKing the specific/full syntax, rather than just "this warning text appears somewhere in the output" - since we're specifically putting it in the output in a particular place.

Hmm, that raises a question: is this warning going to stderr, but the actual stats output is to stdout? If so then I think that's a different problem. Maybe that answers one of my other questions though when I suggested printing the output as "> max int" - https://reviews.llvm.org/D109217#3012175 - is that what you meant in this comment? That the value in the JSON data doesn't have any mention of the warning/overflow, and only has the saturated integer value?

I worry that's error-prone though, since the value is incorrect and a tool might not be aware of that. So I think it may be valuable to ensure we don't encode a valid value/something that could be mistaken for a valid value in the field when it's overflowed?

JSON supports the value being a string, I assume - so perhaps a string representation of ">= max int" or "overflowed" or something would be suitable?


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:51-56
+    if (Value != UINT64_MAX) {
+      if (Value < UINT64_MAX - 1)
+        ++Value;
+      else
+        Value = UINT64_MAX;
+    }
----------------
Maybe just implement this in terms of the other:



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

https://reviews.llvm.org/D109217