[PATCH] Catch more cases when diagnosing integer-constant-expression overflows.

[Richard Smith]

================
Comment at: lib/AST/ExprConstant.cpp:6837-6839
@@ -6836,2 +6836,5 @@
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
-  if (E->isAssignmentOp())
+  // Only bail out early if keepEvaluatingAfterFailure()
+  // is false, otherwise keep evaluating because the evaluator could identify
+  // overflow in subexpressions.
+  if (!Info.keepEvaluatingAfterFailure() && E->isAssignmentOp())
----------------
This comment seems redundant: it's obvious why `keepEvaluatingAfterFailure` should cause us to keep evaluating after a failure ;-)

================
Comment at: lib/AST/ExprConstant.cpp:6852-6853
@@ -6848,6 +6851,4 @@
     bool LHSOK;
-    if (E->getLHS()->getType()->isRealFloatingType()) {
-      LHSOK = EvaluateFloat(E->getLHS(), LHS.FloatReal, Info);
-      if (LHSOK) {
-        LHS.makeComplexFloat();
-        LHS.FloatImag = APFloat(LHS.FloatReal.getSemantics());
+    if (!E->isAssignmentOp()) {
+      if (LHSTy->isRealFloatingType()) {
+        LHSOK = EvaluateFloat(E->getLHS(), LHS.FloatReal, Info);
----------------
Unnecessary nesting here, which makes the code harder to read; switch to

    if (E->isAssignmentOp()) {
      // handle it
    } else if (LHSTy->isRealFloatingType()) {
      // as currently

================
Comment at: lib/AST/ExprConstant.cpp:6859-6864
@@ +6858,8 @@
+        }
+      } else if (LHSTy->isIntegerType()) {
+        LHSOK = EvaluateInteger(E->getLHS(), LHS.IntReal, Info);
+        if (LHSOK) {
+          LHS.makeComplexInt();
+          LHS.IntImag = APSInt(LHS.IntReal.getBitWidth(), !LHS.IntReal.isSigned());
+        }
+      } else {
----------------
These changes look unrelated to your patch: we only get here for a non-assignment-op; you're only changing how we handle assignments.

================
Comment at: lib/AST/ExprConstant.cpp:6869
@@ +6868,3 @@
+    } else
+      LHSOK = true;
+
----------------
You should evaluate the LHS as an lvalue in this case; there could be integer overflow within it. Eg, `arr[A * B] = 3i;`

================
Comment at: lib/AST/ExprConstant.cpp:6874-6885
@@ -6860,9 +6873,14 @@
 
-    if (E->getRHS()->getType()->isRealFloatingType()) {
+    if (RHSTy->isRealFloatingType()) {
       if (!EvaluateFloat(E->getRHS(), RHS.FloatReal, Info) || !LHSOK)
         return false;
       RHS.makeComplexFloat();
       RHS.FloatImag = APFloat(RHS.FloatReal.getSemantics());
+    } else if (RHSTy->isIntegerType()) {
+      if (!EvaluateInteger(E->getLHS(), LHS.IntReal, Info) || !LHSOK)
+        return false;
+      RHS.makeComplexInt();
+      RHS.IntImag = APSInt(RHS.IntReal.getBitWidth(), !RHS.IntReal.isSigned());
     } else if (!EvaluateComplex(E->getRHS(), RHS, Info) || !LHSOK)
       return false;
 
----------------
Please factor out this process rather than duplicating it here and above.

================
Comment at: lib/AST/ExprConstant.cpp:6887-6916
@@ -6868,32 +6886,32 @@
 
     if (LHS.isComplexFloat()) {
       APFloat::cmpResult CR_r =
         LHS.getComplexFloatReal().compare(RHS.getComplexFloatReal());
       APFloat::cmpResult CR_i =
         LHS.getComplexFloatImag().compare(RHS.getComplexFloatImag());
 
       if (E->getOpcode() == BO_EQ)
         return Success((CR_r == APFloat::cmpEqual &&
                         CR_i == APFloat::cmpEqual), E);
       else {
         assert(E->getOpcode() == BO_NE &&
                "Invalid complex comparison.");
         return Success(((CR_r == APFloat::cmpGreaterThan ||
                          CR_r == APFloat::cmpLessThan ||
                          CR_r == APFloat::cmpUnordered) ||
                         (CR_i == APFloat::cmpGreaterThan ||
                          CR_i == APFloat::cmpLessThan ||
                          CR_i == APFloat::cmpUnordered)), E);
       }
     } else {
       if (E->getOpcode() == BO_EQ)
         return Success((LHS.getComplexIntReal() == RHS.getComplexIntReal() &&
                         LHS.getComplexIntImag() == RHS.getComplexIntImag()), E);
       else {
         assert(E->getOpcode() == BO_NE &&
                "Invalid compex comparison.");
         return Success((LHS.getComplexIntReal() != RHS.getComplexIntReal() ||
                         LHS.getComplexIntImag() != RHS.getComplexIntImag()), E);
       }
     }
   }
----------------
This code certainly won't do the right thing if we reach it for an assignment operator. Should you instead be setting `LHSOK` to `false` for an assignment op?

================
Comment at: lib/Sema/SemaChecking.cpp:7028-7039
@@ -7027,4 +7027,14 @@
 void Sema::CheckForIntOverflow (Expr *E) {
-  if (isa<BinaryOperator>(E->IgnoreParenCasts()))
+   // Evaluate each argument of a CallExpr to check
+   // for overflow and ignore both paren and cast expressions.
+   // FIXME: It should be possible to pass any/all Expressions to 
+   // EvaluateForOverflow() and have it do the right thing.  Currently, it only
+   // handles BinaryOperator expressions.
+
+  if (CallExpr *CE = dyn_cast<CallExpr>(E->IgnoreParenCasts())) {
+    for (CallExpr::arg_iterator CEI = CE->arg_begin(),
+         CEE = CE->arg_end(); CEI != CEE; ++CEI) 
+      CheckForIntOverflow(*CEI);
+  } else if (isa<BinaryOperator>(E->IgnoreParenCasts()))
     E->IgnoreParenCasts()->EvaluateForOverflow(Context);
 }
----------------
The original design for the checker was that it would recursively evaluate the entire expression, including all subexpressions. There is no principled reason for it to only apply it when the top-level expression is a `BinaryOperator` (or a `CallExpr`); IIRC that restriction was applied over (possibly-misplaced) performance concerns. So: I'd like to see some performance measurements showing that either we can, or we cannot, simply run the checker over all expressions. Let's not make an arbitrary restriction more complex for no reason...

Also, this appears orthogonal to the other changes in the patch; it would be better to separate it, especially since it has performance implications.

http://reviews.llvm.org/D1238

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/