[AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.

[hokein]

Summary

crash stack:

lang:  workspace/llvm-project/clang/lib/AST/ExprConstant.cpp:13704: bool EvaluateInPlace(clang::APValue &, (anonymous namespace)::EvalInfo &, const (anonymous namespace)::LValue &, const clang::Expr *, bool): Assertion `!E->isValueDependent()' failed.
 #8  EvaluateInPlace(clang::APValue&, (anonymous namespace)::EvalInfo&, (anonymous namespace)::LValue const&, clang::Expr const*, bool)  workspace/llvm-project/clang/lib/AST/ExprConstant.cpp:0:0
 #9  HandleConstructorCall(clang::Expr const*, (anonymous namespace)::LValue const&, clang::APValue*, clang::CXXConstructorDecl const*, (anonymous namespace)::EvalInfo&, clang::APValue&)  workspace/llvm-project/clang/lib/AST/ExprConstant.cpp:5779:57
#10  HandleConstructorCall(clang::Expr const*, (anonymous namespace)::LValue const&, llvm::ArrayRef<clang::Expr const*>, clang::CXXConstructorDecl const*, (anonymous namespace)::EvalInfo&, clang::APValue&)  workspace/llvm-project/clang/lib/AST/ExprConstant.cpp:5819:10
#11  clang::Expr::isPotentialConstantExpr(clang::FunctionDecl const*, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic> >&) workspace/llvm-project/clang/lib/AST/ExprConstant.cpp:14746:5
#12  CheckConstexprFunctionBody(clang::Sema&, clang::FunctionDecl const*, clang::Stmt*, clang::Sema::CheckConstexprKind)  workspace/llvm-project/clang/lib/Sema/SemaDeclCXX.cpp:2306:7
#13  clang::Sema::CheckConstexprFunctionDefinition(clang::FunctionDecl const*, clang::Sema::CheckConstexprKind)  workspace/llvm-project/clang/lib/Sema/SemaDeclCXX.cpp:1766:0
#14  clang::Sema::ActOnFinishFunctionBody(clang::Decl*, clang::Stmt*, bool)  workspace/llvm-project/clang/lib/Sema/SemaDecl.cpp:14357:9
#15  clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&)  workspace/llvm-project/clang/lib/Parse/ParseStmt.cpp:2213:18

[hokein]

clang/lib/Sema/SemaDecl.cpp

14359 ↗

(On Diff #253521)

The crash is in CheckConstexprFunctionDefinition, I tried different ways to fixing it:

1. mark FD invalid when there are any errors in CtorInitailizer -- clang deliberately treats CtorDecl as valid even there are some errors in the initializer to prevent spurious diagnostics (see the cycle delegation in the test as an example), so marking them invalid may affect the quality of diagnostics;

2. Fixing it inside CheckConstexprFunctionDefinition or isPotentialConstantExpr, but it doesn't seem to be a right layer, these functions are expected to be called on a validDecl (or at least after a few sanity checks), and emit diagnostics.

[sammccall]

This makes me nervous, marking the constructor as invalid seems much safer. Can you show tests it regresses?

clang/lib/Sema/SemaDecl.cpp

14359 ↗

(On Diff #253521)

clang deliberately treats CtorDecl as valid even there are some errors in the initializer

Do you mean currently? (when such errors result in destroying the whole init expr)
If so, it would be nice to preserve this indeed.

I'm worried that we're going to decide that a constexpr constructor is valid and then actually try to evaluate it at compile time.
What happens if we try to evaluate constexpr int Z = X().Y in your testcase?

Fixing it inside CheckConstexprFunctionDefinition

I have a couple of concerns with where it's currently implemented:

• aren't there lots of other paths we're going to call isPotentialConstantExpr and crash? CheckConstexprFunctionDefinition is large and complicated, init-lists are only a small part.
• if we treat anything with errors in the ctor as valid (am I reading that right?!) then won't that mask *other* problems where the non-error parts of the definition should cause it to be treated as invalid? e.g. Foo() : m1(broken_but_maybe_constexpr()), m2(exists_and_not_constexpr()) {}

Wherever this goes, if we're going to do something subtle like marking diagnostics as valid based on errors in them, it deserves a comment laying out the cases.

clang/test/SemaCXX/invalid-constructor-init.cpp

18

what's the behavior with -fno-recovery-ast?
(It'd be nice to improve it, failing to improve it is OK. regressing is probably bad...)

[hokein]

• mark CtorDecl as invalid when the Initializer init expr contains errors
• add a testcase that would crash the previous version of the patch

[hokein]

This makes me nervous, marking the constructor as invalid seems much safer. Can you show tests it regresses?

Yeah, the first version of the patch doesn't seem to fix all crashes (X().Y would lead another crash)...

so if we mark the CtorDecl as invalid

1. whenever CtorInitializer->init()->containsError, we don't have failure tests
2. whenever there is any kind of errors (including the containsError case above) in CtorInitailizer, we have three failing tests (SemaCXX/constant-expression-cxx11.cpp, SemaCXX/constructor-initializer.cpp, SemaTemplate/constexpr-instantiate.cpp).

though 1) passes all existing tests, I think it just means current tests don't have enough coverage for recoveryExpr cases. But given the current state, 1) looks most promising -- fixes the crashes, retains broken expressions in CtorInitializer rather than dropping them, doesn't regress the diagnostics a lot (only for CtorInitializer->init()->containsError` case).

