kinu updated this revision to Diff 555842. kinu marked 5 inline comments as done. kinu added a comment.
Updated again... very easy to overlook unaddressed comments! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159284/new/ https://reviews.llvm.org/D159284 Files: clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp =================================================================== --- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1436,6 +1436,99 @@ 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 { @@ -2041,6 +2134,26 @@ }); } +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 { Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp =================================================================== --- clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ 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 @@ 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));
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits