[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG041080c24735: [AST] Fix a crash on invalid constexpr
Ctorinitializer when building… (authored by hokein).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/SemaCXX/invalid-constructor-init.cpp
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4976,6 +4976,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(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;
@@ -14709,6 +14716,15 @@
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(FD)) {
+for (const auto *InitExpr : Ctor->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
Expr::EvalStatus Status;
Status.Diag = &Diags;
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4976,6 +4976,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(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;
@@ -14709,6 +14716,15 @@
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(FD)) {
+for (const auto *InitExpr : Ctor->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
Expr::EvalStatus Status;
Status.Diag = &Diags;
___
cfe-commits mailing list
cfe-commits@lists
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein updated this revision to Diff 254525.
hokein added a comment.
remove accident change.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/SemaCXX/invalid-constructor-init.cpp
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4975,6 +4975,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(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;
@@ -14736,6 +14743,15 @@
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(FD)) {
+for (const auto *InitExpr : Ctor->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
Expr::EvalStatus Status;
Status.Diag = &Diags;
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4975,6 +4975,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(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;
@@ -14736,6 +14743,15 @@
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(FD)) {
+for (const auto *InitExpr : Ctor->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
Expr::EvalStatus Status;
Status.Diag = &Diags;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein updated this revision to Diff 254170.
hokein added a comment.
refine the fix based on the comment:
now constexpr ctor with error initializers is still a valid decl but not
constant-evaluate.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
Files:
clang/lib/AST/ExprConstant.cpp
clang/lib/Sema/SemaDecl.cpp
clang/test/SemaCXX/invalid-constructor-init.cpp
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/Template.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Triple.h"
+#include "llvm/Support/Casting.h"
#include
#include
#include
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4975,6 +4975,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(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;
@@ -14736,6 +14743,15 @@
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(FD)) {
+for (const auto *InitExpr : Ctor->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
Expr::EvalStatus Status;
Status.Diag = &Diags;
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// 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) {}
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/Template.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Triple.h"
+#include "llvm/Support/Casting.h"
#include
#include
#include
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4975,6 +4975,13 @@
return false;
}
+ if (const auto *CtorDecl = dyn_cast_or_null(Definition)) {
+for (const auto *InitExpr : CtorDecl->inits()) {
+ if (InitExpr->getInit() && InitExpr->getInit()->containsErrors())
+return false;
+}
+ }
+
// Can we evaluate t
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
sammccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005 +if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); if (Member->isBaseInitializer()) hokein wrote: > 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. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005 +if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); if (Member->isBaseInitializer()) 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
rsmith added inline comments.
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3459-3461
+ if (MemInit.get()->getInit() &&
+ MemInit.get()->getInit()->containsErrors())
+AnyErrors = true;
The parser should not be inspecting properties of `Expr`s -- that's a layering
violation. Can you move this logic into `ActOnMemInitializers` instead?
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1688
CheckConstexprKind Kind) {
+ assert(!NewFD->isInvalidDecl());
const CXXMethodDecl *MD = dyn_cast(NewFD);
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.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5005
+if (Member->getInit() && Member->getInit()->containsErrors())
+ Constructor->setInvalidDecl();
if (Member->isBaseInitializer())
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein marked an inline comment as done. hokein added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5004 CXXCtorInitializer *Member = Initializers[i]; - +if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); sammccall wrote: > what's the case where this gets hit rather than anyerrors=true? 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) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5004 CXXCtorInitializer *Member = Initializers[i]; - +if (Member->getInit() && Member->getInit()->containsErrors()) + Constructor->setInvalidDecl(); what's the case where this gets hit rather than anyerrors=true? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein updated this revision to Diff 253809.
hokein marked an inline comment as done.
hokein added a comment.
- mark CtorDecl as invalid when the Initializer init expr contains errors
- add a testcase that would crash the previous version of the patch
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
Files:
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/invalid-constructor-init.cpp
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -frecovery-ast -verify %s
+
+struct X {// expected-note 2{{candidate constructor }}
+ int Y;
+ constexpr X() : Y(foo()) {} // expected-error {{use of undeclared identifier
'foo'}}
+};
+// no crash on evaluating the constexpr ctor.
+// FIXME: get rid of the bogus diagnostic below.
+constexpr int Z = X().Y; // expected-error {{no matching constructor for
initialization of 'X'}}
+
+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'}}
+ // FIXME: get rid of the bogus "delegation cycle" diagnostic
+ // CycleDeclegate(int) is marked as invalid.
+ CycleDelegate(float) : CycleDelegate(1) {} // expected-error {{creates a
delegation cycle}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -1685,6 +1685,7 @@
// This implements C++11 [dcl.constexpr]p3,4, as amended by DR1360.
bool Sema::CheckConstexprFunctionDefinition(const FunctionDecl *NewFD,
CheckConstexprKind Kind) {
+ assert(!NewFD->isInvalidDecl());
const CXXMethodDecl *MD = dyn_cast(NewFD);
if (MD && MD->isInstance()) {
// C++11 [dcl.constexpr]p4:
@@ -5000,7 +5001,8 @@
for (unsigned i = 0; i < Initializers.size(); i++) {
CXXCtorInitializer *Member = Initializers[i];
-
+if (Member->getInit() && Member->getInit()->containsErrors())
+ Constructor->setInvalidDecl();
if (Member->isBaseInitializer())
Info.AllBaseFields[Member->getBaseClass()->getAs()] = Member;
else {
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/Template.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Triple.h"
+#include "llvm/Support/Casting.h"
#include
#include
#include
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -3454,9 +3454,12 @@
}
MemInitResult MemInit = ParseMemInitializer(ConstructorDecl);
-if (!MemInit.isInvalid())
+if (!MemInit.isInvalid()) {
MemInitializers.push_back(MemInit.get());
-else
+ if (MemInit.get()->getInit() &&
+ MemInit.get()->getInit()->containsErrors())
+AnyErrors = true;
+} else
AnyErrors = true;
if (Tok.is(tok::comma))
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -frecovery-ast -verify %s
+
+struct X {// expected-note 2{{candidate constructor }}
+ int Y;
+ constexpr X() : Y(foo()) {} // expected-error {{use of undeclared identifier 'foo'}}
+};
+// no crash on evaluating the constexpr ctor.
+// FIXME: get rid of the bogus diagnostic below.
+constexpr int Z = X().Y; // expected-error {{no matching constructor for initialization of 'X'}}
+
+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'}}
+ // FIXME: get rid of the bogus "delegation cycle" diagnostic
+ // CycleDeclegate(int) is marked as invalid.
+ CycleDelegate(float) : CycleDelegate(1) {} // expected-error {{creates a delegation cycle}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clan
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein added a comment. > 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
sammccall added a comment.
This makes me nervous, marking the constructor as invalid seems much safer. Can
you show tests it regresses?
Comment at: clang/lib/Sema/SemaDecl.cpp:14359
+!ErrorsInCtorInitializer &&
!CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
FD->setInvalidDecl();
hokein wrote:
> 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.
> 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.
Comment at: clang/test/SemaCXX/invalid-constructor-init.cpp:17
+ CycleDelegate(int) : Y(foo()) {} // expected-error {{use of undeclared
identifier 'foo'}}
+ // no "delegation cycle" diagnostic emitted!
+ CycleDelegate(float) : CycleDelegate(1) {}
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...)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77041/new/
https://reviews.llvm.org/D77041
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14359 +!ErrorsInCtorInitializer && !CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose)) FD->setInvalidDecl(); 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77041/new/ https://reviews.llvm.org/D77041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77041: [AST] Fix a crash on invalid constexpr Ctorinitializer when building RecoveryExpr.
hokein created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
hokein edited the summary of this revision.
hokein added inline comments.
hokein added a reviewer: sammccall.
Comment at: clang/lib/Sema/SemaDecl.cpp:14359
+!ErrorsInCtorInitializer &&
!CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
FD->setInvalidDecl();
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.
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::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 >&)
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77041
Files:
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/invalid-constructor-init.cpp
Index: clang/test/SemaCXX/invalid-constructor-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-constructor-init.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -frecovery-ast -verify %s
+
+struct X {
+ int Y;
+ constexpr X() : Y(foo()) {} // expected-error {{use of undeclared identifier
'foo'}}
+};
+
+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 "delegation cycle" diagnostic emitted!
+ CycleDelegate(float) : CycleDelegate(1) {}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -1685,6 +1685,7 @@
// This implements C++11 [dcl.constexpr]p3,4, as amended by DR1360.
bool Sema::CheckConstexprFunctionDefinition(const FunctionDecl *NewFD,
CheckConstexprKind Kind) {
+ assert(!NewFD->isInvalidDecl());
const CXXMethodDecl *MD = dyn_cast(NewFD);
if (MD && MD->isInstance()) {
// C++11 [dcl.constexpr]p4:
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -45,6 +45,7 @@
#include "clang/Sema/Template.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Triple.h"
+#include "llvm/Support/Casting.h"
#include
#include
#include
@@ -14345,7 +14346,16 @@
ActivePolicy = &WP;
}
+bool ErrorsInCtorInitializer =
+llvm::isa
