From 36d4e84427a704599bfd8bd72edf46ecd27ff5e5 Mon Sep 17 00:00:00 2001 From: Yitzhak Mandelbaum Date: Wed, 23 Mar 2022 00:01:55 +0000 Subject: [PATCH] [clang][dataflow] Fix handling of base-class fields. Currently, the framework does not track derived class access to base fields. This patch adds that support and a corresponding test. Differential Revision: https://reviews.llvm.org/D122273 --- .../clang/Analysis/FlowSensitive/StorageLocation.h | 4 +- clang/include/clang/Analysis/FlowSensitive/Value.h | 4 +- .../Analysis/FlowSensitive/DataflowEnvironment.cpp | 42 +++++- .../Analysis/FlowSensitive/TransferTest.cpp | 168 +++++++++++++++++++++ 4 files changed, 213 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h index 5532813..f965486 100644 --- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h +++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h @@ -56,7 +56,9 @@ public: /// A storage location which is subdivided into smaller storage locations that /// can be traced independently by abstract interpretation. For example: a -/// struct with public members. +/// struct with public members. The child map is flat, so when used for a struct +/// or class type, all accessible members of base struct and class types are +/// directly accesible as children of this location. class AggregateStorageLocation final : public StorageLocation { public: explicit AggregateStorageLocation(QualType Type) diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h index 98df8f6..859cf7f 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Value.h +++ b/clang/include/clang/Analysis/FlowSensitive/Value.h @@ -189,7 +189,9 @@ public: } }; -/// Models a value of `struct` or `class` type. +/// Models a value of `struct` or `class` type, with a flat map of fields to +/// child storage locations, containing all accessible members of base struct +/// and class types. class StructValue final : public Value { public: StructValue() : StructValue(llvm::DenseMap()) {} diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 0e75b1a..79bbfa0 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -164,6 +164,42 @@ joinConstraints(DataflowAnalysisContext *Context, return JoinedConstraints; } +static void +getFieldsFromClassHierarchy(QualType Type, bool IgnorePrivateFields, + llvm::DenseSet &Fields) { + if (Type->isIncompleteType() || Type->isDependentType() || + !Type->isRecordType()) + return; + + for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) { + if (IgnorePrivateFields && + (Field->getAccess() == AS_private || + (Field->getAccess() == AS_none && Type->getAsRecordDecl()->isClass()))) + continue; + Fields.insert(Field); + } + if (auto *CXXRecord = Type->getAsCXXRecordDecl()) { + for (const CXXBaseSpecifier &Base : CXXRecord->bases()) { + // Ignore private fields (including default access in C++ classes) in + // base classes, because they are not visible in derived classes. + getFieldsFromClassHierarchy(Base.getType(), /*IgnorePrivateFields=*/true, + Fields); + } + } +} + +/// Gets the set of all fields accesible from the type. +/// +/// FIXME: Does not precisely handle non-virtual diamond inheritance. A single +/// field decl will be modeled for all instances of the inherited field. +static llvm::DenseSet +getAccessibleObjectFields(QualType Type) { + llvm::DenseSet Fields; + // Don't ignore private fields for the class itself, only its super classes. + getFieldsFromClassHierarchy(Type, /*IgnorePrivateFields=*/false, Fields); + return Fields; +} + Environment::Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx) : Environment(DACtx) { @@ -296,7 +332,7 @@ StorageLocation &Environment::createStorageLocation(QualType Type) { // FIXME: Explore options to avoid eager initialization of fields as some of // them might not be needed for a particular analysis. llvm::DenseMap FieldLocs; - for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) { + for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { FieldLocs.insert({Field, &createStorageLocation(Field->getType())}); } return takeOwnership( @@ -363,7 +399,7 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) { const QualType Type = AggregateLoc.getType(); assert(Type->isStructureOrClassType()); - for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) { + for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { assert(Field != nullptr); StorageLocation &FieldLoc = AggregateLoc.getChild(*Field); MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field); @@ -479,7 +515,7 @@ Value *Environment::createValueUnlessSelfReferential( // FIXME: Initialize only fields that are accessed in the context that is // being analyzed. llvm::DenseMap FieldValues; - for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) { + for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { assert(Field != nullptr); QualType FieldType = Field->getType(); diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index f1335fb..d9b38f0 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1009,6 +1009,174 @@ TEST_F(TransferTest, StructMember) { }); } +TEST_F(TransferTest, DerivedBaseMemberClass) { + std::string Code = R"( + class A { + int ADefault; + protected: + int AProtected; + private: + int APrivate; + public: + int APublic; + }; + + class B : public A { + int BDefault; + protected: + int BProtected; + private: + int BPrivate; + }; + + void target() { + B Foo; + // [[p]] + } + )"; + runDataflow( + Code, [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + ASSERT_TRUE(FooDecl->getType()->isRecordType()); + + // Derived-class fields. + const FieldDecl *BDefaultDecl = nullptr; + const FieldDecl *BProtectedDecl = nullptr; + const FieldDecl *BPrivateDecl = nullptr; + for (const FieldDecl *Field : + FooDecl->getType()->getAsRecordDecl()->fields()) { + if (Field->getNameAsString() == "BDefault") { + BDefaultDecl = Field; + } else if (Field->getNameAsString() == "BProtected") { + BProtectedDecl = Field; + } else if (Field->getNameAsString() == "BPrivate") { + BPrivateDecl = Field; + } else { + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + } + ASSERT_THAT(BDefaultDecl, NotNull()); + ASSERT_THAT(BProtectedDecl, NotNull()); + ASSERT_THAT(BPrivateDecl, NotNull()); + + // Base-class fields. + const FieldDecl *ADefaultDecl = nullptr; + const FieldDecl *APrivateDecl = nullptr; + const FieldDecl *AProtectedDecl = nullptr; + const FieldDecl *APublicDecl = nullptr; + for (const clang::CXXBaseSpecifier &Base : + FooDecl->getType()->getAsCXXRecordDecl()->bases()) { + QualType BaseType = Base.getType(); + ASSERT_TRUE(BaseType->isRecordType()); + for (const FieldDecl *Field : BaseType->getAsRecordDecl()->fields()) { + if (Field->getNameAsString() == "ADefault") { + ADefaultDecl = Field; + } else if (Field->getNameAsString() == "AProtected") { + AProtectedDecl = Field; + } else if (Field->getNameAsString() == "APrivate") { + APrivateDecl = Field; + } else if (Field->getNameAsString() == "APublic") { + APublicDecl = Field; + } else { + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + } + } + ASSERT_THAT(ADefaultDecl, NotNull()); + ASSERT_THAT(AProtectedDecl, NotNull()); + ASSERT_THAT(APrivateDecl, NotNull()); + ASSERT_THAT(APublicDecl, NotNull()); + + const auto &FooLoc = *cast( + Env.getStorageLocation(*FooDecl, SkipPast::None)); + const auto &FooVal = *cast(Env.getValue(FooLoc)); + + // Note: we can't test presence of children in `FooLoc`, because + // `getChild` requires its argument be present (or fails an assert). So, + // we limit to testing presence in `FooVal` and coherence between the + // two. + + // Base-class fields. + EXPECT_THAT(FooVal.getChild(*ADefaultDecl), IsNull()); + EXPECT_THAT(FooVal.getChild(*APrivateDecl), IsNull()); + + EXPECT_THAT(FooVal.getChild(*AProtectedDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*APublicDecl)), + FooVal.getChild(*APublicDecl)); + EXPECT_THAT(FooVal.getChild(*APublicDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*AProtectedDecl)), + FooVal.getChild(*AProtectedDecl)); + + // Derived-class fields. + EXPECT_THAT(FooVal.getChild(*BDefaultDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*BDefaultDecl)), + FooVal.getChild(*BDefaultDecl)); + EXPECT_THAT(FooVal.getChild(*BProtectedDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*BProtectedDecl)), + FooVal.getChild(*BProtectedDecl)); + EXPECT_THAT(FooVal.getChild(*BPrivateDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*BPrivateDecl)), + FooVal.getChild(*BPrivateDecl)); + }); +} + +TEST_F(TransferTest, DerivedBaseMemberStructDefault) { + std::string Code = R"( + struct A { + int Bar; + }; + struct B : public A { + }; + + void target() { + B Foo; + // [[p]] + } + )"; + runDataflow( + Code, [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + ASSERT_TRUE(FooDecl->getType()->isRecordType()); + const FieldDecl *BarDecl = nullptr; + for (const clang::CXXBaseSpecifier &Base : + FooDecl->getType()->getAsCXXRecordDecl()->bases()) { + QualType BaseType = Base.getType(); + ASSERT_TRUE(BaseType->isStructureType()); + + for (const FieldDecl *Field : BaseType->getAsRecordDecl()->fields()) { + if (Field->getNameAsString() == "Bar") { + BarDecl = Field; + } else { + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + } + } + ASSERT_THAT(BarDecl, NotNull()); + + const auto &FooLoc = *cast( + Env.getStorageLocation(*FooDecl, SkipPast::None)); + const auto &FooVal = *cast(Env.getValue(FooLoc)); + EXPECT_THAT(FooVal.getChild(*BarDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), + FooVal.getChild(*BarDecl)); + }); +} + TEST_F(TransferTest, ClassMember) { std::string Code = R"( class A { -- 2.7.4