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

[Josh Magee]

================
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 {
----------------
rsmith wrote:
> These changes look unrelated to your patch: we only get here for a non-assignment-op; you're only changing how we handle assignments.
Hmm... Quite right; I'll remove this (and the similar treatment for the RHS).

================
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);
       }
     }
   }
----------------
rsmith wrote:
> 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?
Yes, you are absolutely correct.  I am surprised that I managed to run the regression test without something blowing up from this.  Setting LHSOK to false seems like the best thing to do.  (Alternatively I could handle assignment in the above, but in that case I would just return false, so setting LHSOK seems more natural.)

================
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);
 }
----------------
rsmith wrote:
> 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.
Gotcha - I'll drop these changes.  I haven't done any performance measurements here yet, so I agree with holding off on fiddling with the restrictions until the impact of running the checker over all expressions is better understood.

http://reviews.llvm.org/D1238

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