[clang] bdf6fbc - PR49239: Don't take shortcuts when constant evaluating in 'warn on UB'

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2021-02-18T18:31:08-08:00
New Revision: bdf6fbc939646b52af61c0bae7335623a8be59f4

URL: https://github.com/llvm/llvm-project/commit/bdf6fbc939646b52af61c0bae7335623a8be59f4
DIFF: https://github.com/llvm/llvm-project/commit/bdf6fbc939646b52af61c0bae7335623a8be59f4.diff

LOG: PR49239: Don't take shortcuts when constant evaluating in 'warn on UB'
mode.

We use that mode when evaluating ICEs in C, and those shortcuts could
result in ICE evaluation producing the wrong answer, specifically if we
evaluate a statement-expression as part of evaluating the ICE.

Added: 
    

Modified: 
    clang/lib/AST/ExprConstant.cpp
    clang/test/Sema/i-c-e.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 25aaf4c70c36..ae7131eae01d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -926,6 +926,9 @@ namespace {
     /// Whether we're checking for an expression that has undefined behavior.
     /// If so, we will produce warnings if we encounter an operation that is
     /// always undefined.
+    ///
+    /// Note that we still need to evaluate the expression normally when this
+    /// is set; this is used when evaluating ICEs in C.
     bool CheckingForUndefinedBehavior = false;
 
     enum EvaluationMode {
@@ -2715,8 +2718,7 @@ static bool CheckedIntArithmetic(EvalInfo &Info, const Expr *E,
       Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                        diag::warn_integer_constant_overflow)
           << Result.toString(10) << E->getType();
-    else
-      return HandleOverflow(Info, E, Value, E->getType());
+    return HandleOverflow(Info, E, Value, E->getType());
   }
   return true;
 }
@@ -7846,8 +7848,8 @@ class ExprEvaluatorBase
   bool VisitStmtExpr(const StmtExpr *E) {
     // We will have checked the full-expressions inside the statement expression
     // when they were completed, and don't need to check them again now.
-    if (Info.checkingForUndefinedBehavior())
-      return Error(E);
+    llvm::SaveAndRestore<bool> NotCheckingForUB(
+        Info.CheckingForUndefinedBehavior, false);
 
     const CompoundStmt *CS = E->getSubStmt();
     if (CS->body_empty())
@@ -13412,7 +13414,7 @@ bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
         Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                          diag::warn_fixedpoint_constant_overflow)
           << Result.toString() << E->getType();
-      else if (!HandleOverflow(Info, E, Result, E->getType()))
+      if (!HandleOverflow(Info, E, Result, E->getType()))
         return false;
     }
     return Success(Result, E);
@@ -13431,7 +13433,7 @@ bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
         Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                          diag::warn_fixedpoint_constant_overflow)
           << IntResult.toString() << E->getType();
-      else if (!HandleOverflow(Info, E, IntResult, E->getType()))
+      if (!HandleOverflow(Info, E, IntResult, E->getType()))
         return false;
     }
 
@@ -13451,7 +13453,7 @@ bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
         Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                          diag::warn_fixedpoint_constant_overflow)
           << Result.toString() << E->getType();
-      else if (!HandleOverflow(Info, E, Result, E->getType()))
+      if (!HandleOverflow(Info, E, Result, E->getType()))
         return false;
     }
 
@@ -13539,7 +13541,7 @@ bool FixedPointExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
       Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                        diag::warn_fixedpoint_constant_overflow)
         << Result.toString() << E->getType();
-    else if (!HandleOverflow(Info, E, Result, E->getType()))
+    if (!HandleOverflow(Info, E, Result, E->getType()))
       return false;
   }
   return Success(Result, E);

diff  --git a/clang/test/Sema/i-c-e.c b/clang/test/Sema/i-c-e.c
index 120f4b5f4889..63416454f5a4 100644
--- a/clang/test/Sema/i-c-e.c
+++ b/clang/test/Sema/i-c-e.c
@@ -25,7 +25,23 @@ struct c {
            ) : -1);
 };
 
-
+// Check that we can evaluate statement-expressions properly when
+// constant-folding inside an ICE.
+void PR49239() {
+  goto check_not_vla;
+  char not_vla[__builtin_constant_p(1) ? ({ 42; }) : -1]; // expected-warning {{statement expression}}
+check_not_vla:;
+  _Static_assert(sizeof(not_vla) == 42, ""); // expected-warning {{C11 extension}}
+
+  // It's not clear that this should be valid: __builtin_expect(expr1, expr2)
+  // should probably be an ICE if and only if expr1 is an ICE, but we roughly
+  // follow GCC in treating it as an ICE if and only if we can evaluate expr1
+  // regardless of whether it's an ICE.
+  goto check_also_not_vla;
+  char also_not_vla[__builtin_expect(({ 76; }), 0)]; // expected-warning {{statement expression}}
+check_also_not_vla:;
+  _Static_assert(sizeof(also_not_vla) == 76, ""); // expected-warning {{C11 extension}}
+}
 
 
 void test1(int n, int* p) { *(n ? p : (void *)(7-7)) = 1; }