Author: Yitzhak Mandelbaum Date: 2022-05-27T13:39:53Z New Revision: 67136d0e8fb57251dece4be0907414fdbe081f7a
URL: https://github.com/llvm/llvm-project/commit/67136d0e8fb57251dece4be0907414fdbe081f7a DIFF: https://github.com/llvm/llvm-project/commit/67136d0e8fb57251dece4be0907414fdbe081f7a.diff LOG: [clang][dataflow] Remove private-field filtering from `StorageLocation` creation. The API for `AggregateStorageLocation` does not allow for missing fields (it asserts). Therefore, it is incorrect to filter out any fields at location-creation time which may be accessed by the code. Currently, we limit filtering to private, base-calss fields on the assumption that those can never be accessed. However, `friend` declarations can invalidate that assumption, thereby breaking our invariants. This patch removes said field filtering to avoid violating the invariant of "no missing fields" for `AggregateStorageLocation`. Differential Revision: https://reviews.llvm.org/D126420 Added: Modified: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/unittests/Analysis/FlowSensitive/TransferTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index fe573bbf9f379..13fef0d4286cf 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -148,39 +148,26 @@ static void initGlobalVars(const Stmt &S, Environment &Env) { } } +// FIXME: Does not precisely handle non-virtual diamond inheritance. A single +// field decl will be modeled for all instances of the inherited field. static void -getFieldsFromClassHierarchy(QualType Type, bool IgnorePrivateFields, +getFieldsFromClassHierarchy(QualType Type, llvm::DenseSet<const FieldDecl *> &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; + for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) 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); - } - } + if (auto *CXXRecord = Type->getAsCXXRecordDecl()) + for (const CXXBaseSpecifier &Base : CXXRecord->bases()) + getFieldsFromClassHierarchy(Base.getType(), 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<const FieldDecl *> -getAccessibleObjectFields(QualType Type) { +/// Gets the set of all fields in the type. +static llvm::DenseSet<const FieldDecl *> getObjectFields(QualType Type) { llvm::DenseSet<const FieldDecl *> Fields; - // Don't ignore private fields for the class itself, only its super classes. - getFieldsFromClassHierarchy(Type, /*IgnorePrivateFields=*/false, Fields); + getFieldsFromClassHierarchy(Type, Fields); return Fields; } @@ -324,9 +311,8 @@ 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<const ValueDecl *, StorageLocation *> FieldLocs; - for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { + for (const FieldDecl *Field : getObjectFields(Type)) FieldLocs.insert({Field, &createStorageLocation(Field->getType())}); - } return takeOwnership( std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs))); } @@ -392,7 +378,7 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) { const QualType Type = AggregateLoc.getType(); assert(Type->isStructureOrClassType()); - for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { + for (const FieldDecl *Field : getObjectFields(Type)) { assert(Field != nullptr); StorageLocation &FieldLoc = AggregateLoc.getChild(*Field); MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field); @@ -508,7 +494,7 @@ Value *Environment::createValueUnlessSelfReferential( // FIXME: Initialize only fields that are accessed in the context that is // being analyzed. llvm::DenseMap<const ValueDecl *, Value *> FieldValues; - for (const FieldDecl *Field : getAccessibleObjectFields(Type)) { + for (const FieldDecl *Field : getObjectFields(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 bed6b975974cc..dbf59bf695560 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1154,8 +1154,8 @@ TEST_F(TransferTest, DerivedBaseMemberClass) { // two. // Base-class fields. - EXPECT_THAT(FooVal.getChild(*ADefaultDecl), IsNull()); - EXPECT_THAT(FooVal.getChild(*APrivateDecl), IsNull()); + EXPECT_THAT(FooVal.getChild(*ADefaultDecl), NotNull()); + EXPECT_THAT(FooVal.getChild(*APrivateDecl), NotNull()); EXPECT_THAT(FooVal.getChild(*AProtectedDecl), NotNull()); EXPECT_EQ(Env.getValue(FooLoc.getChild(*APublicDecl)), @@ -1177,6 +1177,40 @@ TEST_F(TransferTest, DerivedBaseMemberClass) { }); } +static void derivedBaseMemberExpectations( + llvm::ArrayRef<std::pair<std::string, DataflowAnalysisState<NoopLattice>>> + 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<AggregateStorageLocation>( + Env.getStorageLocation(*FooDecl, SkipPast::None)); + const auto &FooVal = *cast<StructValue>(Env.getValue(FooLoc)); + EXPECT_THAT(FooVal.getChild(*BarDecl), NotNull()); + EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), FooVal.getChild(*BarDecl)); +} + TEST_F(TransferTest, DerivedBaseMemberStructDefault) { std::string Code = R"( struct A { @@ -1190,41 +1224,28 @@ TEST_F(TransferTest, DerivedBaseMemberStructDefault) { // [[p]] } )"; - runDataflow( - Code, [](llvm::ArrayRef< - std::pair<std::string, DataflowAnalysisState<NoopLattice>>> - 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()); + runDataflow(Code, derivedBaseMemberExpectations); +} - for (const FieldDecl *Field : BaseType->getAsRecordDecl()->fields()) { - if (Field->getNameAsString() == "Bar") { - BarDecl = Field; - } else { - FAIL() << "Unexpected field: " << Field->getNameAsString(); - } - } - } - ASSERT_THAT(BarDecl, NotNull()); +TEST_F(TransferTest, DerivedBaseMemberPrivateFriend) { + // Include an access to `Foo.Bar` to verify the analysis doesn't crash on that + // access. + std::string Code = R"( + struct A { + private: + friend void target(); + int Bar; + }; + struct B : public A { + }; - const auto &FooLoc = *cast<AggregateStorageLocation>( - Env.getStorageLocation(*FooDecl, SkipPast::None)); - const auto &FooVal = *cast<StructValue>(Env.getValue(FooLoc)); - EXPECT_THAT(FooVal.getChild(*BarDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), - FooVal.getChild(*BarDecl)); - }); + void target() { + B Foo; + (void)Foo.Bar; + // [[p]] + } + )"; + runDataflow(Code, derivedBaseMemberExpectations); } TEST_F(TransferTest, ClassMember) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits