rsmith created this revision.
rsmith added a reviewer: vsapsai.
rsmith requested review of this revision.
Herald added a project: clang.

Previously -Winteger-overflow did not do any checking of expressions
whose top-level syntactic form wasn't one of a small list of special
cases. This meant that the warning would appear and disappear depending
on enclosing syntax.

Additionally, if constant evaluation hit a case that it didn't
understand, it would often bail out without visiting subexpressions, so
we wouldn't see warnings on overflow in subexpressions depending on the
form of intervening expressions.

We now walk all evaluated subexpressions of a particular expression when
checking for overflow, and evaluate subtrees rooted on each operation
that might overflow. If such evaluation hits an unevaluatable
subexpression, we switch back to walking the AST looking for
subexpressions to evaluate.

The extra evaluation here also exposed an issue where casts between
complex and fixed-point types would create bogus AST nodes using
CK_IntegralCast, which would lead to the constant evaluator asserting.
That's fixed here too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98912

Files:
  clang/include/clang/AST/EvaluatedExprVisitor.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/integer-overflow.c
  clang/test/SemaCXX/integer-overflow.cpp

Index: clang/test/SemaCXX/integer-overflow.cpp
===================================================================
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only -std=gnu++98 -triple x86_64-pc-linux-gnu
-// RUN: %clang_cc1 %s -verify -fsyntax-only -std=gnu++2a -triple x86_64-pc-linux-gnu
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=gnu++98 -triple x86_64-pc-linux-gnu -fcxx-exceptions
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=gnu++2a -triple x86_64-pc-linux-gnu -fcxx-exceptions
 
 typedef unsigned long long uint64_t;
 typedef unsigned int uint32_t;
@@ -188,6 +188,20 @@
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   (void)f2(0, f0(4608 * 1024 * 1024));
+
+#if __cplusplus >= 201103L
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t n = uint64_t{f0(4608 * 1024 * 1024)};
+#endif
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  throw 4608 * 1024 * 1024;
+
+  // No warning, UB is not reachable.
+  void(1 ? 1 : 4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  void(0 ? 1 : 4608 * 1024 * 1024);
 }
 
 // Tests that ensure that evaluation-for-overflow of random expressions doesn't
Index: clang/test/Sema/integer-overflow.c
===================================================================
--- clang/test/Sema/integer-overflow.c
+++ clang/test/Sema/integer-overflow.c
@@ -6,6 +6,7 @@
 int array64[sizeof(uint64_t) == 8 ? 1 : -1];
 int array32[sizeof(uint32_t) == 4 ? 1 : -1];
 int arrayint[sizeof(int) < sizeof(uint64_t) ? 1 : -1];
+extern int array[];
 
 uint64_t f0(uint64_t);
 uint64_t f1(uint64_t, uint32_t);
@@ -39,6 +40,10 @@
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   overflow =  0 ? 0 : 4608 * 1024 * 1024;
 
+  if (i == 1)
+    // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+    return array[4608 * 1024 * 1024];
+
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   if (4608 * 1024 * 1024)
     return 0;
Index: clang/lib/Sema/SemaExprCXX.cpp
===================================================================
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -6381,9 +6381,15 @@
       return QualType();
     }
 
-    LHS = ImpCastExprToType(LHS.get(), ResTy, PrepareScalarCast(LHS, ResTy));
-    RHS = ImpCastExprToType(RHS.get(), ResTy, PrepareScalarCast(RHS, ResTy));
+    CastKind LHSCK = PrepareScalarCast(LHS, ResTy);
+    if (LHS.isInvalid())
+      return QualType();
+    CastKind RHSCK = PrepareScalarCast(RHS, ResTy);
+    if (RHS.isInvalid())
+      return QualType();
 
+    LHS = ImpCastExprToType(LHS.get(), ResTy, LHSCK);
+    RHS = ImpCastExprToType(RHS.get(), ResTy, RHSCK);
     return ResTy;
   }
 
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -7122,7 +7122,8 @@
       Diag(Src.get()->getExprLoc(),
            diag::err_unimplemented_conversion_with_fixed_point_type)
           << DestTy;
