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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked 2 inline comments as done.
hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1688
                                             CheckConstexprKind Kind) {
+  assert(!NewFD->isInvalidDecl());
   const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD);
----------------
rsmith wrote:
> 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.
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.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005
+    if (Member->getInit() && Member->getInit()->containsErrors())
+      Constructor->setInvalidDecl();
     if (Member->isBaseInitializer())
----------------
rsmith wrote:
> rsmith wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77041