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

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 28 15:35:57 PDT 2016


vsk added a comment.

In https://reviews.llvm.org/D25295#582541, @filcab wrote:

> I really don't like that the test is reimplementing part of the instrumentation.


OK. Then we should see if we can do anything about llvm.sadd.with.overflow() returning different values at -O0/-O3 when its operands are %undef. I'll ask around.

> 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).

Right. I have tried changing together e.g multiple calls to getUndef() in one program, but the optimizer throws it away. We may be able to play some tricks with noinline to make this work.

> 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.

Right, I can try this approach again.



================
Comment at: lib/ubsan/ubsan_handlers.cc:111
+  SIntMax UpperLimit =
+      (SIntMax(1) << (L.getType().getIntegerBitWidth() - 1)) - 1; //< U
+  SIntMax LowerLimit = -UpperLimit - 1;                           //< L
----------------
filcab wrote:
> Signed integer overflow triggers undefined behavior.
Is your concern that getIntegerBitWidth() could return something that exceeds the bit-width of SIntMax?


================
Comment at: lib/ubsan/ubsan_handlers.cc:112
+      (SIntMax(1) << (L.getType().getIntegerBitWidth() - 1)) - 1; //< U
+  SIntMax LowerLimit = -UpperLimit - 1;                           //< L
+
----------------
filcab wrote:
> 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.
I'm trying to attach labels to "LHS" and "RHS" based on whether or not they are negative. Would it help to state explicitly that the 'n' suffix denotes that the variable is negative, or should the comments just be dropped?


================
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)
----------------
filcab wrote:
> Won't `LowerLimit / -1` overflow?
Yes, this is a bug. The check should be for RHS < LowerLimit/LHS.


https://reviews.llvm.org/D25295





More information about the llvm-commits mailing list