-      return CK_IntegralCast;
+      Src = ExprError();
+      return CK_Dependent;
     case Type::STK_CPointer:
     case Type::STK_ObjCObjectPointer:
     case Type::STK_BlockPointer:
@@ -7223,7 +7224,8 @@
       Diag(Src.get()->getExprLoc(),
            diag::err_unimplemented_conversion_with_fixed_point_type)
           << SrcTy;
-      return CK_IntegralCast;
+      Src = ExprError();
+      return CK_Dependent;
     }
     llvm_unreachable("Should have returned before this");
 
@@ -7257,7 +7259,8 @@
       Diag(Src.get()->getExprLoc(),
            diag::err_unimplemented_conversion_with_fixed_point_type)
           << SrcTy;
-      return CK_IntegralCast;
+      Src = ExprError();
+      return CK_Dependent;
     }
     llvm_unreachable("Should have returned before this");
   }
@@ -7573,8 +7576,10 @@
       ExprResult Literal = DefaultLvalueConversion(exprs[0]);
       if (Literal.isInvalid())
         return ExprError();
-      Literal = ImpCastExprToType(Literal.get(), ElemTy,
-                                  PrepareScalarCast(Literal, ElemTy));
+      CastKind CK = PrepareScalarCast(Literal, ElemTy);
+      if (Literal.isInvalid())
+        return ExprError();
+      Literal = ImpCastExprToType(Literal.get(), ElemTy, CK);
       return BuildCStyleCastExpr(LParenLoc, TInfo, RParenLoc, Literal.get());
     }
     else if (numExprs < numElems) {
@@ -7595,8 +7600,10 @@
         ExprResult Literal = DefaultLvalueConversion(exprs[0]);
         if (Literal.isInvalid())
           return ExprError();
-        Literal = ImpCastExprToType(Literal.get(), ElemTy,
-                                    PrepareScalarCast(Literal, ElemTy));
+        CastKind CK = PrepareScalarCast(Literal, ElemTy);
+        if (Literal.isInvalid())
+          return ExprError();
+        Literal = ImpCastExprToType(Literal.get(), ElemTy, CK);
         return BuildCStyleCastExpr(LParenLoc, TInfo, RParenLoc, Literal.get());
     }
 
@@ -8220,9 +8227,16 @@
       return QualType();
     }
 
-    LHS = ImpCastExprToType(LHS.get(), ResTy, PrepareScalarCast(LHS, ResTy));
-    RHS = ImpCastExprToType(RHS.get(), ResTy, PrepareScalarCast(RHS, ResTy));
+    CastKind LHSCK = PrepareScalarCast(LHS, ResTy);
+    if (LHS.isInvalid())
+      return QualType();
 
+    CastKind RHSCK = PrepareScalarCast(RHS, ResTy);
+    if (RHS.isInvalid())
+      return QualType();
+
+    LHS = ImpCastExprToType(LHS.get(), ResTy, LHSCK);
+    RHS = ImpCastExprToType(RHS.get(), ResTy, RHSCK);
     return ResTy;
   }
 
@@ -9141,8 +9155,13 @@
   // Arithmetic conversions.
   if (LHSType->isArithmeticType() && RHSType->isArithmeticType() &&
       !(getLangOpts().CPlusPlus && LHSType->isEnumeralType())) {
-    if (ConvertRHS)
+    if (ConvertRHS) {
       Kind = PrepareScalarCast(RHS, LHSType);
+      // FIXME: Let the caller know we already diagnosed this to avoid a
+      // duplicate diagnostic.
+      if (RHS.isInvalid())
+        return Incompatible;
+    }
     return Compatible;
   }
 
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13080,32 +13080,6 @@
   ::CheckBoolLikeConversion(*this, E, CC);
 }
 
