[clang][dataflow] Fix handling of base-class fields

[ymandel]

Summary

This patch adds support for tracking of non-private base-class fields in derived classes.

[ymandel]

Added repros for equality-related failures.

[ymandel]

c

[ymandel]

added missing call

[ymandel]

refactor

[ymandel]

tweak

[sgatev]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

168

Let's add to the documentation of AggregateStorageLocation and StructValue that they implement a flat struct layout. I don't see an immediate reason to revisit this, but let's be explicit about it.

182

The clang namespace is unnecessary.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

1014

Add a similar test with class instead of struct?

1015

Let's also add private and protected members in A and a private member in B.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

185

Will this work well for all cases of diamond shape inheritance?

E.g.:

struct A { int a; };
struct B : virtual A { int b; };
struct C : virtual A { int c; };
struct D : B, C { int d; };

In the above code, I would expect the field a to appear only once. I guess this should work well because of the set representation although we will visit A twice.

On the other hand, consider:

struct A { int a; };
struct B : A { int b; };
struct C : A { int c; };
struct D : B, C { int d; };

Here, in fact, we have 2 instances of the field a. Both B::a and C::a are part of D. I suspect that the current implementation might not handle this.

[ymandel]

address reviewer comments

[ymandel]

adjusted test

[ymandel]

Thanks for the reviews.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

185

Good point, no. But, I'm not sure that fixing this here would be enough - I think we'd also need to revisit how we model structs, since getChild takes a decl as argument -- it doesn't support multiple versions of the same field decl.

I added a FIXME since this seems a larger issue, but let me know if you think it needs fixing now.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

1014

I went with a simpler test for struct, only verifying that the default is understood correctly (and differently than class), but happy to expand it.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

185

I think the argument of getChild needs fixing as well is strong enough to push this to a separate PR.

[sgatev]

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

59–62

I find this a bit confusing. StructValue does not contain storage locations in general. I think we should make it clear that the layout of AggregateStorageLocation is flat, i.e. if it's used for a struct or class type it will contain child storage locations for all accessible members of base struct and class types.

clang/include/clang/Analysis/FlowSensitive/Value.h

193–194

I'm not sure what's meant here by indirect reachability, but I suggest documenting that a StructValue that models a struct or class type contains child values for all accessible members of its base struct and class types.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

1097–1100

1102–1111

Let's also check FooLoc's child storage locations. Same for the test below.

1156–1159

[ymandel]

address comments

[ymandel]

add clarifying comment

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

59–62

Updated. I misunderstood what you meant as "flat" in the previous round of comments. Thanks for clarifying!

[ymandel]

tweak comment

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

public:
 
   static bool classof(const StorageLocation *Loc) {
     return Loc->getKind() == Kind::Scalar;
   }
 };
 
 /// 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 layout is flat: when used for a struct or
+/// class type, it maintains a single, flat map of fields to child storage
+/// locations, containing all accessible members of base struct and class types.
 class AggregateStorageLocation final : public StorageLocation {
 public:
   explicit AggregateStorageLocation(QualType Type)
       : AggregateStorageLocation(
             Type, llvm::DenseMap<const ValueDecl *, StorageLocation *>()) {}
 
   AggregateStorageLocation(
       QualType Type,
       llvm::DenseMap<const ValueDecl *, StorageLocation *> Children)

clang/include/clang/Analysis/FlowSensitive/Value.h

public:
   explicit PointerValue(StorageLocation &PointeeLoc)
       : IndirectionValue(Kind::Pointer, PointeeLoc) {}
 
   static bool classof(const Value *Val) {
     return Val->getKind() == Kind::Pointer;
   }
 };
 
-/// 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 *>()) {}
 
   explicit StructValue(llvm::DenseMap<const ValueDecl *, Value *> Children)
       : Value(Kind::Struct), Children(std::move(Children)) {}
 
   static bool classof(const Value *Val) {

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

joinConstraints(DataflowAnalysisContext *Context,
   // constraint only if both `Val1` and `Val2` are not null.
   if (Val1 != nullptr && Val2 != nullptr)
     JoinedConstraints.insert(
         &Context->getOrCreateDisjunctionValue(*Val1, *Val2));
 
   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) {
   if (const auto *FuncDecl = dyn_cast<FunctionDecl>(&DeclCtx)) {
     assert(FuncDecl->getBody() != nullptr);
     initGlobalVars(*FuncDecl->getBody(), *this);
     for (const auto *ParamDecl : FuncDecl->parameters()) {
       assert(ParamDecl != nullptr);

 }
 
 StorageLocation &Environment::createStorageLocation(QualType Type) {
   assert(!Type.isNull());
   if (Type->isStructureOrClassType() || Type->isUnionType()) {
     // 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(
         std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs)));
   }
   return takeOwnership(std::make_unique<ScalarStorageLocation>(Type));
 }
 
void Environment::setValue(const StorageLocation &Loc, Value &Val) {
   LocToVal[&Loc] = &Val;
 
   if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
     auto &AggregateLoc = *cast<AggregateStorageLocation>(&Loc);
 
     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);
       if (auto *FieldVal = StructVal->getChild(*Field))
         setValue(FieldLoc, *FieldVal);
     }
   }
 
if (Type->isPointerType()) {
     return &takeOwnership(std::make_unique<PointerValue>(PointeeLoc));
   }
 
   if (Type->isStructureOrClassType()) {
     CreatedValuesCount++;
     // 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();
       if (Visited.contains(FieldType.getCanonicalType()))
         continue;
 
       Visited.insert(FieldType.getCanonicalType());
       if (auto *FieldValue = createValueUnlessSelfReferential(

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
 
         const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
         ASSERT_THAT(BazDecl, NotNull());
 
         EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarVal);
       });
 }
 
+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 {
     public:
       int Bar;
     };
 
     void target(A Foo) {