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

[Vedant Kumar via llvm-commits]

vsk marked 5 inline comments as done.
vsk added a comment.

In https://reviews.llvm.org/D25295#582574, @vsk wrote:

> 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've asked around and haven't found a way around this. Part of the nature of undef is that two uses of a single undef may assume different values: "an ‘undef‘ “variable” can arbitrarily change its value over its “live range”" (http://llvm.org/docs/LangRef.html#undefined-values).

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

I tried again to write a plain-C test which gets compiled down to unconditional branches into the ubsan diagnostic routines, but haven't managed to do it. Here is my latest attempt: https://ghostbin.com/paste/wy2g2. The compiler just decides not to codegen the branch-on-undef in the way I like.

Do you have other suggestions for how to test this?

I've taken the rest of your suggestions into account in a local patch.



================
Comment at: lib/ubsan/ubsan_handlers.cc:112
+      (SIntMax(1) << (L.getType().getIntegerBitWidth() - 1)) - 1; //< U
+  SIntMax LowerLimit = -UpperLimit - 1;                           //< L
+
----------------
vsk wrote:
> 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?
It seems like these comments aren't adding anything, so I've removed them locally.


https://reviews.llvm.org/D25295