[clang] 36d4e84 - [clang][dataflow] Fix handling of base-class fields.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 1 08:02:07 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-04-01T15:01:32Z
New Revision: 36d4e84427a704599bfd8bd72edf46ecd27ff5e5
URL: https://github.com/llvm/llvm-project/commit/36d4e84427a704599bfd8bd72edf46ecd27ff5e5
DIFF: https://github.com/llvm/llvm-project/commit/36d4e84427a704599bfd8bd72edf46ecd27ff5e5.diff
LOG: [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
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
clang/include/clang/Analysis/FlowSensitive/Value.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
index 5532813d6d29b..f965486537e61 100644
--- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -56,7 +56,9 @@ class ScalarStorageLocation final : public StorageLocation {
/// 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 98df8f6539f7b..859cf7ff21b5b 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Value.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -189,7 +189,9 @@ class PointerValue final : public IndirectionValue {
}
};
-/// 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<const ValueDecl *, Value *>()) {}
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0e75b1a3be016..79bbfa091f6cb 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<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;
+ 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<const FieldDecl *>
+getAccessibleObjectFields(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);
+ 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<const ValueDecl *, StorageLocation *> 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<const ValueDecl *, Value *> 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 f1335fbd49fa9..d9b38f0d67170 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<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());
+
+ // 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<AggregateStorageLocation>(
+ Env.getStorageLocation(*FooDecl, SkipPast::None));
+ const auto &FooVal = *cast<StructValue>(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<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, ClassMember) {
std::string Code = R"(
class A {
More information about the cfe-commits
mailing list