On Fri, Mar 6, 2020 at 4:12 AM Richard Smith <rich...@metafoo.co.uk> wrote:
>
> I implemented a completely different fix in
> a95cc77be154433c37a3110ac9af394b7447fcba. Please can you let me know if it
> works out?
Stephan reports that it's still breaking compilations (now with an
error diagnostic rather than a crash.)
Stephan, can you share the link to the error (off-list)?
>
> 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 <rich...@metafoo.co.uk> wrote:
>>
>> On Thu, 5 Mar 2020 at 06:13, Benjamin Kramer via cfe-commits
>> <cfe-commits@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....@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@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@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@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@lists.llvm.org
>>> > >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> > >> _______________________________________________
>>> > >> cfe-commits mailing list
>>> > >> cfe-commits@lists.llvm.org
>>> > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> > >
>>> > > _______________________________________________
>>> > > cfe-commits mailing list
>>> > > cfe-commits@lists.llvm.org
>>> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