-/// Diagnose when expression is an integer constant expression and its evaluation
-/// results in integer overflow
-void Sema::CheckForIntOverflow (Expr *E) {
-  // Use a work list to deal with nested struct initializers.
-  SmallVector<Expr *, 2> Exprs(1, E);
-
-  do {
-    Expr *OriginalE = Exprs.pop_back_val();
-    Expr *E = OriginalE->IgnoreParenCasts();
-
-    if (isa<BinaryOperator>(E)) {
-      E->EvaluateForOverflow(Context);
-      continue;
-    }
-
-    if (auto InitList = dyn_cast<InitListExpr>(OriginalE))
-      Exprs.append(InitList->inits().begin(), InitList->inits().end());
-    else if (isa<ObjCBoxedExpr>(OriginalE))
-      E->EvaluateForOverflow(Context);
-    else if (auto Call = dyn_cast<CallExpr>(E))
-      Exprs.append(Call->arg_begin(), Call->arg_end());
-    else if (auto Message = dyn_cast<ObjCMessageExpr>(E))
-      Exprs.append(Message->arg_begin(), Message->arg_end());
-  } while (!Exprs.empty());
-}
-
 namespace {
 
 /// Visitor for expressions which looks for unsequenced operations on the
@@ -13974,8 +13948,8 @@
   CheckImplicitConversions(E, CheckLoc);
   if (!E->isInstantiationDependent())
     CheckUnsequencedOperations(E);
-  if (!IsConstexpr && !E->isValueDependent())
-    CheckForIntOverflow(E);
+  if (!IsConstexpr)
+    E->EvaluateForDiagnostics(Context);
   DiagnoseMisalignedMembers();
 }
 
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -44,6 +44,7 @@
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/CurrentSourceLocExprScope.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/OSLog.h"
 #include "clang/AST/OptionalDiagnostic.h"
 #include "clang/AST/RecordLayout.h"
@@ -958,13 +959,27 @@
       return CheckingPotentialConstantExpression;
     }
 
-    /// Are we checking an expression for overflow?
-    // FIXME: We should check for any kind of undefined or suspicious behavior
-    // in such constructs, not just overflow.
+    /// Are we checking an expression for undefined?
+    ///
+    /// If any new cases are added that emit warnings on undefined behavior,
+    /// mightWarnOnUndefinedBehavior should be updated to match.
+    ///
+    /// FIXME: Add checks for more kinds of undefined or suspicious behavior.
     bool checkingForUndefinedBehavior() const override {
       return CheckingForUndefinedBehavior;
     }
 
+    // Determine if we might warn that the given expression exhibits undefined
+    // behavior.
+    bool mightWarnOnUndefinedBehavior(const Expr *E) {
+      if (!checkingForUndefinedBehavior())
+        return false;
+      if (auto *BO = dyn_cast<BinaryOperator>(E))
+        return BO->getType()->isIntegralOrEnumerationType() ||
+               BO->getType()->isFixedPointType();
+      return false;
+    }
+
     EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
         : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
           CallStackDepth(0), NextCallIndex(1),
@@ -7251,6 +7266,67 @@
   return true;
 }
 
