From 05843dc6ab97a00cbde7aa4f08bf3692eb83109d Mon Sep 17 00:00:00 2001 From: Bruno Ricci Date: Thu, 18 Jun 2020 12:51:11 +0100 Subject: [PATCH] [clang] Fix the serialization of LambdaExpr and the bogus mutation in LambdaExpr::getBody The body of LambdaExpr is currently not properly serialized. Instead LambdaExpr::getBody checks if the body has been already deserialized and if not mutates LambdaExpr. This can be observed with an AST dump test, where the body of the LambdaExpr will be null. The mutation in LambdaExpr::getBody was left because of another bug: it is not true that the body of a LambdaExpr is always a CompoundStmt; it can also be a CoroutineBodyStmt wrapping a CompoundStmt. This is fixed by returning a Stmt * from getBody and introducing a convenience function getCompoundStmtBody which always returns a CompoundStmt *. This function can be used by callers who do not care about the coroutine node. Happily all but one user of getBody treat it as a Stmt * and so this change is non-intrusive. Differential Revision: https://reviews.llvm.org/D81787 Reviewed By: aaron.ballman --- clang/include/clang/AST/ExprCXX.h | 22 ++++++++++++++-- clang/lib/AST/ExprCXX.cpp | 20 -------------- clang/lib/AST/StmtPrinter.cpp | 2 +- clang/lib/Serialization/ASTReaderStmt.cpp | 3 +++ clang/lib/Serialization/ASTWriterStmt.cpp | 2 ++ clang/test/AST/ast-dump-lambda.cpp | 44 ++++++++++++++++++++----------- 6 files changed, 54 insertions(+), 39 deletions(-) diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h index 822025a..4c65daa 100644 --- a/clang/include/clang/AST/ExprCXX.h +++ b/clang/include/clang/AST/ExprCXX.h @@ -26,6 +26,7 @@ #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/OperationKinds.h" #include "clang/AST/Stmt.h" +#include "clang/AST/StmtCXX.h" #include "clang/AST/TemplateBase.h" #include "clang/AST/Type.h" #include "clang/AST/UnresolvedSet.h" @@ -2004,8 +2005,25 @@ public: /// Whether this is a generic lambda. bool isGenericLambda() const { return getTemplateParameterList(); } - /// Retrieve the body of the lambda. - CompoundStmt *getBody() const; + /// Retrieve the body of the lambda. This will be most of the time + /// a \p CompoundStmt, but can also be \p CoroutineBodyStmt wrapping + /// a \p CompoundStmt. Note that unlike functions, lambda-expressions + /// cannot have a function-try-block. + Stmt *getBody() const { return getStoredStmts()[capture_size()]; } + + /// Retrieve the \p CompoundStmt representing the body of the lambda. + /// This is a convenience function for callers who do not need + /// to handle node(s) which may wrap a \p CompoundStmt. + const CompoundStmt *getCompoundStmtBody() const { + Stmt *Body = getBody(); + if (const auto *CoroBody = dyn_cast(Body)) + return cast(CoroBody->getBody()); + return cast(Body); + } + CompoundStmt *getCompoundStmtBody() { + const auto *ConstThis = this; + return const_cast(ConstThis->getCompoundStmtBody()); + } /// Determine whether the lambda is mutable, meaning that any /// captures values can be modified. diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp index 8588e9b..f9f825b 100644 --- a/clang/lib/AST/ExprCXX.cpp +++ b/clang/lib/AST/ExprCXX.cpp @@ -1118,15 +1118,6 @@ LambdaExpr::LambdaExpr(QualType T, SourceRange IntroducerRange, LambdaExpr::LambdaExpr(EmptyShell Empty, unsigned NumCaptures) : Expr(LambdaExprClass, Empty) { LambdaExprBits.NumCaptures = NumCaptures; - - // FIXME: There is no need to do this since these members should be - // initialized during deserialization. - LambdaExprBits.CaptureDefault = LCD_None; - LambdaExprBits.ExplicitParams = false; - LambdaExprBits.ExplicitResultType = false; - - // FIXME: Remove once the bug in LambdaExpr::getBody is fixed. - getStoredStmts()[capture_size()] = nullptr; } LambdaExpr *LambdaExpr::Create(const ASTContext &Context, CXXRecordDecl *Class, @@ -1223,17 +1214,6 @@ ArrayRef LambdaExpr::getExplicitTemplateParameters() const { return Record->getLambdaExplicitTemplateParameters(); } -CompoundStmt *LambdaExpr::getBody() const { - // FIXME: this mutation in getBody is bogus. It should be - // initialized in ASTStmtReader::VisitLambdaExpr, but for reasons I - // don't understand, that doesn't work. - if (!getStoredStmts()[capture_size()]) - *const_cast(&getStoredStmts()[capture_size()]) = - getCallOperator()->getBody(); - - return static_cast(getStoredStmts()[capture_size()]); -} - bool LambdaExpr::isMutable() const { return !getCallOperator()->isConst(); } ExprWithCleanups::ExprWithCleanups(Expr *subexpr, diff --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp index d5d0253..11e222f 100644 --- a/clang/lib/AST/StmtPrinter.cpp +++ b/clang/lib/AST/StmtPrinter.cpp @@ -2051,7 +2051,7 @@ void StmtPrinter::VisitLambdaExpr(LambdaExpr *Node) { if (Policy.TerseOutput) OS << "{}"; else - PrintRawCompoundStmt(Node->getBody()); + PrintRawCompoundStmt(Node->getCompoundStmtBody()); } void StmtPrinter::VisitCXXScalarValueInitExpr(CXXScalarValueInitExpr *Node) { diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp index e38c309..d5ae897 100644 --- a/clang/lib/Serialization/ASTReaderStmt.cpp +++ b/clang/lib/Serialization/ASTReaderStmt.cpp @@ -1701,6 +1701,9 @@ void ASTStmtReader::VisitLambdaExpr(LambdaExpr *E) { CEnd = E->capture_init_end(); C != CEnd; ++C) *C = Record.readSubExpr(); + + // Ok, not one past the end. + E->getStoredStmts()[NumCaptures] = Record.readSubStmt(); } void diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp index 1194132..3a3b7bd 100644 --- a/clang/lib/Serialization/ASTWriterStmt.cpp +++ b/clang/lib/Serialization/ASTWriterStmt.cpp @@ -1604,6 +1604,8 @@ void ASTStmtWriter::VisitLambdaExpr(LambdaExpr *E) { Record.AddStmt(*C); } + Record.AddStmt(E->getBody()); + Code = serialization::EXPR_LAMBDA; } diff --git a/clang/test/AST/ast-dump-lambda.cpp b/clang/test/AST/ast-dump-lambda.cpp index 3f10d12..d5f43c1 100644 --- a/clang/test/AST/ast-dump-lambda.cpp +++ b/clang/test/AST/ast-dump-lambda.cpp @@ -8,7 +8,7 @@ // RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \ // RUN: -x c++ -include-pch %t -ast-dump-all | FileCheck -strict-whitespace %s -// XFAIL: * + template void test(Ts... a) { struct V { @@ -64,7 +64,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-FieldDecl {{.*}} col:8{{( imported)?}} implicit 'V *' // CHECK-NEXT: | | |-ParenListExpr {{.*}} 'NULL TYPE' // CHECK-NEXT: | | | `-CXXThisExpr {{.*}} 'V *' this -// CHECK-NEXT: | | `-<<>> +// CHECK-NEXT: | | `-CompoundStmt {{.*}} // CHECK-NEXT: | `-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:17:7)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:7{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init @@ -80,7 +80,7 @@ template void test(Ts... a) { // CHECK-NEXT: | |-ParenListExpr {{.*}} 'NULL TYPE' // CHECK-NEXT: | | `-UnaryOperator {{.*}} '' prefix '*' cannot overflow // CHECK-NEXT: | | `-CXXThisExpr {{.*}} 'V *' this -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-DeclStmt {{.*}} // CHECK-NEXT: | |-VarDecl {{.*}} col:7{{( imported)?}} referenced b 'int' // CHECK-NEXT: | `-VarDecl {{.*}} col:10{{( imported)?}} referenced c 'int' @@ -97,7 +97,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator auto (*)() 'auto (*() const noexcept)()' inline // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto ()' static inline -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:22:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -113,7 +113,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator auto (*)(int, ...) 'auto (*() const noexcept)(int, ...)' inline // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto (int, ...)' static inline // CHECK-NEXT: | | `-ParmVarDecl {{.*}} col:10{{( imported)?}} a 'int' -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:23:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init @@ -128,7 +128,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | `-FieldDecl {{.*}} col:4{{( imported)?}} implicit 'Ts...' // CHECK-NEXT: | |-ParenListExpr {{.*}} 'NULL TYPE' // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'Ts...' lvalue ParmVar {{.*}} 'a' 'Ts...' -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:24:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -140,7 +140,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-Destructor simple irrelevant trivial needs_implicit // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} operator() 'auto () const -> auto' inline // CHECK-NEXT: | | `-CompoundStmt {{.*}} -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:25:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -154,7 +154,9 @@ template void test(Ts... a) { // CHECK-NEXT: | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | `-ReturnStmt {{.*}} // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'const int' lvalue Var {{.*}} 'b' 'int' -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} +// CHECK-NEXT: | `-ReturnStmt {{.*}} +// CHECK-NEXT: | `-DeclRefExpr {{.*}} 'const int' lvalue Var {{.*}} 'b' 'int' // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:26:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -166,7 +168,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-Destructor simple irrelevant trivial needs_implicit // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} operator() 'auto () const -> auto' inline // CHECK-NEXT: | | `-CompoundStmt {{.*}} -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:27:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -180,7 +182,9 @@ template void test(Ts... a) { // CHECK-NEXT: | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | `-ReturnStmt {{.*}} // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'c' 'int' -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} +// CHECK-NEXT: | `-ReturnStmt {{.*}} +// CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'c' 'int' // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:28:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda trivially_copyable literal can_const_default_init @@ -203,7 +207,13 @@ template void test(Ts... a) { // CHECK-NEXT: | |-ImplicitCastExpr {{.*}} 'int' // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'b' 'int' // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'c' 'int' -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} +// CHECK-NEXT: | `-ReturnStmt {{.*}} +// CHECK-NEXT: | `-BinaryOperator {{.*}} 'int' '+' +// CHECK-NEXT: | |-ImplicitCastExpr {{.*}} 'int' +// CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'const int' lvalue Var {{.*}} 'b' 'int' +// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} 'int' +// CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'c' 'int' // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:29:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init @@ -220,7 +230,7 @@ template void test(Ts... a) { // CHECK-NEXT: | |-ParenListExpr {{.*}} 'NULL TYPE' // CHECK-NEXT: | | `-DeclRefExpr {{.*}} 'Ts...' lvalue ParmVar {{.*}} 'a' 'Ts...' // CHECK-NEXT: | |-IntegerLiteral {{.*}} 'int' 12 -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:30:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -234,7 +244,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator auto (*)() 'auto (*() const noexcept)()' inline // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto ()' static inline -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:31:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -248,7 +258,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator auto (*)() 'auto (*() const noexcept)()' inline // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto ()' static inline -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: |-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:32:3)' // CHECK-NEXT: | |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -262,7 +272,7 @@ template void test(Ts... a) { // CHECK-NEXT: | | | `-CompoundStmt {{.*}} // CHECK-NEXT: | | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator auto (*)() noexcept 'auto (*() const noexcept)() noexcept' inline // CHECK-NEXT: | | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto () noexcept' static inline -// CHECK-NEXT: | `-<<>> +// CHECK-NEXT: | `-CompoundStmt {{.*}} // CHECK-NEXT: `-LambdaExpr {{.*}} '(lambda at {{.*}}ast-dump-lambda.cpp:33:3)' // CHECK-NEXT: |-CXXRecordDecl {{.*}} col:3{{( imported)?}} implicit{{( )?}} class definition // CHECK-NEXT: | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init @@ -278,4 +288,6 @@ template void test(Ts... a) { // CHECK-NEXT: | | `-IntegerLiteral {{.*}} 'int' 0 // CHECK-NEXT: | |-CXXConversionDecl {{.*}} col:3{{( imported)?}} implicit constexpr operator int (*)() 'auto (*() const noexcept)() -> int' inline // CHECK-NEXT: | `-CXXMethodDecl {{.*}} col:3{{( imported)?}} implicit __invoke 'auto () -> int' static inline -// CHECK-NEXT: `-<<>> +// CHECK-NEXT: `-CompoundStmt {{.*}} +// CHECK-NEXT: `-ReturnStmt {{.*}} +// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 0 -- 2.7.4