Author: Kinuko Yasuda Date: 2023-09-07T07:37:50Z New Revision: f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3
URL: https://github.com/llvm/llvm-project/commit/f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3 DIFF: https://github.com/llvm/llvm-project/commit/f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3.diff LOG: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances Usually RecordValues for record objects (e.g. struct) are initialized with `Environment::createValue()` which internally calls `getObjectFields()` to collects all fields from the current and base classes, and then filter them with `ModeledValues` via `DACtx::getModeledFields()` so that the fields that are actually referenced are modeled. The consistent set of fields should be initialized when a record is initialized with an initializer list (InitListExpr), however the existing code's behavior was different. Before this patch: * When a struct is initialized with InitListExpr, its fields are initialized based on what is returned by `getFieldsForInitListExpr()`, which only collects the direct fields in the current class, but not from the base classes. Moreover, if the base classes have their own InitListExpr, values that are initialized by their InitListExpr's weren't merged into the child objects. After this patch: * When a struct is initialized with InitListExpr, it collects and merges the fields in the base classes that were initialized by their InitListExpr's. The code also asserts that the consistent set of fields are initialized with the ModeledFields. Reviewed By: mboehme Differential Revision: https://reviews.llvm.org/D159284 Added: Modified: clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/TransferTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index fcd9b20027cde9..67d8be392ae605 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -27,12 +27,12 @@ #include "clang/Analysis/FlowSensitive/Value.h" #include "clang/Basic/Builtins.h" #include "clang/Basic/OperatorKinds.h" -#include "llvm/ADT/STLExtras.h" #include "llvm/Support/Casting.h" -#include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/Debug.h" +#include <assert.h> #include <cassert> -#include <memory> -#include <tuple> + +#define DEBUG_TYPE "dataflow" namespace clang { namespace dataflow { @@ -629,17 +629,66 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { return; } - std::vector<FieldDecl *> Fields = - getFieldsForInitListExpr(Type->getAsRecordDecl()); llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs; - for (auto [Field, Init] : llvm::zip(Fields, S->inits())) { - assert(Field != nullptr); - assert(Init != nullptr); + // This only contains the direct fields for the given type. + std::vector<FieldDecl *> FieldsForInit = + getFieldsForInitListExpr(Type->getAsRecordDecl()); - FieldLocs.insert({Field, &Env.createObject(Field->getType(), Init)}); + // `S->inits()` contains all the initializer epressions, including the + // ones for direct base classes. + auto Inits = S->inits(); + size_t InitIdx = 0; + + // Initialize base classes. + if (auto* R = S->getType()->getAsCXXRecordDecl()) { + assert(FieldsForInit.size() + R->getNumBases() == Inits.size()); + for ([[maybe_unused]] const CXXBaseSpecifier &Base : R->bases()) { + assert(InitIdx < Inits.size()); + auto Init = Inits[InitIdx++]; + assert(Base.getType().getCanonicalType() == + Init->getType().getCanonicalType()); + auto* BaseVal = cast_or_null<RecordValue>(Env.getValue(*Init)); + if (!BaseVal) + BaseVal = cast<RecordValue>(Env.createValue(Init->getType())); + // Take ownership of the fields of the `RecordValue` for the base class + // and incorporate them into the "flattened" set of fields for the + // derived class. + auto Children = BaseVal->getLoc().children(); + FieldLocs.insert(Children.begin(), Children.end()); + } } + assert(FieldsForInit.size() == Inits.size() - InitIdx); + for (auto Field : FieldsForInit) { + assert(InitIdx < Inits.size()); + auto Init = Inits[InitIdx++]; + assert( + // The types are same, or + Field->getType().getCanonicalType().getUnqualifiedType() == + Init->getType().getCanonicalType() || + // The field's type is T&, and initializer is T + (Field->getType()->isReferenceType() && + Field->getType().getCanonicalType()->getPointeeType() == + Init->getType().getCanonicalType())); + auto& Loc = Env.createObject(Field->getType(), Init); + FieldLocs.insert({Field, &Loc}); + } + + LLVM_DEBUG({ + // Check that we satisfy the invariant that a `RecordStorageLoation` + // contains exactly the set of modeled fields for that type. + // `ModeledFields` includes fields from all the bases, but only the + // modeled ones. However, if a class type is initialized with an + // `InitListExpr`, all fields in the class, including those from base + // classes, are included in the set of modeled fields. The code above + // should therefore populate exactly the modeled fields. + auto ModeledFields = Env.getDataflowAnalysisContext().getModeledFields(Type); + assert(ModeledFields.size() == FieldLocs.size()); + for ([[maybe_unused]] auto [Field, Loc] : FieldLocs) + assert(ModeledFields.contains(cast_or_null<FieldDecl>(Field))); + }); + auto &Loc = Env.getDataflowAnalysisContext().arena().create<RecordStorageLocation>( Type, std::move(FieldLocs)); diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 28d062196cb07b..ca6c00343e58d8 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1446,6 +1446,99 @@ TEST(TransferTest, BaseClassInitializer) { llvm::Succeeded()); } +TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) { + std::string Code = R"( + struct Base1 { + int base1_1; + int base1_2; + }; + struct Intermediate : Base1 { + int intermediate_1; + int intermediate_2; + }; + struct Base2 { + int base2_1; + int base2_2; + }; + struct MostDerived : public Intermediate, Base2 { + int most_derived_1; + int most_derived_2; + }; + + void target() { + MostDerived MD; + MD.base1_2 = 1; + MD.intermediate_2 = 1; + MD.base2_2 = 1; + MD.most_derived_2 = 1; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + const Environment &Env = + getEnvironmentAtAnnotation(Results, "p"); + + // Only the accessed fields should exist in the model. + auto &MDLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MD"); + std::vector<const ValueDecl*> Fields; + for (auto [Field, _] : MDLoc.children()) + Fields.push_back(Field); + ASSERT_THAT(Fields, UnorderedElementsAre( + findValueDecl(ASTCtx, "base1_2"), + findValueDecl(ASTCtx, "intermediate_2"), + findValueDecl(ASTCtx, "base2_2"), + findValueDecl(ASTCtx, "most_derived_2"))); + }); +} + +TEST(TransferTest, StructInitializerListWithComplicatedInheritance) { + std::string Code = R"( + struct Base1 { + int base1; + }; + struct Intermediate : Base1 { + int intermediate; + }; + struct Base2 { + int base2; + }; + struct MostDerived : public Intermediate, Base2 { + int most_derived; + }; + + void target() { + MostDerived MD = {}; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + const Environment &Env = + getEnvironmentAtAnnotation(Results, "p"); + + // When a struct is initialized with a initializer list, all the + // fields are considered "accessed", and therefore do exist. + auto &MD = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MD"); + ASSERT_THAT(cast<IntegerValue>( + getFieldValue(&MD, *findValueDecl(ASTCtx, "base1"), Env)), + NotNull()); + ASSERT_THAT(cast<IntegerValue>( + getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate"), Env)), + NotNull()); + ASSERT_THAT(cast<IntegerValue>( + getFieldValue(&MD, *findValueDecl(ASTCtx, "base2"), Env)), + NotNull()); + ASSERT_THAT(cast<IntegerValue>( + getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived"), Env)), + NotNull()); + }); +} + TEST(TransferTest, ReferenceMember) { std::string Code = R"( struct A { @@ -2051,6 +2144,26 @@ TEST(TransferTest, AssignmentOperatorFromCallResult) { }); } +TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) { + // This is a crash repro. + std::string Code = R"( + struct B { int Foo; }; + struct S : public B {}; + void target() { + S S1 = { 1 }; + S S2; + S S3; + S1 = S2; // Only Dst has InitListExpr. + S3 = S1; // Only Src has InitListExpr. + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) {}); +} + TEST(TransferTest, CopyConstructor) { std::string Code = R"( struct A { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
