Page MenuHomePhabricator

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

Authored by hokein on Mar 30 2020, 1:18 AM.



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

Diff Detail

Event Timeline

hokein created this revision.Mar 30 2020, 1:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 1:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein edited the summary of this revision. (Show Details)Mar 30 2020, 1:19 AM
hokein added inline comments.Mar 30 2020, 1:30 AM
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;
  1. 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.

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

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.


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 updated this revision to Diff 253809.Mar 31 2020, 1:44 AM
hokein marked an inline comment as done.
  • mark CtorDecl as invalid when the Initializer init expr contains errors
  • add a testcase that would crash the previous version of the patch

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 accepted this revision.Mar 31 2020, 6:20 AM
sammccall added inline comments.
5004 ↗(On Diff #253809)

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

This revision is now accepted and ready to land.Mar 31 2020, 6:20 AM
hokein marked an inline comment as done.Mar 31 2020, 7:17 AM
hokein added inline comments.
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 added a subscriber: rsmith.Mar 31 2020, 1:03 PM
rsmith added inline comments.
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?

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 added inline comments.Mar 31 2020, 1:06 PM
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 marked 2 inline comments as done.Apr 1 2020, 2:15 AM
hokein added inline comments.
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 added inline comments.Apr 1 2020, 2:31 AM
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 updated this revision to Diff 254170.Apr 1 2020, 5:18 AM

refine the fix based on the comment:

now constexpr ctor with error initializers is still a valid decl but not

hokein updated this revision to Diff 254525.Apr 2 2020, 7:36 AM

remove accident change.

sammccall accepted this revision.Apr 7 2020, 3:19 AM
This revision was automatically updated to reflect the committed changes.