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

[Vedant Kumar via llvm-commits]

vsk added a comment.

> Why is the test a bc file instead of the plain ll file?

llvm has a bitcode backwards compatibility policy. Checking in bitcode means that people can change the textual IR without breaking this test (e.g when the typeless pointer work lands). We should also check in the textual IR used to produce the bitcode so that people have an easy time updating the test in the future.

> Better yet: Can you get a reproducible for this bug in C/C++?

I can reproduce this issue several ways with my system compiler. The problem is that we can't guarantee that all versions of all compilers which compile a test case in C will treat overflow UB in the same way. That's why I thought it'd be better to check in a bitcode test which unambiguously demonstrates the issue.

Fwiw, here is the original program I tried:

  // Compile at -fsanitize=integer -O2
  
  struct Undef {
    int I;
  }
  
  int getUndefInt() {
    Undef U;
    return U.I; //< Read of uninitialized memory.
  }
  
  int main() { return getUndefInt() + getUndefInt(); }



================
Comment at: lib/ubsan/ubsan_handlers.cc:115
+  default:
+    return 0;
+  }
----------------
filcab wrote:
> Don't make errors disappear like this. Add an `UNREACHABLE()` to the default case.
I will try and think of a better way to do this. As it stands, the problem is that the default case isn't unreachable. If e.g the optimizer decides to pass in (0, 0) into the overflow handler instead of (INT_MAX, INT_MAX), we would hit it.


================
Comment at: lib/ubsan/ubsan_handlers.cc:123
+  if (IsSigned) {
+    SIntMax V = evaluateOperation(L.getSIntValue(), Operator, R.getSIntValue());
+    auto UpperLimit =
----------------
filcab wrote:
> Won't you overflow if the values would overflow (for the case where your values are the same type as `SIntMax`)?
Yes, I brought this up in my last comment as something I didn't have a good solution for. I think I have a better approach now that doesn't actually necessitate evaluating the arithmetic op. I'll update the patch soon!


================
Comment at: lib/ubsan/ubsan_handlers.cc:128
+    if (V > UpperLimit || V < LowerLimit)
+      return true;
+  } else {
----------------
filcab wrote:
> `return V > UpperLimit || V < LowerLimit;`
> Then remove the indentation level for the `else`, make the same transformation, and remove the `return false;` at the end.
Good point; but I'd rather rework the overflow check.


https://reviews.llvm.org/D25295