Skip to content

Commit

Permalink
[clang][dataflow] Remove private-field filtering from `StorageLocatio…
Browse files Browse the repository at this point in the history 
…n` 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
  • Loading branch information
@ymand
ymand committed May 27, 2022
1 parent dae2c24 commit 67136d0
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 62 deletions.
40 changes: 13 additions & 27 deletions clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
View file
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)));
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
91 changes: 56 additions & 35 deletions clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
View file
Expand Up @@ -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)),
Expand All @@ -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 {
Expand All @@ -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) {
Expand Down

0 comments on commit 67136d0

Please sign in to comment.