[clang] bdad0a1 - PR45083: Mark statement expressions as being dependent if they appear in
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 5 19:12:03 PST 2020
I implemented a completely different fix in
a95cc77be154433c37a3110ac9af394b7447fcba. Please can you let me know if it
works out?
The old approach was to treat statement expressions as being value- and
instantiation-dependent if they appear in a dependent context (matching
GCC's apparent behavior). But it turns out that Clang doesn't really know
whether it's in a dependent context! (In particular, Sema::CurContext might
be a template when substituting into that template, might be the enclosing
context, and might be the resulting declaration if it already exists. This
has other implications -- we at least get access checks wrong in some cases
-- but fixing it would be far to invasive to do for Clang 10.)
The new approach is to walk the body of the statement expression and see if
it contains any value- or instantiation-dependent subexpressions, and treat
the statement expression as being correspondingly dependent if so.
Hans, the new fix will need some work to backport due to ec3060c (it should
be largely mechanical, but perhaps not completely obvious what the
mechanical steps are). Let me know if you'd like me to do the port.
On Thu, 5 Mar 2020 at 12:20, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, 5 Mar 2020 at 06:13, Benjamin Kramer via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> creduce produced this. It's a crash on invalid, but was created from a
>> valid input.
>>
>
> Well, "valid" is unclear when using language extensions, but OK.
>
>
>> $ cat r.ii
>> template <class a> auto b(a) {
>> auto c = [](auto, int) -> decltype(({})) {};
>> return c(0, 0);
>> }
>> using d = decltype(b(0));
>> bool f = d ::e;
>>
>
> How important is this testcase? The fix you reverted was fixing a
> release-blocker; is this also a release-blocker in your view?
>
>
>> $ clang r.ii -std=c++17 -w
>> clang-11: clang/lib/AST/Decl.cpp:2343: clang::APValue
>> *clang::VarDecl::evaluateValue(SmallVectorImpl<clang::PartialDiagnosticAt>
>> &) const: Assertion `!Init->isValueDependent()' failed.
>>
>> On Thu, Mar 5, 2020 at 2:18 PM Benjamin Kramer <benny.kra at gmail.com>
>> wrote:
>> >
>> > It's still crashing clang, reverted this and
>> > f545ede91c9d9f271e7504282cab7bf509607ead in 66addf8e8036. c-reduce is
>> > still chewing on the reproducer.
>> >
>> > On Wed, Mar 4, 2020 at 10:20 PM Richard Smith via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> > >
>> > > We found a regression introduced by this patch; fixed in
>> f545ede91c9d9f271e7504282cab7bf509607ead.
>> > >
>> > > On Wed, 4 Mar 2020 at 00:30, Hans Wennborg via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>> > >>
>> > >> Pushed to 10.x as 3a843031a5ad83a00d2603f623881cb2b2bf719d. Please
>> let
>> > >> me know if you hear about any follow-up issues.
>> > >>
>> > >> Thanks!
>> > >>
>> > >> On Wed, Mar 4, 2020 at 12:28 AM Richard Smith via cfe-commits
>> > >> <cfe-commits at lists.llvm.org> wrote:
>> > >> >
>> > >> >
>> > >> > Author: Richard Smith
>> > >> > Date: 2020-03-03T15:20:40-08:00
>> > >> > New Revision: bdad0a1b79273733df9acc1be4e992fa5d70ec56
>> > >> >
>> > >> > URL:
>> https://github.com/llvm/llvm-project/commit/bdad0a1b79273733df9acc1be4e992fa5d70ec56
>> > >> > DIFF:
>> https://github.com/llvm/llvm-project/commit/bdad0a1b79273733df9acc1be4e992fa5d70ec56.diff
>> > >> >
>> > >> > LOG: PR45083: Mark statement expressions as being dependent if
>> they appear in
>> > >> > dependent contexts.
>> > >> >
>> > >> > We previously assumed they were neither value- nor
>> > >> > instantiation-dependent under any circumstances, which would lead
>> to
>> > >> > crashes and other misbehavior.
>> > >> >
>> > >> > Added:
>> > >> >
>> > >> >
>> > >> > Modified:
>> > >> > clang/include/clang/AST/Expr.h
>> > >> > clang/include/clang/Sema/Sema.h
>> > >> > clang/lib/AST/ASTImporter.cpp
>> > >> > clang/lib/Parse/ParseExpr.cpp
>> > >> > clang/lib/Sema/SemaExpr.cpp
>> > >> > clang/lib/Sema/SemaExprCXX.cpp
>> > >> > clang/lib/Sema/TreeTransform.h
>> > >> > clang/test/SemaTemplate/dependent-expr.cpp
>> > >> >
>> > >> > Removed:
>> > >> >
>> > >> >
>> > >> >
>> > >> >
>> ################################################################################
>> > >> > diff --git a/clang/include/clang/AST/Expr.h
>> b/clang/include/clang/AST/Expr.h
>> > >> > index fcdb0b992134..87f9b883486a 100644
>> > >> > --- a/clang/include/clang/AST/Expr.h
>> > >> > +++ b/clang/include/clang/AST/Expr.h
>> > >> > @@ -3960,14 +3960,14 @@ class StmtExpr : public Expr {
>> > >> > Stmt *SubStmt;
>> > >> > SourceLocation LParenLoc, RParenLoc;
>> > >> > public:
>> > >> > - // FIXME: Does type-dependence need to be computed
>> > >> > diff erently?
>> > >> > - // FIXME: Do we need to compute instantiation
>> instantiation-dependence for
>> > >> > - // statements? (ugh!)
>> > >> > StmtExpr(CompoundStmt *substmt, QualType T,
>> > >> > - SourceLocation lp, SourceLocation rp) :
>> > >> > + SourceLocation lp, SourceLocation rp, bool
>> InDependentContext) :
>> > >> > + // Note: we treat a statement-expression in a dependent
>> context as always
>> > >> > + // being value- and instantiation-dependent. This matches the
>> behavior of
>> > >> > + // lambda-expressions and GCC.
>> > >> > Expr(StmtExprClass, T, VK_RValue, OK_Ordinary,
>> > >> > - T->isDependentType(), false, false, false),
>> > >> > - SubStmt(substmt), LParenLoc(lp), RParenLoc(rp) { }
>> > >> > + T->isDependentType(), InDependentContext,
>> InDependentContext, false),
>> > >> > + SubStmt(substmt), LParenLoc(lp), RParenLoc(rp) {}
>> > >> >
>> > >> > /// Build an empty statement expression.
>> > >> > explicit StmtExpr(EmptyShell Empty) : Expr(StmtExprClass,
>> Empty) { }
>> > >> >
>> > >> > diff --git a/clang/include/clang/Sema/Sema.h
>> b/clang/include/clang/Sema/Sema.h
>> > >> > index 2304a9718567..5739808753e3 100644
>> > >> > --- a/clang/include/clang/Sema/Sema.h
>> > >> > +++ b/clang/include/clang/Sema/Sema.h
>> > >> > @@ -4964,7 +4964,7 @@ class Sema final {
>> > >> > LabelDecl *TheDecl);
>> > >> >
>> > >> > void ActOnStartStmtExpr();
>> > >> > - ExprResult ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
>> > >> > + ExprResult ActOnStmtExpr(Scope *S, SourceLocation LPLoc, Stmt
>> *SubStmt,
>> > >> > SourceLocation RPLoc); // "({..})"
>> > >> > // Handle the final expression in a statement expression.
>> > >> > ExprResult ActOnStmtExprResult(ExprResult E);
>> > >> >
>> > >> > diff --git a/clang/lib/AST/ASTImporter.cpp
>> b/clang/lib/AST/ASTImporter.cpp
>> > >> > index 52288318337d..0cf00f6ca15b 100644
>> > >> > --- a/clang/lib/AST/ASTImporter.cpp
>> > >> > +++ b/clang/lib/AST/ASTImporter.cpp
>> > >> > @@ -6631,8 +6631,9 @@ ExpectedStmt
>> ASTNodeImporter::VisitStmtExpr(StmtExpr *E) {
>> > >> > if (Err)
>> > >> > return std::move(Err);
>> > >> >
>> > >> > - return new (Importer.getToContext()) StmtExpr(
>> > >> > - ToSubStmt, ToType, ToLParenLoc, ToRParenLoc);
>> > >> > + return new (Importer.getToContext())
>> > >> > + StmtExpr(ToSubStmt, ToType, ToLParenLoc, ToRParenLoc,
>> > >> > + E->isInstantiationDependent());
>> > >> > }
>> > >> >
>> > >> > ExpectedStmt ASTNodeImporter::VisitUnaryOperator(UnaryOperator
>> *E) {
>> > >> >
>> > >> > diff --git a/clang/lib/Parse/ParseExpr.cpp
>> b/clang/lib/Parse/ParseExpr.cpp
>> > >> > index 584de6b87d90..b038e6935d87 100644
>> > >> > --- a/clang/lib/Parse/ParseExpr.cpp
>> > >> > +++ b/clang/lib/Parse/ParseExpr.cpp
>> > >> > @@ -2655,7 +2655,8 @@
>> Parser::ParseParenExpression(ParenParseOption &ExprType, bool
>> stopIfCastExpr,
>> > >> >
>> > >> > // If the substmt parsed correctly, build the AST node.
>> > >> > if (!Stmt.isInvalid()) {
>> > >> > - Result = Actions.ActOnStmtExpr(OpenLoc, Stmt.get(),
>> Tok.getLocation());
>> > >> > + Result = Actions.ActOnStmtExpr(getCurScope(), OpenLoc,
>> Stmt.get(),
>> > >> > + Tok.getLocation());
>> > >> > } else {
>> > >> > Actions.ActOnStmtExprError();
>> > >> > }
>> > >> >
>> > >> > diff --git a/clang/lib/Sema/SemaExpr.cpp
>> b/clang/lib/Sema/SemaExpr.cpp
>> > >> > index f50a77a40510..1870e440199d 100644
>> > >> > --- a/clang/lib/Sema/SemaExpr.cpp
>> > >> > +++ b/clang/lib/Sema/SemaExpr.cpp
>> > >> > @@ -13911,9 +13911,8 @@ void Sema::ActOnStmtExprError() {
>> > >> > PopExpressionEvaluationContext();
>> > >> > }
>> > >> >
>> > >> > -ExprResult
>> > >> > -Sema::ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
>> > >> > - SourceLocation RPLoc) { // "({..})"
>> > >> > +ExprResult Sema::ActOnStmtExpr(Scope *S, SourceLocation LPLoc,
>> Stmt *SubStmt,
>> > >> > + SourceLocation RPLoc) { // "({..})"
>> > >> > assert(SubStmt && isa<CompoundStmt>(SubStmt) && "Invalid action
>> invocation!");
>> > >> > CompoundStmt *Compound = cast<CompoundStmt>(SubStmt);
>> > >> >
>> > >> > @@ -13942,9 +13941,18 @@ Sema::ActOnStmtExpr(SourceLocation LPLoc,
>> Stmt *SubStmt,
>> > >> > }
>> > >> > }
>> > >> >
>> > >> > + bool IsDependentContext = false;
>> > >> > + if (S)
>> > >> > + IsDependentContext = S->getTemplateParamParent() != nullptr;
>> > >> > + else
>> > >> > + // FIXME: This is not correct when substituting inside a
>> templated
>> > >> > + // context that isn't a DeclContext (such as a variable
>> template).
>> > >> > + IsDependentContext = CurContext->isDependentContext();
>> > >> > +
>> > >> > // FIXME: Check that expression type is complete/non-abstract;
>> statement
>> > >> > // expressions are not lvalues.
>> > >> > - Expr *ResStmtExpr = new (Context) StmtExpr(Compound, Ty, LPLoc,
>> RPLoc);
>> > >> > + Expr *ResStmtExpr =
>> > >> > + new (Context) StmtExpr(Compound, Ty, LPLoc, RPLoc,
>> IsDependentContext);
>> > >> > if (StmtExprMayBindToTemp)
>> > >> > return MaybeBindToTemporary(ResStmtExpr);
>> > >> > return ResStmtExpr;
>> > >> >
>> > >> > diff --git a/clang/lib/Sema/SemaExprCXX.cpp
>> b/clang/lib/Sema/SemaExprCXX.cpp
>> > >> > index c8c6d7c95a7f..e521ba1d4af1 100644
>> > >> > --- a/clang/lib/Sema/SemaExprCXX.cpp
>> > >> > +++ b/clang/lib/Sema/SemaExprCXX.cpp
>> > >> > @@ -6955,8 +6955,9 @@ Stmt *Sema::MaybeCreateStmtWithCleanups(Stmt
>> *SubStmt) {
>> > >> > // a new AsmStmtWithTemporaries.
>> > >> > CompoundStmt *CompStmt = CompoundStmt::Create(
>> > >> > Context, SubStmt, SourceLocation(), SourceLocation());
>> > >> > - Expr *E = new (Context) StmtExpr(CompStmt, Context.VoidTy,
>> SourceLocation(),
>> > >> > - SourceLocation());
>> > >> > + Expr *E = new (Context)
>> > >> > + StmtExpr(CompStmt, Context.VoidTy, SourceLocation(),
>> SourceLocation(),
>> > >> > + CurContext->isDependentContext());
>> > >> > return MaybeCreateExprWithCleanups(E);
>> > >> > }
>> > >> >
>> > >> >
>> > >> > diff --git a/clang/lib/Sema/TreeTransform.h
>> b/clang/lib/Sema/TreeTransform.h
>> > >> > index 002b73c3a1dd..05b41aa53da6 100644
>> > >> > --- a/clang/lib/Sema/TreeTransform.h
>> > >> > +++ b/clang/lib/Sema/TreeTransform.h
>> > >> > @@ -2549,10 +2549,9 @@ class TreeTransform {
>> > >> > ///
>> > >> > /// By default, performs semantic analysis to build the new
>> expression.
>> > >> > /// Subclasses may override this routine to provide
>> > >> > diff erent behavior.
>> > >> > - ExprResult RebuildStmtExpr(SourceLocation LParenLoc,
>> > >> > - Stmt *SubStmt,
>> > >> > - SourceLocation RParenLoc) {
>> > >> > - return getSema().ActOnStmtExpr(LParenLoc, SubStmt, RParenLoc);
>> > >> > + ExprResult RebuildStmtExpr(SourceLocation LParenLoc, Stmt
>> *SubStmt,
>> > >> > + SourceLocation RParenLoc) {
>> > >> > + return getSema().ActOnStmtExpr(nullptr, LParenLoc, SubStmt,
>> RParenLoc);
>> > >> > }
>> > >> >
>> > >> > /// Build a new __builtin_choose_expr expression.
>> > >> > @@ -11888,6 +11887,8 @@
>> TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
>> > >> > NewTrailingRequiresClause = getDerived().TransformExpr(TRC);
>> > >> >
>> > >> > // Create the local class that will describe the lambda.
>> > >> > + // FIXME: KnownDependent below is wrong when substituting
>> inside a templated
>> > >> > + // context that isn't a DeclContext (such as a variable
>> template).
>> > >> > CXXRecordDecl *OldClass = E->getLambdaClass();
>> > >> > CXXRecordDecl *Class
>> > >> > = getSema().createLambdaClosureType(E->getIntroducerRange(),
>> > >> >
>> > >> > diff --git a/clang/test/SemaTemplate/dependent-expr.cpp
>> b/clang/test/SemaTemplate/dependent-expr.cpp
>> > >> > index bb1e239c3490..e333ed927b9e 100644
>> > >> > --- a/clang/test/SemaTemplate/dependent-expr.cpp
>> > >> > +++ b/clang/test/SemaTemplate/dependent-expr.cpp
>> > >> > @@ -1,5 +1,4 @@
>> > >> > // RUN: %clang_cc1 -fsyntax-only -verify %s
>> > >> > -// expected-no-diagnostics
>> > >> >
>> > >> > // PR5908
>> > >> > template <typename Iterator>
>> > >> > @@ -108,3 +107,22 @@ namespace PR18152 {
>> > >> > };
>> > >> > template struct A<0>;
>> > >> > }
>> > >> > +
>> > >> > +template<typename T> void stmt_expr_1() {
>> > >> > + static_assert( ({ false; }), "" );
>> > >> > +}
>> > >> > +void stmt_expr_2() {
>> > >> > + static_assert( ({ false; }), "" ); // expected-error {{failed}}
>> > >> > +}
>> > >> > +
>> > >> > +namespace PR45083 {
>> > >> > + struct A { bool x; };
>> > >> > +
>> > >> > + template<typename> struct B : A {
>> > >> > + void f() {
>> > >> > + const int n = ({ if (x) {} 0; });
>> > >> > + }
>> > >> > + };
>> > >> > +
>> > >> > + template void B<int>::f();
>> > >> > +}
>> > >> >
>> > >> >
>> > >> >
>> > >> > _______________________________________________
>> > >> > cfe-commits mailing list
>> > >> > cfe-commits at lists.llvm.org
>> > >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> > >> _______________________________________________
>> > >> cfe-commits mailing list
>> > >> cfe-commits at lists.llvm.org
>> > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> > >
>> > > _______________________________________________
>> > > cfe-commits mailing list
>> > > cfe-commits at lists.llvm.org
>> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200305/36f95786/attachment-0001.html>
More information about the cfe-commits
mailing list