[clang][dataflow] Remove private-field filtering from `StorageLocation` creation.

[ymandel]

Summary

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.

[sgatev]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

381

Why not use getObjectFields here too?

497

Why not use getObjectFields here too?

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

1274–1275

Might as well check that Foo.Bar is assigned a value like the DerivedBaseMemberClass test case?

[ymandel]

address review comments

[ymandel]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

381

We could. I figured it was still worth the optimization on the values. Usually, its correct and otherwise we lose some precision. But, now that you asked, I'm inclined to get rid of it altogether -- seems like we should have evidence of the benefit in practice before adopt an approach that we know costs us precision (in potentially surprising ways).

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

if (auto *DS = dyn_cast<DeclStmt>(&S)) {
     }
   } else if (auto *E = dyn_cast<DeclRefExpr>(&S)) {
     initGlobalVar(*E->getDecl(), Env);
   } else if (auto *E = dyn_cast<MemberExpr>(&S)) {
     initGlobalVar(*E->getMemberDecl(), 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;
 }
 
 Environment::Environment(DataflowAnalysisContext &DACtx)
     : DACtx(&DACtx), FlowConditionToken(&DACtx.makeFlowConditionToken()) {}
 
 Environment::Environment(const Environment &Other)
     : DACtx(Other.DACtx), DeclToLoc(Other.DeclToLoc),

 }
 
 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 : getAccessibleObjectFields(Type)) {
+    for (const FieldDecl *Field : getObjectFields(Type))
       FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
-    }
     return takeOwnership(
         std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs)));
   }
   return takeOwnership(std::make_unique<ScalarStorageLocation>(Type));
 }
 
 StorageLocation &Environment::createStorageLocation(const VarDecl &D) {
   // Evaluated declarations are always assigned the same storage locations to
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 : getAccessibleObjectFields(Type)) {
+    for (const FieldDecl *Field : getObjectFields(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 : getAccessibleObjectFields(Type)) {
+    for (const FieldDecl *Field : getObjectFields(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 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(*ADefaultDecl), NotNull());
+        EXPECT_THAT(FooVal.getChild(*APrivateDecl), NotNull());
 
         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;
+static void derivedBaseMemberExpectations(
+    llvm::ArrayRef<std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
 
-        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));
-      });
+  EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), FooVal.getChild(*BarDecl));
+}
+
+TEST_F(TransferTest, DerivedBaseMemberStructDefault) {
+  std::string Code = R"(
+    struct A {
+      int Bar;
+    };
+    struct B : public A {
+    };
+
+    void target() {
+      B Foo;
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, derivedBaseMemberExpectations);
+}
+
+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 {
+    };
+
+    void target() {
+      B Foo;
+      (void)Foo.Bar;
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, derivedBaseMemberExpectations);
 }
 
 TEST_F(TransferTest, ClassMember) {
   std::string Code = R"(
     class A {
     public:
       int Bar;
     };
runDataflow(
                    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()->isClassType());
         auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields();
 
         FieldDecl *BarDecl = nullptr;
         for (FieldDecl *Field : FooFields) {
           if (Field->getNameAsString() == "Bar") {
             BarDecl = Field;
           } else {
             FAIL() << "Unexpected field: " << Field->getNameAsString();
           }