[PATCH] D25295: [ubsan] Handle undef values in the integer overflow diagnostic

[Filipe Cabecinhas via llvm-commits]

filcab requested changes to this revision.
filcab added a subscriber: kcc.
filcab added a comment.
This revision now requires changes to proceed.

I really don't like that the test is reimplementing part of the instrumentation.
I know a test like this will probably be a bit fragile, as we will be relying on some optimizations (to make the undefs appear).

The original test (the one in the phab summary) works, and gets reduced to a call to the reported function on `-O2 -fsanitize=undefined`. It looks safe enough to use tests like that (With some comments so people understand what we're doing there if the tests break for some reason).
@kcc might have a different opinion, so I'd like him to chime in.



================
Comment at: lib/ubsan/ubsan_handlers.cc:105
 
+/// \brief Check if a signed integer arithmetic operation will overflow.
+static bool checkForSignedOverflow(const Value &L, const char *Operator,
----------------
No need for `\brief`


================
Comment at: lib/ubsan/ubsan_handlers.cc:111
+  SIntMax UpperLimit =
+      (SIntMax(1) << (L.getType().getIntegerBitWidth() - 1)) - 1; //< U
+  SIntMax LowerLimit = -UpperLimit - 1;                           //< L
----------------
Signed integer overflow triggers undefined behavior.


================
Comment at: lib/ubsan/ubsan_handlers.cc:112
+      (SIntMax(1) << (L.getType().getIntegerBitWidth() - 1)) - 1; //< U
+  SIntMax LowerLimit = -UpperLimit - 1;                           //< L
+
----------------
What do these comments mean? You seem to want to "name" some of the variables, but if I got that correctly, you ended up with two variables which you refer to as `L`.
Having names depend on the values (`Ln` vs `Lp`) doesn't seem to help either.


================
Comment at: lib/ubsan/ubsan_handlers.cc:135
+    else if (LHS > 0 && RHS < 0)
+      return LHS > (LowerLimit / RHS); // Lp * Rn < L => Lp > L/Rn
+    else if (LHS < 0 && RHS > 0)
----------------
Won't `LowerLimit / -1` overflow?


https://reviews.llvm.org/D25295