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

[yronglin]

Summary

This fix issue: https://github.com/llvm/llvm-project/issues/63453

constexpr int foo(unsigned char c) {
    switch (f) {
    case 0:
        return 7;
    default:
        break;
    }
    return 0;
}

static_assert(foo('d'));

[yronglin]

Oops, compiler explorer website seems crashed, Once it's recovery, I'll append a link.

<source>:2:13: error: use of undeclared identifier 'f'
    2 |     switch (f) {
      |             ^
clang++: /root/llvm-project/llvm/lib/Support/APInt.cpp:282: int llvm::APInt::compareSigned(const llvm::APInt&) const: Assertion `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /opt/compiler-explorer/clang-assertions-trunk/bin/clang++ -gdwarf-4 -g -o /app/output.s -mllvm --x86-asm-syntax=intel -S --gcc-toolchain=/opt/compiler-explorer/gcc-snapshot -fcolor-diagnostics -fno-crash-diagnostics -std=c++20 <source>
1.	<source>:11:1: current parser token 'static_assert'
2.	<source>:1:36: parsing function body 'foo'
 #0 0x000055cfeaf267ff llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3c3a7ff)
 #1 0x000055cfeaf2456c llvm::sys::CleanupOnSignal(unsigned long) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3c3856c)
 #2 0x000055cfeae6e528 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007f6bd7059420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
 #4 0x00007f6bd6b2600b raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300b)
 #5 0x00007f6bd6b05859 abort (/lib/x86_64-linux-gnu/libc.so.6+0x22859)
 #6 0x00007f6bd6b05729 (/lib/x86_64-linux-gnu/libc.so.6+0x22729)
 #7 0x00007f6bd6b16fd6 (/lib/x86_64-linux-gnu/libc.so.6+0x33fd6)
 #8 0x000055cfeae461cd llvm::APInt::compareSigned(llvm::APInt const&) const (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3b5a1cd)
 #9 0x000055cfee84a705 EvaluateStmt((anonymous namespace)::StmtResult&, (anonymous namespace)::EvalInfo&, clang::Stmt const*, clang::SwitchCase const*) (.part.0) ExprConstant.cpp:0:0
#10 0x000055cfee84a25f EvaluateStmt((anonymous namespace)::StmtResult&, (anonymous namespace)::EvalInfo&, clang::Stmt const*, clang::SwitchCase const*) (.part.0) ExprConstant.cpp:0:0
#11 0x000055cfee84e961 HandleFunctionCall(clang::SourceLocation, clang::FunctionDecl const*, (anonymous namespace)::LValue const*, llvm::ArrayRef<clang::Expr const*>, (anonymous namespace)::CallRef, clang::Stmt const*, (anonymous namespace)::EvalInfo&, clang::APValue&, (anonymous namespace)::LValue const*) ExprConstant.cpp:0:0
#12 0x000055cfee8918d8 clang::Expr::isPotentialConstantExpr(clang::FunctionDecl const*, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic>>&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x75a58d8)
#13 0x000055cfedaae4de clang::Sema::CheckConstexprFunctionDefinition(clang::FunctionDecl const*, clang::Sema::CheckConstexprKind) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x67c24de)
#14 0x000055cfeda05350 clang::Sema::ActOnFinishFunctionBody(clang::Decl*, clang::Stmt*, bool) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x6719350)
#15 0x000055cfed77b2dd clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x648f2dd)
#16 0x000055cfed6a5dc1 clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b9dc1)
#17 0x000055cfed6cccd0 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63e0cd0)
#18 0x000055cfed699661 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63ad661)
#19 0x000055cfed699f1f clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) (.part.0) Parser.cpp:0:0
#20 0x000055cfed6a08c1 clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b48c1)
#21 0x000055cfed6a1236 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b5236)
#22 0x000055cfed6a16d4 clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63b56d4)
#23 0x000055cfed694fca clang::ParseAST(clang::Sema&, bool, bool) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x63a8fca)
#24 0x000055cfec1997d8 clang::CodeGenAction::ExecuteAction() (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x4ead7d8)
#25 0x000055cfeb9e6a49 clang::FrontendAction::Execute() (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x46faa49)
#26 0x000055cfeb96b996 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x467f996)
#27 0x000055cfebacac96 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x47dec96)
#28 0x000055cfe83d4fed cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x10e8fed)
#29 0x000055cfe83d0cea ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0
#30 0x000055cfeb7ccd5d void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::'lambda'()>(long) Job.cpp:0:0
#31 0x000055cfeae6ea30 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x3b82a30)
#32 0x000055cfeb7cd37f clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const (.part.0) Job.cpp:0:0
#33 0x000055cfeb79472c clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x44a872c)
#34 0x000055cfeb7951bd clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&, bool) const (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x44a91bd)
#35 0x000055cfeb79d29d clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x44b129d)
#36 0x000055cfe83d324a clang_main(int, char**, llvm::ToolContext const&) (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x10e724a)
#37 0x000055cfe82d95c5 main (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0xfed5c5)
#38 0x00007f6bd6b07083 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24083)
#39 0x000055cfe83cbace _start (/opt/compiler-explorer/clang-assertions-trunk/bin/clang+++0x10dface)
clang++: error: clang frontend command failed with exit code 134 (use -v to see invocation)
Compiler returned: 134

[yronglin]

Fix test

[erichkeane]

clang/lib/AST/ExprConstant.cpp

4991

It seems to me that the 'better' solution is to make EvaluateDependentExpr (or one of its children) be RecoveryExpr aware, and result in a failed value instead. That way we get this 'fix' for more than just switch statements.

[yronglin]

Address comments

[yronglin]

clang/lib/AST/ExprConstant.cpp

4991

Thanks for your review!

[erichkeane]

Also needs a release note.

clang/lib/AST/ExprConstant.cpp

4893

I'd probably suggest E->containsErrors() instead, to cover cases where we're not the 'root' of a recovery expr? So something like:

switch(func_call(unknown_value))

should create a dependent call expr, but would still contain errors.

[yronglin]

Add ReleaseNotes

[erichkeane]

Just a rewording of the message, else LGTM.

clang/docs/ReleaseNotes.rst

523 ↗

(On Diff #533735)

[yronglin]

clang/lib/AST/ExprConstant.cpp

4893

Thanks! Use E->containsErrors() and added into release note.

[yronglin]

Update wording.

[yronglin]

In D153296#4442363, @erichkeane wrote:

Just a rewording of the message, else LGTM.

Thanks a lot for you're review!

[yronglin]

clang/lib/AST/ExprConstant.cpp

4893

Seems check error inside EvaluateDependentExpr will missing diagnostic messages.

This case was introduced in D84637

constexpr int test5() { // expected-error {{constexpr function never produce}}
  for (;; a++); // expected-error {{use of undeclared identifier}}  \
                   expected-note {{constexpr evaluation hit maximum step limit; possible infinite loop?}}
  return 1;
}

./main.cpp:2:11: error: use of undeclared identifier 'a'
    2 |   for (;; a++); // expected-error {{use of undeclared identifier}}  \
      |           ^
1 error generated.

But I think the infinite loop diagnostic is unnecessary, should we update the test case? WDYT?

[shafik]

clang/lib/AST/ExprConstant.cpp

4991

Erich so there are places in ExprConstant.cpp where if we isValueDependent() we bail out like in the Stmt::ReturnStmtClass case inside EvaluateStmt1(). The gist I get from the comment there is

cpp
// We know we returned, but we don't know what the value is.

Is that not correct or does it depend on each specific case?

[shafik]

clang/lib/AST/ExprConstant.cpp

4989

As far as I can tell Value will still not be set if we don't return here and we will still crash when we attempt to compare Value below:

LHS <= Value && Value <= RHS

[erichkeane]

clang/lib/AST/ExprConstant.cpp

4893

Huh... thats interesting. The 'Info.noteSideEffect' is sufficient to do that? Looking closer, I wonder if this is the wrong fix and his original idea was better. It seems that this already has a 'containsError' check at the end, and should if it doesn't have side effects.

I was hoping to have the problem generalized, but I think I was wrong, and we should go back to just fixing the 'switch' statements.

4989

I don't understand the comment? We're returning 'failed', aren't we? Except now EvaluateDependentExpris where the failure is coming from?

4991

TBH, i don't have a good idea of that. I spent a little time digging here, but the constant evaluator isn't my forte.

[erichkeane]

So I think I'm pretty confident that the only time we would call EvaluateDependentExpr is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I think returning early there is incorrect as well, at least from a 'code correctness' (even if there isn't a reproducer that would matter?). I think we're in a case where we want to continue in order to ensure we go through the entire flow, so I THINK we should treat this as 'we have a value we don't know, so its just not found', and should fall into the check on 5019 (unless of course, there is a 'default' option!).

So I think that we should be checking if Value is valid right after the default check, which lets us fall into the 'default' branch and get additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?

[shafik]

In D153296#4444612, @erichkeane wrote:

So I think I'm pretty confident that the only time we would call EvaluateDependentExpr is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I think returning early there is incorrect as well, at least from a 'code correctness' (even if there isn't a reproducer that would matter?). I think we're in a case where we want to continue in order to ensure we go through the entire flow, so I THINK we should treat this as 'we have a value we don't know, so its just not found', and should fall into the check on 5019 (unless of course, there is a 'default' option!).

So I think that we should be checking if Value is valid right after the default check, which lets us fall into the 'default' branch and get additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?

I do see what you are saying but I think that we have similar cases where without a value the next step is impossible to do for example in EvaluateStmt the case Stmt::ReturnStmtClass: case:

case Stmt::ReturnStmtClass: {
   const Expr *RetExpr = cast<ReturnStmt>(S)->getRetValue();
   FullExpressionRAII Scope(Info);
   if (RetExpr && RetExpr->isValueDependent()) {
     EvaluateDependentExpr(RetExpr, Info);
     // We know we returned, but we don't know what the value is.
     return ESR_Failed;
   }

We can't return a value we can't calculate right? and here as well for the Stmt::DoStmtClass case

if (DS->getCond()->isValueDependent()) {
   EvaluateDependentExpr(DS->getCond(), Info);
   // Bailout as we don't know whether to keep going or terminate the loop.
   return ESR_Failed;
 }

This case feels the same as the two above, we can't calculate the jump for the switch if we can't calculate the value.

CC @rsmith

[yronglin]

In D153296#4444612, @erichkeane wrote:

So I think I'm pretty confident that the only time we would call EvaluateDependentExpr is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I think returning early there is incorrect as well, at least from a 'code correctness' (even if there isn't a reproducer that would matter?). I think we're in a case where we want to continue in order to ensure we go through the entire flow, so I THINK we should treat this as 'we have a value we don't know, so its just not found', and should fall into the check on 5019 (unless of course, there is a 'default' option!).

So I think that we should be checking if Value is valid right after the default check, which lets us fall into the 'default' branch and get additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?

Erich, do you mean we do a modification like this？If I'm not misunderstand, I think this looks good to me, we can get more diagnostics.

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f1f89122d4cc..967695c61df5 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
       return ESR_Failed;
     if (SS->getCond()->isValueDependent()) {
       // Stop evaluate if condition expression contains errors.
-      if (SS->getCond()->containsErrors() ||
-          !EvaluateDependentExpr(SS->getCond(), Info))
+      if (!EvaluateDependentExpr(SS->getCond(), Info))
         return ESR_Failed;
     } else {
       if (!EvaluateInteger(SS->getCond(), Value, Info))
@@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
       return ESR_Failed;
   }
 
+  bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
+
   // Find the switch case corresponding to the value of the condition.
   // FIXME: Cache this lookup.
   const SwitchCase *Found = nullptr;
@@ -5009,7 +5010,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
     APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
     APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
                               : LHS;
-    if (LHS <= Value && Value <= RHS) {
+    if (!CondHasSideEffects && LHS <= Value && Value <= RHS) {
       Found = SC;
       break;
     }

[erichkeane]

In D153296#4446016, @shafik wrote:

In D153296#4444612, @erichkeane wrote:

So I think I'm pretty confident that the only time we would call EvaluateDependentExpr is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I think returning early there is incorrect as well, at least from a 'code correctness' (even if there isn't a reproducer that would matter?). I think we're in a case where we want to continue in order to ensure we go through the entire flow, so I THINK we should treat this as 'we have a value we don't know, so its just not found', and should fall into the check on 5019 (unless of course, there is a 'default' option!).

So I think that we should be checking if Value is valid right after the default check, which lets us fall into the 'default' branch and get additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?

I do see what you are saying but I think that we have similar cases where without a value the next step is impossible to do for example in EvaluateStmt the case Stmt::ReturnStmtClass: case:

case Stmt::ReturnStmtClass: {
   const Expr *RetExpr = cast<ReturnStmt>(S)->getRetValue();
   FullExpressionRAII Scope(Info);
   if (RetExpr && RetExpr->isValueDependent()) {
     EvaluateDependentExpr(RetExpr, Info);
     // We know we returned, but we don't know what the value is.
     return ESR_Failed;
   }

We can't return a value we can't calculate right? and here as well for the Stmt::DoStmtClass case

if (DS->getCond()->isValueDependent()) {
   EvaluateDependentExpr(DS->getCond(), Info);
   // Bailout as we don't know whether to keep going or terminate the loop.
   return ESR_Failed;
 }

This case feels the same as the two above, we can't calculate the jump for the switch if we can't calculate the value.

CC @rsmith

In the return-case I think it makes sense to skip out right away, I'm less convinced in the 'doStmt' case. Either way, I think we can get a somewhat 'better' diagnostic by continuing here, which is my point. We COULD start essentially compling with error-limit=1, but there is value to continuing when we can. And in this case, I think it is logical to continue.

In D153296#4446662, @yronglin wrote:

In D153296#4444612, @erichkeane wrote:

So I think I'm pretty confident that the only time we would call EvaluateDependentExpr is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK ignoring them right now.

So since we are in a state where ignoring this error-case is acceptable, I think returning early there is incorrect as well, at least from a 'code correctness' (even if there isn't a reproducer that would matter?). I think we're in a case where we want to continue in order to ensure we go through the entire flow, so I THINK we should treat this as 'we have a value we don't know, so its just not found', and should fall into the check on 5019 (unless of course, there is a 'default' option!).

So I think that we should be checking if Value is valid right after the default check, which lets us fall into the 'default' branch and get additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?

Erich, do you mean we do a modification like this？If I'm not misunderstand, I think this looks good to me, we can get more diagnostics.

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f1f89122d4cc..967695c61df5 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
       return ESR_Failed;
     if (SS->getCond()->isValueDependent()) {
       // Stop evaluate if condition expression contains errors.
-      if (SS->getCond()->containsErrors() ||
-          !EvaluateDependentExpr(SS->getCond(), Info))
+      if (!EvaluateDependentExpr(SS->getCond(), Info))
         return ESR_Failed;
     } else {
       if (!EvaluateInteger(SS->getCond(), Value, Info))
@@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
       return ESR_Failed;
   }
 
+  bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
+
   // Find the switch case corresponding to the value of the condition.
   // FIXME: Cache this lookup.
   const SwitchCase *Found = nullptr;
@@ -5009,7 +5010,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info,
     APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
     APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
                               : LHS;
-    if (LHS <= Value && Value <= RHS) {
+    if (!CondHasSideEffects && LHS <= Value && Value <= RHS) {
       Found = SC;
       break;
     }

Instead of the CondHasSideEffects, just check the validity of Value (which is a 1 bit zero value... WHICH is actually a valid value here unfortunately thanks to Bitint, so we'd have to check if the LHS/RHS bit size matches the Value here I think).. We shouldn't be using the HasSideEffects, because that is overly dependent on the implementation of EvaluateDependentExpr.

[yronglin]

Can we both check SS->getCond()->containsErrors() ? Maybe it can avoid bitint's effect. WDYT? @erichkeane @shafik

[erichkeane]

In D153296#4449089, @yronglin wrote:

Can we both check SS->getCond()->containsErrors() ? Maybe it can avoid bitint's effect. WDYT? @erichkeane @shafik

I'm a TOUCH uncomfortable in that this only fixes the 'current' issue, and is a little fragile perhaps? The problem we have is that the 'Value' is the incorrect size, which I presume we can get to a couple of ways? I'd like to make sure we fix as many of those problems as possible.

[yronglin]

Address comments

[yronglin]

Seems the diagnostic message <source>:5:9: note: constexpr evaluation hit maximum step limit; possible infinite loop? was redundant, also gcc dose not emit this message.

https://godbolt.org/z/v55P88cdT

[yronglin]

Please help me, I have no better idea on this issue, do you have any better ideas? @erichkeane @shafik

[yronglin]

friendly ping~

[aaron.ballman]

In D153296#4459769, @yronglin wrote:

Please help me, I have no better idea on this issue, do you have any better ideas? @erichkeane @shafik

I think what's being suggested is to change EvaluateDependentExpr() somewhat along these lines:

static bool EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
  assert(E->isValueDependent());

  // 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 SideEffects;
}

This way, code paths that get down to a RecoveryExpr will not continue to evaluate further (as there's really no point -- there's no way to get a reasonable value from from the recovery expression anyway), but the fix isn't specific to just switch statements. After making these changes, you should look for places where EvaluateDependentExpr() is being called to try to come up with a test case where that expression is a recovery expression so that we can fill out test coverage beyond just the situation with switch from the original report. Does that make sense?

(Marking as requesting changes so it's clear this review isn't yet accepted.)

[yronglin]

In D153296#4468373, @aaron.ballman wrote:

In D153296#4459769, @yronglin wrote:

Please help me, I have no better idea on this issue, do you have any better ideas? @erichkeane @shafik

I think what's being suggested is to change EvaluateDependentExpr() somewhat along these lines:

static bool EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
  assert(E->isValueDependent());

  // 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 SideEffects;
}

This way, code paths that get down to a RecoveryExpr will not continue to evaluate further (as there's really no point -- there's no way to get a reasonable value from from the recovery expression anyway), but the fix isn't specific to just switch statements. After making these changes, you should look for places where EvaluateDependentExpr() is being called to try to come up with a test case where that expression is a recovery expression so that we can fill out test coverage beyond just the situation with switch from the original report. Does that make sense?

(Marking as requesting changes so it's clear this review isn't yet accepted.)

Thanks a lot! @aaron.ballman , I try to address comments and add more test, this case (https://godbolt.org/z/ExPoEKcrf) looks strange, why the do-statement missing in the printed AST?

[yronglin]

Address comment.

• Change EvaluateDependentExpr.
• Add more test for do/while/for/return/ctor.

[hokein]

Thanks for the fix. Sorry for being late (I was looking through the emails and found this patch).

clang/lib/AST/ExprConstant.cpp

4901

The constant evaluator is not aware of the "error" concept, it is only aware of value-dependent -- the general idea behind is that we treat the dependent-on-error and dependent-on-template-parameters cases the same, they are potentially constant (if we see an expression contains errors, it could be constant depending on how the error is resolved), this will give us nice recovery and avoid bogus following diagnostics.

So, a RecoveryExpr should not result in failure when checking for a potential constant expression.

I think the right fix is to remove the conditional check if (!EvaluateDependentExpr(SS->getCond(), Info)) in EvaluateSwitch, and return ESR_Failed unconditionally (we don't know its value, any switch-case anwser will be wrong in some cases). We already do this for return-statment, do-statement etc.

clang/test/SemaCXX/constexpr-function-recovery-crash.cpp

91 ↗

(On Diff #536802)

nit: we can simplify it with the TEST_EVALUATE macro:

TEST_EVALUATE(SwitchErrorCond, switch(undef) { case 0: return 7; default: break;})

93 ↗

(On Diff #536802)

Is this a new crash (and the tests below)?

They don't look like new crashes, I think the current constant evaluator should be able to handle them well. IIUC the only crash we have is the case where we have a error-dependent condition in switch?

[yronglin]

Thanks a lot for your comments! @hokein

clang/lib/AST/ExprConstant.cpp

4901

Do you mean?

if (SS->getCond()->isValueDependent()) {
    EvaluateDependentExpr(SS->getCond(), Info);
    return ESR_Failed;
}

clang/test/SemaCXX/constexpr-function-recovery-crash.cpp

91 ↗

(On Diff #536802)

Thanks, I will use TEST_EVALUATE to simplify.

93 ↗

(On Diff #536802)

Thanks you for catch this, it's my mistake, I have ever copied these tests from the code above.

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

4901

the general idea behind is that we treat the dependent-on-error and dependent-on-template-parameters cases the same, they are potentially constant (if we see an expression contains errors, it could be constant depending on how the error is resolved), this will give us nice recovery and avoid bogus following diagnostics.

I could use some further education on why this is the correct approach. For a dependent-on-template-parameters case, this makes sense -- either the template will be instantiated (at which point we'll know if it's a constant expression) or it won't be (at which point it's constant expression-ness doesn't matter). But for error recovery, we will *never* get a valid constant expression.

I worry about the performance overhead of continuing on with constant expression evaluation in the error case. We use these code paths not only to get a value but to say "is this a constant expression at all?".

I don't see why the fix should be localized to just the switch statement condition; it seems like *any* attempt to get a dependent value from an error recovery expression is a point at which we can definitively say "this is not a constant expression" and move on.

[yronglin]

clang/lib/AST/ExprConstant.cpp

4901

I understand that continuing to perform constant evaluation when an error occurs can bring more additional diagnostic information (such as jumping to the default branch to continue calculation when the condition expression evaluation of switch-statement fails), but the additional diagnostic message that is emitted is in some cases doesn't usually useful, and as Aaron said may affect performance of clang. I don't have enough experience to make a tradeoff between the two. BTW https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909 I don't quite understand why a RecoveryExpr is not created here, which caused to the whole do statement disappears on the AST(https://godbolt.org/z/PsPb31YKP), should we fix this?

[yronglin]

clang/lib/AST/ExprConstant.cpp

4901

Thanks a lot for your comments! @aaron.ballman

[hokein]

clang/lib/AST/ExprConstant.cpp

4901

But for error recovery, we will *never* get a valid constant expression.

Yeah, this is true, and we don't evaluate these error-dependent expressions.

I think the question here is that when we encounter an error-dependent expression during a constant expression evaluation, do we want to bailout the whole evaluation immediately, or just ignore the evaluation of this error-dependent expression and continue to proceed when possible?

We choose 2) -- this was based on the discussion with @rsmith. This will likely give us decent error-recovery and useful followup diagnostics.

Some concrete examples,

int abc();
constexpr int f() { 
  error(); 
  // We emit a diagnostic "Constexpr function never produces a constant expression, because abc() cannot be used in a constant expression"
  return abc(); 
}

// We prefer to treat the function f as a constexpr function even though it has an error expression. We will preserve more information in the AST, e.g. clangd's hover on the function call `f()` can give you the return value 1.

constexpr int f() {
   error();
   return 1;
}

I worry about the performance overhead of continuing on with constant expression evaluation in the error case. We use these code paths not only to get a value but to say "is this a constant expression at all?".

Yeah, performance maybe a valid concern, but I don't have any data. These code paths are also used for generating diagnostics. I think this is a cost we pay for having nice error recovery.
If the performance is a real issue here, we probably need to figure out bottlenecks before coming up solutions.

I don't see why the fix should be localized to just the switch statement condition; it seems like *any* attempt to get a dependent value from an error recovery expression is a point at which we can definitively say "this is not a constant expression" and move on.

Right.

Sorry for not being clear, I didn't mean the fix should be localized to switch-statement condition only, we already do it for the condition for for, while etc -- whenever we encounter a value-dependent condition, we bailout unconditionally, the switch-statement seems to be an oversight in https://reviews.llvm.org/D113752 (sorry for not catching it earlier during the review).

4901

Do you mean?

if (SS->getCond()->isValueDependent()) {

EvaluateDependentExpr(SS->getCond(), Info);
return ESR_Failed;

}

Yeah, this is what I meant.

4901

BTW https://github.com/llvm/llvm-project/blob/843ff7581408a02e852c0f1f7ebf176cabbc7527/clang/lib/Parse/ParseStmt.cpp#L1894-L1909 I don't quite understand why a RecoveryExpr is not created here, which caused to the whole do statement disappears on the AST(https://godbolt.org/z/PsPb31YKP), should we fix this?

RecoveryExpr is created in an ad-hoc way, there are cases we might miss. It would be nice to fix it so that clang can preserve more AST nodes for broken code. But this is different issue, no need to address it here.

I think we run into the if (Cond.isUsable()) branch (rather than the else branch), the Cond expression is a TypoExpr, and we fail to resolve the TypoExpr, so the whole do-while statement get discarded during the parsing.

I think we can probably fix it by passing a true RecoverUncorrectedTypos flag in the CorrectDelayedTyposInExpr call there -- the TypoExpr will fallback to RecoveryExpr if clang's TypoCorrection fails to resolve it.

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

4901

But for error recovery, we will *never* get a valid constant expression.

Yeah, this is true, and we don't evaluate these error-dependent expressions.

That's why I think EvaluateDependentExpr() should return false when we *are* evaluating these error-dependent expressions. When we get to the point of saying "I need this value", we're saying "I don't know what the value is but keep going just in case" and that's a potentially significant hit on compile time. Consider: https://godbolt.org/z/cGWrY3fa7 -- the slow case takes about twice as long to compute as the fast case and the diagnostic behavior is the same.

I think the question here is that when we encounter an error-dependent expression during a constant expression evaluation, do we want to bailout the whole evaluation immediately, or just ignore the evaluation of this error-dependent expression and continue to proceed when possible?

I guess I fall on the side of "fail quickly so long as the failure is actionable". I don't see a lot of benefit to the follow-on diagnostics because a RecoveryExpr is always fatal anyway and I expect it's more frequent for there to be one compile error in a constant expression than it is for there to be several unrelated compile errors, but that's based on intuition and not anything I've measured.

Constant expression evaluation and template instantiation absolutely murder compile times in C++, so we need to be careful about the overhead tradeoffs between follow-on diagnostics and compile times. For example, if the constant expression is being used in a SFINAE context, the extra follow-on diagnostics are always overhead without benefit because that template won't instantiate anyway (and that could very well be expected behavior for the given template arguments).

If the performance is a real issue here, we probably need to figure out bottlenecks before coming up solutions.

Two issues: 1) RecoveryExpr keeps showing up in places where it's not expected and we're having to play whack-a-mole with fixing the resulting crashes. 2) Performance concerns. I think #2 is being handled in other ways, but #1 is what really worries me given that it's often not obvious what the right answer is.

[shafik]

clang/lib/AST/ExprConstant.cpp

4989

Please don't forget to remove this if and make the return unconditional as reinforced by @hokein comment above.

[hokein]

clang/lib/AST/ExprConstant.cpp

4901

Consider: https://godbolt.org/z/cGWrY3fa7 -- the slow case takes about twice as long to compute as the fast case and the diagnostic behavior is the same.

Thanks for the example, but I'm not sure I'm convinced -- I think the slow part is coming from the constexpr function itself -- it has a heavy while-loop in the implementation:

1. for correct code f(0x7FFF'FFFF'FFFF'FFFF, 1), it is slow
2. for error code f(0x7FFF'FFFF'FFFF'FFFF, 0), it is slow

There is no performance difference between 1) and 2), compiling the correct code is already slow. And we have the constexpr_step_limit during the constant evaluation, which gives us a upper-bound compiling time.

And yeah, we can save some cost by skipping the heady loop if we encounter any fatal errors in advance, but this only speeds up error cases (with the cost of downgrading diagnostics), I guess I don't get the point why the error-code case is more special than the correct-code case, and we just do the optimization for it.

IMO, for error code, I think diagnostics are important for error-code, it gives users information about the wrong places about their code. I'd image users spend most of time on reading these diagnostics and fixing their code

I guess I fall on the side of "fail quickly so long as the failure is actionable".

I have some concerns:

1. downgrading diagnostics -- imaging there are N fatal errors, now we have to compile the code N times in order to view&fix them (which will take more time in total). Moreover, it doesn't seem to align with what clang does today (clang doesn't stop parsing when encountering the first fatal error because of performance, it tries the best to recover from the error and emit as many useful diagnostics as possible in a single run in order to provide nice diagnostics).

2. it only fixes error-dependent crashes -- ideally, the value-dependent mechanism in the constant evaluator is designed to handle normal template-dependent code and error-dependent code, though the template-dependent code part is not implemented yet (see the FIXME), we will likely encounter similar crashes if we only special-case the error-code.

Two issues: 1) RecoveryExpr keeps showing up in places where it's not expected and we're having to play whack-a-mole with fixing the resulting crashes. 2) Performance concerns. I think #2 is being handled in other ways, but #1 is what really worries me given that it's often not obvious what the right answer is.

For #1, I'm sorry for the issues caused by RecoveryExpr (and thanks for all the hard work you have done in tracking, reviewing, and fixing these issues).

I think the fundamental reason is we now keep more AST nodes that would otherwise be discarded without RecoveryExpr, these AST nodes are similar to regular dependent nodes, and have weaker invariants (e.g. we can now have value-dependent expressions outside of dependent contexts), some clang codepaths don't handle them well, we have to teach these codepaths.
And I want to emphasize that these fixes are not just ad-hoc fixes, they also improve the code health of clang infrastructure in the long run which enables technical debt removal etc.

It is frustrating to see these crashes occur from time to time, I will like to improve the situation here, I don't have a better solution to find out all of them, except fixing them case-by-case (internally we have daily crash clangd report, we keep monitoring it and fixing highly-frequent crashes).

If you see any related issues in the future, feel free to ping me as I might miss them, I'm happy to fix them or provide any help.

For 2), it seems to be a known issue of the current constant evaluator, there are other attempts (e.g. https://www.redhat.com/en/blog/new-constant-expression-interpreter-clang) to address the performance issue .

[yronglin]

Address comments.

[yronglin]

Many thanks for all of your comments, I learned a lot from the discussions, your incredible depth of knowledge have helped fundamentally shaped Clang into a great compiler!
It seems the common denominator is that constant evaluation should stop when we encounter a value dependent expression, and return ESR_Failed.

clang/lib/AST/ExprConstant.cpp

4989

Thanks for your tips!, removed!

[rsmith]

Making the return ESR_Failed; unconditional looks to be the correct change here. We can't continue evaluation past that point because we don't know what would be executed next. Unconditionally returning ESR_Failed in that situation is what the other similar paths through EvaluateStmt do.

clang/lib/AST/ExprConstant.cpp

4890

I don't think the changes to this function are appropriate, because:

1. The special-casing of RecoveryExpr doesn't seem like it can be correct. There's no guarantee that we get a RecoveryExpr any time we encounter an expression that contains errors; error-dependence can be propagated from other places, such as types.
2. For other error-dependent expressions, we also can't necessarily compute a value.
3. It's not the responsibility of this function to deal with the situation where a value is needed and can't be produced -- the responsibility to handle that lies with the caller of this function instead. Eg, look at the handling of ReturnStmt or DoStmt.

So I think we should undo all the changes in this function, and only fix SwitchStmt to properly handle a value-dependent condition.

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

4890

Thank you for the explanation -- this makes more sense to me now. I agree with the suggestion to just change EvaluateSwitch(), sorry for the false start!

[yronglin]

Address comments

[yronglin]

Format

[yronglin]

Thanks for your review!

clang/lib/AST/ExprConstant.cpp

4890

Thanks! I have undo this change.

[hokein]

Thanks, this looks good.

[yronglin]

In D153296#4479718, @hokein wrote:

Thanks, this looks good.

Thanks for your review! I don't know why the reversion status still Needs Review, and the libcxx ci often fails to start.

[hokein]

In D153296#4480141, @yronglin wrote:

In D153296#4479718, @hokein wrote:

Thanks, this looks good.

Thanks for your review! I don't know why the reversion status still Needs Review, and the libcxx ci often fails to start.

I guess the Needs Review is probably caused by the previous "Requested Changes" by Aaron.
The libcxx ci failure doesn't seem to be relevant. I think we're all on the same page about the fix, it is fine to land it assuming that the ninja check-clang passes.

[aaron.ballman]

LGTM. thank you!

In D153296#4480243, @hokein wrote:

In D153296#4480141, @yronglin wrote:

In D153296#4479718, @hokein wrote:

Thanks, this looks good.

Thanks for your review! I don't know why the reversion status still Needs Review, and the libcxx ci often fails to start.

I guess the Needs Review is probably caused by the previous "Requested Changes" by Aaron.

Correct.

The libcxx ci failure doesn't seem to be relevant. I think we're all on the same page about the fix, it is fine to land it assuming that the ninja check-clang passes.

Agreed, the libcxx failure is unrelated.

[yronglin]

Thanks, landed! I have benefited a lot from your comments!

clang/lib/AST/ExprConstant.cpp

static bool EvaluateDecl(EvalInfo &Info, const Decl *D) {
   if (const DecompositionDecl *DD = dyn_cast<DecompositionDecl>(D))
     for (auto *BD : DD->bindings())
       if (auto *VD = BD->getHoldingVar())
         OK &= EvaluateDecl(Info, VD);
 
   return OK;
 }
 
 static bool EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
   assert(E->isValueDependent());
+  // Stop evaluate if E is a RecoveryExpr.
+  if (isa<RecoveryExpr>(E))
+    return false;
   if (Info.noteSideEffect())
     return true;
   assert(E->containsErrors() && "valid value-dependent expression should never "
                                 "reach invalid code path.");
   return false;
 }
 
 /// Evaluate a condition (either a variable declaration or an expression).
 static bool EvaluateCond(EvalInfo &Info, const VarDecl *CondDecl,
                          const Expr *Cond, bool &Result) {
   if (Cond->isValueDependent())
     return false;
   FullExpressionRAII Scope(Info);
   if (CondDecl && !EvaluateDecl(Info, CondDecl))
     return false;
if (const Stmt *Init = SS->getInit()) {
       }
     }
 
     FullExpressionRAII CondScope(Info);
     if (SS->getConditionVariable() &&
         !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;
     }
     if (!CondScope.destroy())
       return ESR_Failed;
   }
 
   // Find the switch case corresponding to the value of the condition.

clang/test/SemaCXX/switch.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
 
 void test() {
   bool x = true;
   switch (x) { // expected-warning {{bool}}
     case 0:
       break;
   }
 
void test(Empty e) {
   }
   switch (e) {
   default:
     break;
   }
 }
 
 } // namespace EmptyEnum
+
+#if __cplusplus >= 201703L
+constexpr int foo(unsigned char c) {
+    switch (unknown_value) { // expected-error {{use of undeclared identifier}}
+    case 0:
+        return 7;
+    default:
+        break;
+    }
+    return 0;
+}
+
+static_assert(foo('d')); // expected-error {{static assertion expression is not an integral constant expression}}
+#endif