[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 537714.
yronglin added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-function-recovery-crash.cpp


Index: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
===================================================================
--- clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
+++ clang/test/SemaCXX/constexpr-function-recovery-crash.cpp
@@ -87,6 +87,7 @@
 // We're not checking specific recovery here so don't assert diagnostics.
 TEST_EVALUATE(Switch, switch (!!){});              // expected-error + {{}}
 TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}}
+TEST_EVALUATE(SwitchCondValDep, switch (invalid_value) { default: break; });    // expected-error + {{}}
 TEST_EVALUATE(For, for (!!){}); // expected-error + {{}}
                                 // FIXME: should bail out instead of looping.
                                 // expected-note at -2 + {{infinite loop}}
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4913,11 +4913,20 @@
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
   assert(E->isValueDependent());
-  if (Info.noteSideEffect())
-    return true;
+  // Note that we have a side effect that matters for constant evaluation.
+  bool SideEffects = Info.noteSideEffect();
+  // If the reason we're here is because of a recovery expression, we don't
+  // want to continue to evaluate further as we will never know what the actual
+  // value is.
+  if (isa<RecoveryExpr>(E))
+    return false;
+
+  // Otherwise, return whether we want to continue after noting the side
+  // effects, which should only happen if the expression has errors but isn't
+  // a recovery expression on its own.
   assert(E->containsErrors() && "valid value-dependent expression should never "
                                 "reach invalid code path.");
-  return false;
+  return SideEffects;
 }
 
 /// Evaluate a condition (either a variable declaration or an expression).
@@ -5007,12 +5016,13 @@
         !EvaluateDecl(Info, SS->getConditionVariable()))
       return ESR_Failed;
     if (SS->getCond()->isValueDependent()) {
-      if (!EvaluateDependentExpr(SS->getCond(), Info))
-        return ESR_Failed;
-    } else {
-      if (!EvaluateInteger(SS->getCond(), Value, Info))
-        return ESR_Failed;
+      // We don't know what the value is, and which branch should jump to.
+      EvaluateDependentExpr(SS->getCond(), Info);
+      return ESR_Failed;
     }
+    if (!EvaluateInteger(SS->getCond(), Value, Info))
+        return ESR_Failed;
+   
     if (!CondScope.destroy())
       return ESR_Failed;
   }
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -568,6 +568,9 @@
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 <https://github.com/llvm/llvm-project/issues/61758>`_)
+- Stop evaluating a constant expression if the condition expression which in
+  switch statement contains errors.
+  (`#63453 <https://github.com/llvm/llvm-project/issues/63453>_`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D153296.537714.patch
Type: text/x-patch
Size: 3270 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230706/5096352b/attachment-0001.bin>