ymandel created this revision.
ymandel added reviewers: sgatev, xazax.hun, li.zhe.hua.
Herald added subscribers: tschuett, steakhal, rnkovacs.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
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`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D126420
Files:
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.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
@@ -1226,6 +1226,35 @@
});
}
+TEST_F(TransferTest, DerivedBaseMemberPrivateFriend) {
+ // Include an access to `Foo.Bar` to verify the analysis doesn't crash on that
+ // access.
+ std::string Code = R"(
+ class A {
+ private:
+ friend void target();
+ int Bar;
+ };
+ struct B : public A {
+ };
+
+ void target() {
+ B Foo;
+ (void)Foo.Bar;
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code, [](llvm::ArrayRef<
+ std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+ Results,
+ ASTContext &ASTCtx) {
+ // The purpose of this test is to verify non-crashing behavior for the
+ // analysis of the access to `Foo.Bar`. So, we
+ EXPECT_THAT(Results, ElementsAre(Pair("p", _)));
+ });
+}
+
TEST_F(TransferTest, ClassMember) {
std::string Code = R"(
class A {
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -148,6 +148,8 @@
}
}
+// 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,
llvm::DenseSet<const FieldDecl *> &Fields) {
@@ -162,24 +164,40 @@
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);
- }
- }
+ if (auto *CXXRecord = Type->getAsCXXRecordDecl())
+ for (const CXXBaseSpecifier &Base : CXXRecord->bases())
+ getFieldsFromClassHierarchy(
+ Base.getType(), IgnorePrivateFields, Fields);
}
-/// Gets the set of all fields accesible from the type.
+/// Gets the set of all fields accesible from the type. Admits private fields of
+/// the type, but not of any base classes (which are presumably inaccessible.
///
-/// FIXME: Does not precisely handle non-virtual diamond inheritance. A single
-/// field decl will be modeled for all instances of the inherited field.
+/// FIXME: Does not account for friend declarations, which may make private
+/// fields visible.
static llvm::DenseSet<const FieldDecl *>
getAccessibleObjectFields(QualType Type) {
llvm::DenseSet<const FieldDecl *> Fields;
- // Don't ignore private fields for the class itself, only its super classes.
+ if (Type->isIncompleteType() || Type->isDependentType() ||
+ !Type->isRecordType())
+ return Fields;
+
+ 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);
+
+ return Fields;
+}
+
+/// Gets the set of all fields in the type.
+static llvm::DenseSet<const FieldDecl *> getObjectFields(QualType Type) {
+ llvm::DenseSet<const FieldDecl *> Fields;
getFieldsFromClassHierarchy(Type, /*IgnorePrivateFields=*/false, Fields);
return Fields;
}
@@ -319,9 +337,8 @@
// 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)));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