[sammccall]

clang/lib/Sema/SemaDeclCXX.cpp

5004 ↗

(On Diff #253809)

what's the case where this gets hit rather than anyerrors=true?

[hokein]

clang/lib/Sema/SemaDeclCXX.cpp

5004 ↗

(On Diff #253809)

no cases, if Member->getInit()->containsErrors() is true, then anyerrors is always true (this is done in ParseDeclCXX.cpp).

the reason why we added the check here is that we mark the constructor as invalid only for case Member->getInit()->containsErrors() (not for other cases leading anyerrors to true)

[rsmith]

clang/lib/Parse/ParseDeclCXX.cpp

3459–3461 ↗

(On Diff #253809)

The parser should not be inspecting properties of Exprs -- that's a layering violation. Can you move this logic into ActOnMemInitializers instead?

clang/lib/Sema/SemaDeclCXX.cpp

1688 ↗

(On Diff #253809)

Instead of asserting this, please just return false on an invalid declaration. There's nothing fundamentally wrong with calling this function on an invalid declaration, but as usual, if we encounter something invalid, we should suppress follow-on diagnostics.

5005 ↗

(On Diff #253809)

This is inappropriate. The "invalid" bit on a declaration indicates whether the declaration is invalid, not whether the definition is invalid.

What we should do is add a "contains errors" bit to Stmt, and mark the function body as containing errors if it has an initializer that contains an error.

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

5005 ↗

(On Diff #253809)

As an example of why this distinction matters: the "contains errors" bit indicates whether external users of the declaration should ignore it / suppress errors on it, or whether they can still treat it as a regular declaration. It's not ideal for an error in the body of a function to affect the diagnostic behavior of a caller to that function, since the body is (typically) irrelevant to the validity of that caller.

[hokein]

clang/lib/Sema/SemaDeclCXX.cpp

1688 ↗

(On Diff #253809)

oh, thanks. This function is only called in two places, and both of them check the NewFD->isValid before calling it, so my understanding is that this function is required a valid NewFD.

5005 ↗

(On Diff #253809)

Thanks for the suggestions and clarifications! The "invalid" bit of decl is subtle, I didn't infer it from the code before, thanks for the explanation.

Setting the decl invalid seemed much safer to us, and would avoid running a lot of unexpected code paths in clang which might violate the assumptions.

What we should do is add a "contains errors" bit to Stmt, and mark the function body as containing errors if it has an initializer that contains an error.

This sounds promising, but there are no free bit in Stmt at the moment :( (to add the ErrorBit to expr, we have sacrificed the IsOMPStructuredBlock bit, it would be easier after the ongoing FPOptions stuff finished, which will free certain bits).

since this crash is blocking recovery-expr stuff, it is prioritized for us to fix it. Maybe a compromising plan for now is to fix it in CheckConstexprFunctionDefinition (just moving the inspecting InitExpr code there) -- the crash is from isPotentialConstantExpr which is only called in CheckConstexprFunctionDefinition, should cover all cases.

[sammccall]

clang/lib/Sema/SemaDeclCXX.cpp

5005 ↗

(On Diff #253809)

I don't think the availability or otherwise of a bit in stmt is the critical factor here. Adding a bit to stmt will be a huge amount of work because (unlike expr/type) stmts don't have dependence, so there's no existing dependence propagation/handling to reuse.

When a ctor has an init expr with an error, I think we ultimately need to choose between:

1. ctor is constant-evaluable. (This seems obviously wrong)
2. ctor is not-constant-evaluable. (This creates spurious "must be a constant expression" diags)
3. ctor is invalid and therefore we never ask. (This creates other spurious diags due to not finding the ctor)
4. ctor-with-errors is handled specially (we find it but don't check it for constant-evaluability).

Adding a stmt bit is 4, and is certainly the best long-term direction. 2 seems like a reasonable short-term solution though. Can we modify the is-constexpr check to return false if errors are present, before asserting there's no dependent code?

[hokein]

refine the fix based on the comment:

now constexpr ctor with error initializers is still a valid decl but not
constant-evaluate.

[hokein]

remove accident change.

clang/lib/AST/ExprConstant.cpp

if (!Info.Ctx.getLangOpts().CPlusPlus2a && isa<CXXMethodDecl>(Declaration) &&
       cast<CXXMethodDecl>(Declaration)->isVirtual())
     Info.CCEDiag(CallLoc, diag::note_constexpr_virtual_call);
 
   if (Definition && Definition->isInvalidDecl()) {
     Info.FFDiag(CallLoc, diag::note_invalid_subexpr_in_const_expr);
     return false;
   }
 
+  if (const auto *CtorDecl = dyn_cast_or_null<CXXConstructorDecl>(Definition)) {
+    for (const auto *InitExpr : CtorDecl->inits()) {
+      if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+        return false;
+    }
+  }
+
   // Can we evaluate this function call?
   if (Definition && Definition->isConstexpr() && Body)
     return true;
 
   if (Info.getLangOpts().CPlusPlus11) {
     const FunctionDecl *DiagDecl = Definition ? Definition : Declaration;
 
     // If this function is not constexpr because it is an inherited
bool Expr::isPotentialConstantExpr(const FunctionDecl *FD,
                                    SmallVectorImpl<
                                      PartialDiagnosticAt> &Diags) {
   // FIXME: It would be useful to check constexpr function templates, but at the
   // moment the constant expression evaluator cannot cope with the non-rigorous
   // ASTs which we build for dependent expressions.
   if (FD->isDependentContext())
     return true;
 
+  // Bail out if a constexpr constructor has an initializer that contains an
+  // error. We deliberately don't produce a diagnostic, as we have produced a
+  // relevant diagnostic when parsing the error initializer.
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(FD)) {
+    for (const auto *InitExpr : Ctor->inits()) {
+      if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+        return false;
+    }
+  }
   Expr::EvalStatus Status;
   Status.Diag = &Diags;
 
   EvalInfo Info(FD->getASTContext(), Status, EvalInfo::EM_ConstantExpression);
   Info.InConstantContext = true;
   Info.CheckingPotentialConstantExpression = true;
 
   // The constexpr VM attempts to compile all methods to bytecode here.

clang/test/SemaCXX/invalid-constructor-init.cpp

+// RUN: %clang_cc1 -frecovery-ast -verify %s
+
+struct X {
+  int Y;
+  constexpr X() // expected-error {{constexpr constructor never produces}}
+      : Y(foo()) {} // expected-error {{use of undeclared identifier 'foo'}}
+};
+// no crash on evaluating the constexpr ctor.
+constexpr int Z = X().Y; // expected-error {{constexpr variable 'Z' must be initialized by a constant expression}}
+
+struct X2 {
+  int Y = foo();    // expected-error {{use of undeclared identifier 'foo'}} \
+                 // expected-note {{subexpression not valid in a constant expression}}
+  constexpr X2() {} // expected-error {{constexpr constructor never produces a constant expression}}
+};
+
+struct CycleDelegate {
+  int Y;
+  CycleDelegate(int)
+      : Y(foo()) {} // expected-error {{use of undeclared identifier 'foo'}}
+  // no bogus "delegation cycle" diagnostic
+  CycleDelegate(float) : CycleDelegate(1) {}
+};