[clang] 67136d0 - [clang][dataflow] Remove private-field filtering from `StorageLocation` creation.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri May 27 06:53:36 PDT 2022
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) {
More information about the cfe-commits
mailing list