+/// Evaluate the potentially-evaluated subexpressions of the given expression
+/// looking for undefined behavior, and discard the evaluation results.
+static void EvaluateForDiagnostics(EvalInfo &Info, const Expr *E,
+                                   bool SubexpressionsOnly) {
+  assert(Info.checkingForUndefinedBehavior() &&
+         "should only be called when checking for UB");
+
+  // Let the EvalInfo know that we're potentially skipping side-effects.
+  if (!Info.noteFailure())
+    return;
+
+  struct SubexpressionVisitor
+      : ConstEvaluatedExprVisitor<SubexpressionVisitor> {
+    EvalInfo &Info;
+    // A list of interesting expressions that we are going to evaluate.
+    SmallVector<const Expr*, 8> WorkList;
+
+    SubexpressionVisitor(EvalInfo &Info)
+        : ConstEvaluatedExprVisitor(Info.Ctx), Info(Info) {}
+
+    bool ShouldEvaluate(const Expr *E) {
+      // Evaluate an expression if it might either produce a warning or result
+      // in control flow.
+      // FIXME: We should use DiagRuntimeBehavior for our UB warnings, but it's
+      // not reachable from here.
+      return Info.mightWarnOnUndefinedBehavior(E) ||
+             isa<AbstractConditionalOperator>(E);
+    }
+
+    void Enqueue(const Expr *E, bool SubexpressionsOnly) {
+      if (SubexpressionsOnly)
+        EnqueueSubexpressions(E);
+      else
+        WorkList.push_back(E);
+    }
+
+    void EnqueueSubexpressions(const Expr *E) {
+      ConstEvaluatedExprVisitor::Visit(E);
+    }
+
+    void Run() {
+      while (!WorkList.empty() && Info.keepEvaluatingAfterFailure()) {
+        const Expr *E = WorkList.pop_back_val();
+        if (!E->isValueDependent() && ShouldEvaluate(E))
+          EvaluateIgnoredValue(Info, E);
+        else
+          EnqueueSubexpressions(E);
+      }
+    }
+
+    void Visit(const Stmt *Child) {
+      // Don't recurse to sub-statements; those will be checked separately.
+      if (auto *SubExpr = dyn_cast<Expr>(Child))
+        WorkList.push_back(SubExpr);
+    }
+  } Visitor(Info);
+
+  Visitor.Enqueue(E, SubexpressionsOnly);
+  Visitor.Run();
+}
+
 template <class Derived>
 class ExprEvaluatorBase
   : public ConstStmtVisitor<Derived, bool> {
@@ -7340,7 +7416,14 @@
     llvm_unreachable("Expression evaluator should not be called on stmts");
   }
   bool VisitExpr(const Expr *E) {
-    return Error(E);
+    Error(E);
+
+    // We can't produce a value for this expression. If we're checking for UB,
+    // we should still recurse into subexpressions to check them.
+    if (Info.checkingForUndefinedBehavior())
+      ::EvaluateForDiagnostics(Info, E, /*SubexpressionsOnly=*/true);
+
+    return false;
   }
 
   bool VisitConstantExpr(const ConstantExpr *E) {
@@ -7403,7 +7486,7 @@
   bool VisitBinaryOperator(const BinaryOperator *E) {
     switch (E->getOpcode()) {
     default:
-      return Error(E);
+      return VisitExpr(E);
 
     case BO_Comma:
       VisitIgnoredValue(E->getLHS());
@@ -7651,8 +7734,9 @@
           return HandleOperatorDeleteCall(Info, E) && CallScope.destroy();
         }
       }
-    } else
-      return Error(E);
+    } else {
+      return VisitExpr(E);
+    }
 
     // Evaluate the arguments now if we've not already done so.
     if (!Call) {
@@ -7711,7 +7795,7 @@
       return DerivedZeroInitialization(E);
     if (E->getNumInits() == 1)
       return StmtVisitorTy::Visit(E->getInit(0));
-    return Error(E);
+    return VisitExpr(E);
   }
   bool VisitImplicitValueInitExpr(const ImplicitValueInitExpr *E) {
     return DerivedZeroInitialization(E);
@@ -7825,7 +7909,7 @@
     }
     }
 
-    return Error(E);
+    return VisitExpr(E);
   }
 
   bool VisitUnaryPostInc(const UnaryOperator *UO) {
@@ -7835,8 +7919,8 @@
     return VisitUnaryPostIncDec(UO);
   }
   bool VisitUnaryPostIncDec(const UnaryOperator *UO) {
-    if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
-      return Error(UO);
+    if (!Info.getLangOpts().CPlusPlus14)
+      return VisitExpr(UO);
 
     LValue LVal;
     if (!EvaluateLValue(UO->getSubExpr(), LVal, Info))
@@ -8352,7 +8436,7 @@
 bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
   // FIXME: Deal with vectors as array subscript bases.
   if (E->getBase()->getType()->isVectorType())
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitArraySubscriptExpr(E);
 
   APSInt Index;
   bool Success = true;
@@ -8395,8 +8479,8 @@
 }
 
 bool LValueExprEvaluator::VisitUnaryPreIncDec(const UnaryOperator *UO) {
-  if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
-    return Error(UO);
+  if (!Info.getLangOpts().CPlusPlus14)
+    return VisitExpr(UO);
 
   if (!this->Visit(UO->getSubExpr()))
     return false;
@@ -8602,9 +8686,7 @@
   bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E) {
     if (E->isExpressibleAsConstantInitializer())
       return Success(E);
-    if (Info.noteFailure())
-      EvaluateIgnoredValue(Info, E->getSubExpr());
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitObjCBoxedExpr(E);
   }
   bool VisitAddrLabelExpr(const AddrLabelExpr *E)
       { return Success(E); }
