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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/locstats-bytes-overflow.yaml:24
+
+# CHECK: warning:  this field overflows
+
----------------
dblaikie wrote:
> 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?
Yep, that was my concern. It was going to stderr, but the JSON data goes to stdout.

>JSON supports the value being a string, I assume - so perhaps a string representation of ">= max int" or "overflowed" or something would be suitable?
Hmm, I agree... using some special value will make this output ready for the tools from outside. I'll update that.


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


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

https://reviews.llvm.org/D109217