Author: Bruno Ricci Date: 2020-06-21T13:59:10+01:00 New Revision: e7ce0528202306d8b751f132d9d3a6519ce4e688
URL: https://github.com/llvm/llvm-project/commit/e7ce0528202306d8b751f132d9d3a6519ce4e688 DIFF: https://github.com/llvm/llvm-project/commit/e7ce0528202306d8b751f132d9d3a6519ce4e688.diff LOG: [clang][Serialization] Fix the serialization of ConstantExpr. The serialization of ConstantExpr has currently a number of problems: - Some fields are just not serialized (ConstantExprBits.APValueKind and ConstantExprBits.IsImmediateInvocation). - ASTStmtReader::VisitConstantExpr forgets to add the trailing APValue to the list of objects to be destroyed when the APValue needs cleanup. While we are at it, bring the serialization of ConstantExpr more in-line with what is done with the other expressions by doing the following NFCs: - Get rid of ConstantExpr::DefaultInit. It is better to not initialize the fields of an empty ConstantExpr since this will allow msan to detect if a field was not deserialized. - Move the initialization of the fields of ConstantExpr to the constructor; ConstantExpr::Create allocates the memory and ConstantExpr::ConstantExpr is responsible for the initialization. Review after commit since this is a straightforward mechanical fix similar to the other serialization fixes. Added: clang/test/AST/ast-dump-constant-expr.cpp Modified: clang/include/clang/AST/Expr.h clang/include/clang/AST/Stmt.h clang/lib/AST/Expr.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterStmt.cpp Removed: ################################################################################ diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index f8f104b5580b..8ee8aa96ae63 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -990,6 +990,9 @@ class ConstantExpr final static_assert(std::is_same<uint64_t, llvm::APInt::WordType>::value, "ConstantExpr assumes that llvm::APInt::WordType is uint64_t " "for tail-allocated storage"); + friend TrailingObjects; + friend class ASTStmtReader; + friend class ASTStmtWriter; public: /// Describes the kind of result that can be tail-allocated. @@ -1003,7 +1006,6 @@ class ConstantExpr final return ConstantExprBits.ResultKind == ConstantExpr::RSK_Int64; } - void DefaultInit(ResultStorageKind StorageKind); uint64_t &Int64Result() { assert(ConstantExprBits.ResultKind == ConstantExpr::RSK_Int64 && "invalid accessor"); @@ -1021,21 +1023,18 @@ class ConstantExpr final return const_cast<ConstantExpr *>(this)->APValueResult(); } - ConstantExpr(Expr *subexpr, ResultStorageKind StorageKind); - ConstantExpr(ResultStorageKind StorageKind, EmptyShell Empty); + ConstantExpr(Expr *SubExpr, ResultStorageKind StorageKind, + bool IsImmediateInvocation); + ConstantExpr(EmptyShell Empty, ResultStorageKind StorageKind); public: - friend TrailingObjects; - friend class ASTStmtReader; - friend class ASTStmtWriter; static ConstantExpr *Create(const ASTContext &Context, Expr *E, const APValue &Result); static ConstantExpr *Create(const ASTContext &Context, Expr *E, ResultStorageKind Storage = RSK_None, bool IsImmediateInvocation = false); static ConstantExpr *CreateEmpty(const ASTContext &Context, - ResultStorageKind StorageKind, - EmptyShell Empty); + ResultStorageKind StorageKind); static ResultStorageKind getStorageKind(const APValue &Value); static ResultStorageKind getStorageKind(const Type *T, diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h index d76d99b65551..a226790aa76c 100644 --- a/clang/include/clang/AST/Stmt.h +++ b/clang/include/clang/AST/Stmt.h @@ -325,7 +325,7 @@ class alignas(void *) Stmt { /// The kind of result that is tail-allocated. unsigned ResultKind : 2; - /// The kind of Result as defined by APValue::Kind + /// The kind of Result as defined by APValue::Kind. unsigned APValueKind : 4; /// When ResultKind == RSK_Int64, true if the tail-allocated integer is diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 27a12b880a7b..5cbd66f11601 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -244,6 +244,7 @@ static void AssertResultStorageKind(ConstantExpr::ResultStorageKind Kind) { assert((Kind == ConstantExpr::RSK_APValue || Kind == ConstantExpr::RSK_Int64 || Kind == ConstantExpr::RSK_None) && "Invalid StorageKind Value"); + (void)Kind; } ConstantExpr::ResultStorageKind @@ -268,33 +269,31 @@ ConstantExpr::getStorageKind(const Type *T, const ASTContext &Context) { return ConstantExpr::RSK_APValue; } -void ConstantExpr::DefaultInit(ResultStorageKind StorageKind) { +ConstantExpr::ConstantExpr(Expr *SubExpr, ResultStorageKind StorageKind, + bool IsImmediateInvocation) + : FullExpr(ConstantExprClass, SubExpr) { ConstantExprBits.ResultKind = StorageKind; ConstantExprBits.APValueKind = APValue::None; + ConstantExprBits.IsUnsigned = false; + ConstantExprBits.BitWidth = 0; ConstantExprBits.HasCleanup = false; - ConstantExprBits.IsImmediateInvocation = false; + ConstantExprBits.IsImmediateInvocation = IsImmediateInvocation; + if (StorageKind == ConstantExpr::RSK_APValue) ::new (getTrailingObjects<APValue>()) APValue(); } -ConstantExpr::ConstantExpr(Expr *subexpr, ResultStorageKind StorageKind) - : FullExpr(ConstantExprClass, subexpr) { - DefaultInit(StorageKind); -} - ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E, ResultStorageKind StorageKind, bool IsImmediateInvocation) { assert(!isa<ConstantExpr>(E)); AssertResultStorageKind(StorageKind); + unsigned Size = totalSizeToAlloc<APValue, uint64_t>( StorageKind == ConstantExpr::RSK_APValue, StorageKind == ConstantExpr::RSK_Int64); void *Mem = Context.Allocate(Size, alignof(ConstantExpr)); - ConstantExpr *Self = new (Mem) ConstantExpr(E, StorageKind); - Self->ConstantExprBits.IsImmediateInvocation = - IsImmediateInvocation; - return Self; + return new (Mem) ConstantExpr(E, StorageKind, IsImmediateInvocation); } ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E, @@ -305,21 +304,23 @@ ConstantExpr *ConstantExpr::Create(const ASTContext &Context, Expr *E, return Self; } -ConstantExpr::ConstantExpr(ResultStorageKind StorageKind, EmptyShell Empty) +ConstantExpr::ConstantExpr(EmptyShell Empty, ResultStorageKind StorageKind) : FullExpr(ConstantExprClass, Empty) { - DefaultInit(StorageKind); + ConstantExprBits.ResultKind = StorageKind; + + if (StorageKind == ConstantExpr::RSK_APValue) + ::new (getTrailingObjects<APValue>()) APValue(); } ConstantExpr *ConstantExpr::CreateEmpty(const ASTContext &Context, - ResultStorageKind StorageKind, - EmptyShell Empty) { + ResultStorageKind StorageKind) { AssertResultStorageKind(StorageKind); + unsigned Size = totalSizeToAlloc<APValue, uint64_t>( StorageKind == ConstantExpr::RSK_APValue, StorageKind == ConstantExpr::RSK_Int64); void *Mem = Context.Allocate(Size, alignof(ConstantExpr)); - ConstantExpr *Self = new (Mem) ConstantExpr(StorageKind, Empty); - return Self; + return new (Mem) ConstantExpr(EmptyShell(), StorageKind); } void ConstantExpr::MoveIntoResult(APValue &Value, const ASTContext &Context) { diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp index d5ae8979b1e1..0c379839ba83 100644 --- a/clang/lib/Serialization/ASTReaderStmt.cpp +++ b/clang/lib/Serialization/ASTReaderStmt.cpp @@ -541,18 +541,35 @@ void ASTStmtReader::VisitExpr(Expr *E) { void ASTStmtReader::VisitConstantExpr(ConstantExpr *E) { VisitExpr(E); - E->ConstantExprBits.ResultKind = Record.readInt(); - switch (E->ConstantExprBits.ResultKind) { - case ConstantExpr::RSK_Int64: { + + auto StorageKind = Record.readInt(); + assert(E->ConstantExprBits.ResultKind == StorageKind && "Wrong ResultKind!"); + + E->ConstantExprBits.APValueKind = Record.readInt(); + E->ConstantExprBits.IsUnsigned = Record.readInt(); + E->ConstantExprBits.BitWidth = Record.readInt(); + E->ConstantExprBits.HasCleanup = false; // Not serialized, see below. + E->ConstantExprBits.IsImmediateInvocation = Record.readInt(); + + switch (StorageKind) { + case ConstantExpr::RSK_None: + break; + + case ConstantExpr::RSK_Int64: E->Int64Result() = Record.readInt(); - uint64_t tmp = Record.readInt(); - E->ConstantExprBits.IsUnsigned = tmp & 0x1; - E->ConstantExprBits.BitWidth = tmp >> 1; break; - } + case ConstantExpr::RSK_APValue: E->APValueResult() = Record.readAPValue(); + if (E->APValueResult().needsCleanup()) { + E->ConstantExprBits.HasCleanup = true; + Record.getContext().addDestruction(&E->APValueResult()); + } + break; + default: + llvm_unreachable("unexpected ResultKind!"); } + E->setSubExpr(Record.readSubExpr()); } @@ -2859,10 +2876,8 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) { case EXPR_CONSTANT: S = ConstantExpr::CreateEmpty( - Context, - static_cast<ConstantExpr::ResultStorageKind>( - Record[ASTStmtReader::NumExprFields]), - Empty); + Context, static_cast<ConstantExpr::ResultStorageKind>( + /*StorageKind=*/Record[ASTStmtReader::NumExprFields])); break; case EXPR_PREDEFINED: diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp index 3a3b7bdfbb46..b13d0df9c376 100644 --- a/clang/lib/Serialization/ASTWriterStmt.cpp +++ b/clang/lib/Serialization/ASTWriterStmt.cpp @@ -548,16 +548,27 @@ void ASTStmtWriter::VisitExpr(Expr *E) { void ASTStmtWriter::VisitConstantExpr(ConstantExpr *E) { VisitExpr(E); - Record.push_back(static_cast<uint64_t>(E->ConstantExprBits.ResultKind)); + Record.push_back(E->ConstantExprBits.ResultKind); + + Record.push_back(E->ConstantExprBits.APValueKind); + Record.push_back(E->ConstantExprBits.IsUnsigned); + Record.push_back(E->ConstantExprBits.BitWidth); + // HasCleanup not serialized since we can just query the APValue. + Record.push_back(E->ConstantExprBits.IsImmediateInvocation); + switch (E->ConstantExprBits.ResultKind) { + case ConstantExpr::RSK_None: + break; case ConstantExpr::RSK_Int64: Record.push_back(E->Int64Result()); - Record.push_back(E->ConstantExprBits.IsUnsigned | - E->ConstantExprBits.BitWidth << 1); break; case ConstantExpr::RSK_APValue: Record.AddAPValue(E->APValueResult()); + break; + default: + llvm_unreachable("unexpected ResultKind!"); } + Record.AddStmt(E->getSubExpr()); Code = serialization::EXPR_CONSTANT; } diff --git a/clang/test/AST/ast-dump-constant-expr.cpp b/clang/test/AST/ast-dump-constant-expr.cpp new file mode 100644 index 000000000000..c9ddb245ef64 --- /dev/null +++ b/clang/test/AST/ast-dump-constant-expr.cpp @@ -0,0 +1,80 @@ +// Test without serialization: +// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown -ast-dump -ast-dump-filter Test %s \ +// RUN: | FileCheck --strict-whitespace --match-full-lines %s +// +// Test with serialization: +// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown -emit-pch -o %t %s +// RUN: %clang_cc1 -x c++ -std=c++20 -triple x86_64-unknown-unknown -include-pch %t \ +// RUN: -ast-dump-all -ast-dump-filter Test /dev/null \ +// RUN: | FileCheck --strict-whitespace --match-full-lines %s + +// FIXME: ASTRecordReader::readAPValue and ASTRecordWriter::AddAPValue +// just give up on some APValue kinds! This *really* should be fixed. + +struct array_holder { + int i[2]; +}; + +struct S { + int i = 42; +}; + +union U { + int i = 42; + float f; +}; + +struct SU { + S s[2]; + U u[3]; +}; + +consteval int test_Int() { return 42; } +consteval float test_Float() { return 1.0f; } +consteval _Complex int test_ComplexInt() { return 1+2i; } +consteval _Complex float test_ComplexFloat() { return 1.2f+3.4fi; } +consteval __int128 test_Int128() { return (__int128)0xFFFFFFFFFFFFFFFF + (__int128)1; } +// FIXME: consteval array_holder test_Array() { return array_holder(); } +// FIXME: consteval S test_Struct() { return S(); } +// FIXME: consteval U test_Union() { return U(); } +// FIXME: consteval SU test_SU() { return SU(); } + +void Test() { + (void) test_Int(); + (void) test_Float(); + (void) test_ComplexInt(); + (void) test_ComplexFloat(); + (void) test_Int128(); + //(void) test_Array(); + //(void) test_Struct(); + //(void) test_Union(); + //(void) test_SU(); +} +// CHECK:Dumping Test: +// CHECK-NEXT:FunctionDecl {{.*}} <{{.*}}ast-dump-constant-expr.cpp:42:1, line:52:1> line:42:6{{( imported)?}} Test 'void ()' +// CHECK-NEXT:`-CompoundStmt {{.*}} <col:13, line:52:1> +// CHECK-NEXT: |-CStyleCastExpr {{.*}} <line:43:3, col:19> 'void' <ToVoid> +// CHECK-NEXT: | `-ConstantExpr {{.*}} <col:10, col:19> 'int' Int: 42 +// CHECK-NEXT: | `-CallExpr {{.*}} <col:10, col:19> 'int' +// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:10> 'int (*)()' <FunctionToPointerDecay> +// CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:10> 'int ()' lvalue Function {{.*}} 'test_Int' 'int ()' +// CHECK-NEXT: |-CStyleCastExpr {{.*}} <line:44:3, col:21> 'void' <ToVoid> +// CHECK-NEXT: | `-ConstantExpr {{.*}} <col:10, col:21> 'float' Float: 1.000000e+00 +// CHECK-NEXT: | `-CallExpr {{.*}} <col:10, col:21> 'float' +// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:10> 'float (*)()' <FunctionToPointerDecay> +// CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:10> 'float ()' lvalue Function {{.*}} 'test_Float' 'float ()' +// CHECK-NEXT: |-CStyleCastExpr {{.*}} <line:45:3, col:26> 'void' <ToVoid> +// CHECK-NEXT: | `-ConstantExpr {{.*}} <col:10, col:26> '_Complex int' ComplexInt: 1, 2 +// CHECK-NEXT: | `-CallExpr {{.*}} <col:10, col:26> '_Complex int' +// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:10> '_Complex int (*)()' <FunctionToPointerDecay> +// CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:10> '_Complex int ()' lvalue Function {{.*}} 'test_ComplexInt' '_Complex int ()' +// CHECK-NEXT: |-CStyleCastExpr {{.*}} <line:46:3, col:28> 'void' <ToVoid> +// CHECK-NEXT: | `-ConstantExpr {{.*}} <col:10, col:28> '_Complex float' ComplexFloat: 1.200000e+00, 3.400000e+00 +// CHECK-NEXT: | `-CallExpr {{.*}} <col:10, col:28> '_Complex float' +// CHECK-NEXT: | `-ImplicitCastExpr {{.*}} <col:10> '_Complex float (*)()' <FunctionToPointerDecay> +// CHECK-NEXT: | `-DeclRefExpr {{.*}} <col:10> '_Complex float ()' lvalue Function {{.*}} 'test_ComplexFloat' '_Complex float ()' +// CHECK-NEXT: `-CStyleCastExpr {{.*}} <line:47:3, col:22> 'void' <ToVoid> +// CHECK-NEXT: `-ConstantExpr {{.*}} <col:10, col:22> '__int128' Int: 18446744073709551616 +// CHECK-NEXT: `-CallExpr {{.*}} <col:10, col:22> '__int128' +// CHECK-NEXT: `-ImplicitCastExpr {{.*}} <col:10> '__int128 (*)()' <FunctionToPointerDecay> +// CHECK-NEXT: `-DeclRefExpr {{.*}} <col:10> '__int128 ()' lvalue Function {{.*}} 'test_Int128' '__int128 ()' _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