@@ -10174,7 +10256,7 @@
         return false;
       Val = APValue(std::move(FloatResult));
     } else {
-      return Error(E);
+      return ExprEvaluatorBaseTy::VisitCastExpr(E);
     }
 
     // Splat and create vector APValue.
@@ -10440,7 +10522,7 @@
   const ConstantArrayType *CAT = Info.Ctx.getAsConstantArrayType(
       AllocType.isNull() ? E->getType() : AllocType);
   if (!CAT)
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitInitListExpr(E);
 
   // C++11 [dcl.init.string]p1: A char array [...] can be initialized by [...]
   // an appropriately-typed string literal enclosed in braces.
@@ -10580,7 +10662,7 @@
   }
 
   if (!Type->isRecordType())
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitCXXConstructExpr(E);
 
   return RecordExprEvaluator(Info, Subobject, *Value)
              .VisitCXXConstructExpr(E, Type);
@@ -12214,7 +12296,9 @@
   static bool shouldEnqueue(const BinaryOperator *E) {
     return E->getOpcode() == BO_Comma || E->isLogicalOp() ||
            (E->isRValue() && E->getType()->isIntegralOrEnumerationType() &&
+            E->getLHS()->isRValue() &&
             E->getLHS()->getType()->isIntegralOrEnumerationType() &&
+            E->getRHS()->isRValue() &&
             E->getRHS()->getType()->isIntegralOrEnumerationType());
   }
 
@@ -12224,7 +12308,8 @@
     while (!Queue.empty())
       process(PrevResult);
 
-    if (PrevResult.Failed) return false;
+    if (PrevResult.Failed)
+      return false;
 
     FinalResult.swap(PrevResult.Val);
     return true;
@@ -12839,21 +12924,9 @@
 }
 
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
-  // We don't support assignment in C. C++ assignments don't get here because
-  // assignment is an lvalue in C++.
-  if (E->isAssignmentOp()) {
-    Error(E);
-    if (!Info.noteFailure())
-      return false;
-  }
-
   if (DataRecursiveIntBinOpEvaluator::shouldEnqueue(E))
     return DataRecursiveIntBinOpEvaluator(*this, Result).Traverse(E);
 
-  assert((!E->getLHS()->getType()->isIntegralOrEnumerationType() ||
-          !E->getRHS()->getType()->isIntegralOrEnumerationType()) &&
-         "DataRecursiveIntBinOpEvaluator should have handled integral types");
-
   if (E->isComparisonOp()) {
     // Evaluate builtin binary comparisons by evaluating them as three-way
     // comparisons and then translating the result.
@@ -13103,7 +13176,7 @@
   default:
     // Address, indirect, pre/post inc/dec, etc are not valid constant exprs.
     // See C99 6.6p3.
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitUnaryOperator(E);
   case UO_Extension:
     // FIXME: Should extension allow i-c-e extension expressions in its scope?
     // If so, we could clear the diagnostic ID.
@@ -13193,7 +13266,8 @@
   case CK_ARCReclaimReturnedObject:
   case CK_ARCExtendBlockObject:
   case CK_CopyAndAutoreleaseBlockObject:
-    return Error(E);
+    // Allow the base class to reject.
+    return ExprEvaluatorBaseTy::VisitCastExpr(E);
 
   case CK_UserDefinedConversion:
   case CK_LValueToRValue:
@@ -13358,7 +13432,7 @@
   switch (E->getOpcode()) {
     default:
       // Invalid unary operators
-      return Error(E);
+      return ExprEvaluatorBaseTy::VisitUnaryOperator(E);
     case UO_Plus:
       // The result is just the value.
       return Visit(E->getSubExpr());
@@ -13395,14 +13469,8 @@
       return false;
     bool Overflowed;
     APFixedPoint Result = Src.convert(DestFXSema, &Overflowed);
-    if (Overflowed) {
-      if (Info.checkingForUndefinedBehavior())
-        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-                                         diag::warn_fixedpoint_constant_overflow)
-          << Result.toString() << E->getType();
-      if (!HandleOverflow(Info, E, Result, E->getType()))
-        return false;
-    }
+    if (Overflowed && !HandleOverflow(Info, E, Result, E->getType()))
+      return false;
     return Success(Result, E);
   }
   case CK_IntegralToFixedPoint: {
@@ -13414,14 +13482,8 @@
     APFixedPoint IntResult = APFixedPoint::getFromIntValue(
         Src, Info.Ctx.getFixedPointSemantics(DestType), &Overflowed);
 
-    if (Overflowed) {
-      if (Info.checkingForUndefinedBehavior())
-        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-                                         diag::warn_fixedpoint_constant_overflow)
-          << IntResult.toString() << E->getType();
-      if (!HandleOverflow(Info, E, IntResult, E->getType()))
-        return false;
-    }
+    if (Overflowed && !HandleOverflow(Info, E, IntResult, E->getType()))
+      return false;
 
     return Success(IntResult, E);
   }
@@ -13434,22 +13496,13 @@
     APFixedPoint Result = APFixedPoint::getFromFloatValue(
         Src, Info.Ctx.getFixedPointSemantics(DestType), &Overflowed);
 
-    if (Overflowed) {
-      if (Info.checkingForUndefinedBehavior())
-        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
-                                         diag::warn_fixedpoint_constant_overflow)
-          << Result.toString() << E->getType();
-      if (!HandleOverflow(Info, E, Result, E->getType()))
-        return false;
-    }
+    if (Overflowed && !HandleOverflow(Info, E, Result, E->getType()))
+      return false;
 
     return Success(Result, E);
   }
-  case CK_NoOp:
-  case CK_LValueToRValue:
-    return ExprEvaluatorBaseTy::VisitCastExpr(E);
   default:
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitCastExpr(E);
   }
 }
 
@@ -13526,7 +13579,7 @@
     if (Info.checkingForUndefinedBehavior())
       Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                        diag::warn_fixedpoint_constant_overflow)
-        << Result.toString() << E->getType();
+          << Result.toString() << E->getType();
     if (!HandleOverflow(Info, E, Result, E->getType()))
       return false;
   }
@@ -13715,7 +13768,8 @@
 
 bool FloatExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
   switch (E->getOpcode()) {
-  default: return Error(E);
+  default:
+    return ExprEvaluatorBaseTy::VisitUnaryOperator(E);
   case UO_Plus:
     return EvaluateFloat(E->getSubExpr(), Result, Info);
   case UO_Minus:
@@ -13892,8 +13946,8 @@
   case CK_PointerToBoolean:
   case CK_ToVoid:
   case CK_VectorSplat:
+  case CK_BooleanToSignedIntegral:
   case CK_IntegralCast:
-  case CK_BooleanToSignedIntegral:
   case CK_IntegralToBoolean:
   case CK_IntegralToFloating:
   case CK_FloatingToIntegral:
@@ -13929,12 +13983,10 @@
   case CK_AtomicToNonAtomic:
   case CK_NoOp:
   case CK_LValueToRValueBitCast:
-    return ExprEvaluatorBaseTy::VisitCastExpr(E);
-
   case CK_Dependent:
   case CK_LValueBitCast:
   case CK_UserDefinedConversion:
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitCastExpr(E);
 
   case CK_FloatingRealToComplex: {
     APFloat &Real = Result.FloatReal;
@@ -14246,7 +14298,7 @@
 
   switch (E->getOpcode()) {
   default:
-    return Error(E);
+    return ExprEvaluatorBaseTy::VisitUnaryOperator(E);
   case UO_Extension:
     return true;
   case UO_Plus:
@@ -14988,17 +15040,11 @@
   return EVResult.Val.getInt();
 }
 
-void Expr::EvaluateForOverflow(const ASTContext &Ctx) const {
-  assert(!isValueDependent() &&
-         "Expression evaluator can't be called on a dependent expression.");
-
-  bool IsConst;
-  EvalResult EVResult;
-  if (!FastEvaluateAsRValue(this, EVResult, Ctx, IsConst)) {
-    EvalInfo Info(Ctx, EVResult, EvalInfo::EM_IgnoreSideEffects);
-    Info.CheckingForUndefinedBehavior = true;
-    (void)::EvaluateAsRValue(Info, this, EVResult.Val);
-  }
+void Expr::EvaluateForDiagnostics(const ASTContext &Ctx) const {
+  EvalStatus Status;
+  EvalInfo Info(Ctx, Status, EvalInfo::EM_IgnoreSideEffects);
+  Info.CheckingForUndefinedBehavior = true;
+  ::EvaluateForDiagnostics(Info, this, /*SubexpressionsOnly=*/false);
 }
 
 bool Expr::EvalResult::isGlobalLValue() const {
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -12565,7 +12565,6 @@
 private:
   void CheckImplicitConversions(Expr *E, SourceLocation CC = SourceLocation());
   void CheckBoolLikeConversion(Expr *E, SourceLocation CC);
-  void CheckForIntOverflow(Expr *E);
   void CheckUnsequencedOperations(const Expr *E);
 
   /// Perform semantic checks on a completed expression. This will either
Index: clang/include/clang/AST/Expr.h
===================================================================
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -686,7 +686,9 @@
       const ASTContext &Ctx,
       SmallVectorImpl<PartialDiagnosticAt> *Diag = nullptr) const;
 
-  void EvaluateForOverflow(const ASTContext &Ctx) const;
+  /// Evaluate this expression looking for and warning on undefined behavior.
+  /// This is valid even on value-dependent expressions.
+  void EvaluateForDiagnostics(const ASTContext &Ctx) const;
 
   /// EvaluateAsLValue - Evaluate an expression to see if we can fold it to an
   /// lvalue with link time known address, with no side-effects.
Index: clang/include/clang/AST/EvaluatedExprVisitor.h
===================================================================
--- clang/include/clang/AST/EvaluatedExprVisitor.h
+++ clang/include/clang/AST/EvaluatedExprVisitor.h
@@ -48,7 +48,7 @@
 
   void VisitMemberExpr(PTR(MemberExpr) E) {
     // Only the base matters.
-    return this->Visit(E->getBase());
+    return static_cast<ImplClass*>(this)->Visit(E->getBase());
   }
 
   void VisitChooseExpr(PTR(ChooseExpr) E) {
@@ -56,7 +56,7 @@
     if (E->getCond()->isValueDependent())
       return;
     // Only the selected subexpression matters; the other one is not evaluated.
-    return this->Visit(E->getChosenSubExpr());
+    return static_cast<ImplClass*>(this)->Visit(E->getChosenSubExpr());
   }
 
   void VisitGenericSelectionExpr(PTR(GenericSelectionExpr) E) {
@@ -67,18 +67,18 @@
       return;
     // Only the selected subexpression matters; the other subexpressions and the
     // controlling expression are not evaluated.
-    return this->Visit(E->getResultExpr());
+    return static_cast<ImplClass*>(this)->Visit(E->getResultExpr());
   }
 
   void VisitDesignatedInitExpr(PTR(DesignatedInitExpr) E) {
     // Only the actual initializer matters; the designators are all constant
     // expressions.
-    return this->Visit(E->getInit());
+    return static_cast<ImplClass*>(this)->Visit(E->getInit());
   }
 
   void VisitCXXTypeidExpr(PTR(CXXTypeidExpr) E) {
     if (E->isPotentiallyEvaluated())
-      return this->Visit(E->getExprOperand());
+      return static_cast<ImplClass*>(this)->Visit(E->getExprOperand());
   }
 
   void VisitCallExpr(PTR(CallExpr) CE) {
@@ -92,7 +92,7 @@
                                                  E = LE->capture_init_end();
          I != E; ++I)
       if (*I)
-        this->Visit(*I);
+        static_cast<ImplClass*>(this)->Visit(*I);
   }
 
   /// The basis case walks all of the children of the statement or
@@ -100,7 +100,7 @@
   void VisitStmt(PTR(Stmt) S) {
     for (auto *SubStmt : S->children())
       if (SubStmt)
-        this->Visit(SubStmt);
+        static_cast<ImplClass*>(this)->Visit(SubStmt);
   }
 
 #undef PTR
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D98912: F... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to