[clang][dataflow] Enable use of synthetic properties on all Value instances.

[wyt]

Summary

This patch moves the implementation of synthetic properties from the StructValue class into the Value base class so that it can be used across all Value instances.

[ymandel]

Thanks!

[sgatev]

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

29

I'm not sure I understand what is meant by "separately allocated" and "stored by pointer". Could you please clarify?

[gribozavr2]

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

29–30

[wyt]

Remove cast to StructValues since get/setProperty can be used directly on Values now

[wyt]

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

29

(See suggested fix by Dmitri)
When we create a new Value instance, we let DataflowAnalysisCtx take ownership of the value and refer to it by pointer.

[gribozavr2]

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

28–31

Please add a blank line between the short description and the rest.

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

 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <cassert>
 #include <utility>
 
 namespace clang {
 namespace dataflow {
 
 /// Base class for all values computed by abstract interpretation.
+///
+/// Don't use `Value` instances by value. All `Value` instances are allocated
+/// and owned by `DataflowAnalysisContext`.
 class Value {
 public:
   enum class Kind {
     Integer,
     Reference,
     Pointer,
     Struct,
 
     // Synthetic boolean values are either atomic values or composites that
     // represent conjunctions, disjunctions, and negations.
     AtomicBool,
     Conjunction,
     Disjunction,
     Negation
   };
 
   explicit Value(Kind ValKind) : ValKind(ValKind) {}
 
   virtual ~Value() = default;
 
   Kind getKind() const { return ValKind; }
 
+  /// Returns the value of the synthetic property with the given `Name` or null
+  /// if the property isn't assigned a value.
+  Value *getProperty(llvm::StringRef Name) const {
+    auto It = Properties.find(Name);
+    return It == Properties.end() ? nullptr : It->second;
+  }
+
+  /// Assigns `Val` as the value of the synthetic property with the given
+  /// `Name`.
+  void setProperty(llvm::StringRef Name, Value &Val) {
+    Properties.insert_or_assign(Name, &Val);
+  }
+
 private:
   Kind ValKind;
+  llvm::StringMap<Value *> Properties;
 };
 
 /// Models a boolean.
 class BoolValue : public Value {
 public:
   explicit BoolValue(Kind ValueKind) : Value(ValueKind) {}
 
   static bool classof(const Value *Val) {
Value *getChild(const ValueDecl &D) const {
     if (It == Children.end())
       return nullptr;
     return It->second;
   }
 
   /// Assigns `Val` as the child value for `D`.
   void setChild(const ValueDecl &D, Value &Val) { Children[&D] = &Val; }
 
-  /// Returns the value of the synthetic property with the given `Name` or null
-  /// if the property isn't assigned a value.
-  Value *getProperty(llvm::StringRef Name) const {
-    auto It = Properties.find(Name);
-    return It == Properties.end() ? nullptr : It->second;
-  }
-
-  /// Assigns `Val` as the value of the synthetic property with the given
-  /// `Name`.
-  void setProperty(llvm::StringRef Name, Value &Val) {
-    Properties.insert_or_assign(Name, &Val);
-  }
-
 private:
   llvm::DenseMap<const ValueDecl *, Value *> Children;
-  llvm::StringMap<Value *> Properties;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_VALUE_H

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

 /// symbolic value of its "has_value" property.
 StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
   auto OptionalVal = std::make_unique<StructValue>();
   OptionalVal->setProperty("has_value", HasValueVal);
   return Env.takeOwnership(std::move(OptionalVal));
 }
 
 /// Returns the symbolic value that represents the "has_value" property of the
-/// optional value `Val`. Returns null if `Val` is null.
-BoolValue *getHasValue(Value *Val) {
-  if (auto *OptionalVal = cast_or_null<StructValue>(Val)) {
+/// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
+BoolValue *getHasValue(Value *OptionalVal) {
+  if (OptionalVal) {
     return cast<BoolValue>(OptionalVal->getProperty("has_value"));
   }
   return nullptr;
 }
 
 /// If `Type` is a reference type, returns the type of its pointee. Otherwise,
 /// returns `Type` itself.
 QualType stripReference(QualType Type) {
return 1 + countOptionalWrappers(
                      .get(0)
                      .getAsType()
                      .getDesugaredType(ASTCtx));
 }
 
 void initializeOptionalReference(const Expr *OptionalExpr,
                                  const MatchFinder::MatchResult &,
                                  LatticeTransferState &State) {
-  if (auto *OptionalVal = cast_or_null<StructValue>(
-          State.Env.getValue(*OptionalExpr, SkipPast::Reference))) {
+  if (auto *OptionalVal =
+          State.Env.getValue(*OptionalExpr, SkipPast::Reference)) {
     if (OptionalVal->getProperty("has_value") == nullptr) {
       OptionalVal->setProperty("has_value", State.Env.makeAtomicBoolValue());
     }
   }
 }
 
 void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
                         LatticeTransferState &State) {
-  if (auto *OptionalVal = cast_or_null<StructValue>(
-          State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer))) {
+  if (auto *OptionalVal =
+          State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) {
     auto *HasValueVal = getHasValue(OptionalVal);
     assert(HasValueVal != nullptr);
 
     if (State.Env.flowConditionImplies(*HasValueVal))
       return;
   }
 
   // Record that this unwrap is *not* provably safe.

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

public:
 
   void transfer(const Stmt *S, NoopLattice &, Environment &Env) {
     auto SpecialBoolRecordDecl = recordDecl(hasName("SpecialBool"));
     auto HasSpecialBoolType = hasType(SpecialBoolRecordDecl);
 
     if (const auto *E = selectFirst<CXXConstructExpr>(
             "call", match(cxxConstructExpr(HasSpecialBoolType).bind("call"), *S,
                           getASTContext()))) {
-      auto &ConstructorVal = *cast<StructValue>(Env.createValue(E->getType()));
+      auto &ConstructorVal = *Env.createValue(E->getType());
       ConstructorVal.setProperty("is_set", Env.getBoolLiteralValue(false));
       Env.setValue(*Env.getStorageLocation(*E, SkipPast::None), ConstructorVal);
     } else if (const auto *E = selectFirst<CXXMemberCallExpr>(
                    "call", match(cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(
                                                        SpecialBoolRecordDecl))))
                                      .bind("call"),
                                  *S, getASTContext()))) {
       auto *Object = E->getImplicitObjectArgument();
       assert(Object != nullptr);
 
       auto *ObjectLoc =
           Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer);
       assert(ObjectLoc != nullptr);
 
-      auto &ConstructorVal =
-          *cast<StructValue>(Env.createValue(Object->getType()));
+      auto &ConstructorVal = *Env.createValue(Object->getType());
       ConstructorVal.setProperty("is_set", Env.getBoolLiteralValue(true));
       Env.setValue(*ObjectLoc, ConstructorVal);
     }
   }
 
   bool compareEquivalent(QualType Type, const Value &Val1,
                          const Environment &Env1, const Value &Val2,
                          const Environment &Env2) final {
     const auto *Decl = Type->getAsCXXRecordDecl();
     if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
         Decl->getName() != "SpecialBool")
       return false;
 
-    auto *IsSet1 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val1)->getProperty("is_set"));
+    auto *IsSet1 = cast_or_null<BoolValue>(Val1.getProperty("is_set"));
     if (IsSet1 == nullptr)
       return true;
 
-    auto *IsSet2 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val2)->getProperty("is_set"));
+    auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
     if (IsSet2 == nullptr)
       return false;
 
     return Env1.flowConditionImplies(*IsSet1) ==
            Env2.flowConditionImplies(*IsSet2);
   }
 
   // Always returns `true` to accept the `MergedVal`.
   bool merge(QualType Type, const Value &Val1, const Environment &Env1,
              const Value &Val2, const Environment &Env2, Value &MergedVal,
              Environment &MergedEnv) final {
     const auto *Decl = Type->getAsCXXRecordDecl();
     if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
         Decl->getName() != "SpecialBool")
       return true;
 
-    auto *IsSet1 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val1)->getProperty("is_set"));
+    auto *IsSet1 = cast_or_null<BoolValue>(Val1.getProperty("is_set"));
     if (IsSet1 == nullptr)
       return true;
 
-    auto *IsSet2 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val2)->getProperty("is_set"));
+    auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
     if (IsSet2 == nullptr)
       return true;
 
     auto &IsSet = MergedEnv.makeAtomicBoolValue();
-    cast<StructValue>(&MergedVal)->setProperty("is_set", IsSet);
+    MergedVal.setProperty("is_set", IsSet);
     if (Env1.flowConditionImplies(*IsSet1) &&
         Env2.flowConditionImplies(*IsSet2))
       MergedEnv.addToFlowCondition(IsSet);
 
     return true;
   }
 };
 
void target(bool Cond) {
       } else {
         Foo.set();
         /*[[p3]]*/
       }
       (void)0;
       /*[[p4]]*/
     }
   )";
-  runDataflow(Code,
-              [](llvm::ArrayRef<
-                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
-                     Results,
-                 ASTContext &ASTCtx) {
-                ASSERT_THAT(Results, ElementsAre(Pair("p4", _), Pair("p3", _),
-                                                 Pair("p2", _), Pair("p1", _)));
-                const Environment &Env1 = Results[3].second.Env;
-                const Environment &Env2 = Results[2].second.Env;
-                const Environment &Env3 = Results[1].second.Env;
-                const Environment &Env4 = Results[0].second.Env;
+  runDataflow(
+      Code, [](llvm::ArrayRef<
 
-                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-                ASSERT_THAT(FooDecl, NotNull());
 
-                auto GetFooValue = [FooDecl](const Environment &Env) {
-                  return cast<BoolValue>(
-                      cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None))
-                          ->getProperty("is_set"));
-                };
+              Env.getValue(*FooDecl, SkipPast::None)->getProperty("is_set"));
 
-                EXPECT_FALSE(Env1.flowConditionImplies(*GetFooValue(Env1)));
-                EXPECT_TRUE(Env2.flowConditionImplies(*GetFooValue(Env2)));
-                EXPECT_TRUE(Env3.flowConditionImplies(*GetFooValue(Env3)));
-                EXPECT_TRUE(Env4.flowConditionImplies(*GetFooValue(Env3)));
-              });
 }
 
 class OptionalIntAnalysis
     : public DataflowAnalysis<OptionalIntAnalysis, NoopLattice> {
 public:
   explicit OptionalIntAnalysis(ASTContext &Context, BoolValue &HasValueTop)
       : DataflowAnalysis<OptionalIntAnalysis, NoopLattice>(Context),
         HasValueTop(HasValueTop) {}
 
   static NoopLattice initialElement() { return {}; }
 
   void transfer(const Stmt *S, NoopLattice &, Environment &Env) {
     auto OptionalIntRecordDecl = recordDecl(hasName("OptionalInt"));
     auto HasOptionalIntType = hasType(OptionalIntRecordDecl);
 
     if (const auto *E = selectFirst<CXXConstructExpr>(
             "call", match(cxxConstructExpr(HasOptionalIntType).bind("call"), *S,
                           getASTContext()))) {
-      auto &ConstructorVal = *cast<StructValue>(Env.createValue(E->getType()));
+      auto &ConstructorVal = *Env.createValue(E->getType());
       ConstructorVal.setProperty("has_value", Env.getBoolLiteralValue(false));
       Env.setValue(*Env.getStorageLocation(*E, SkipPast::None), ConstructorVal);
     } else if (const auto *E = selectFirst<CXXOperatorCallExpr>(
                    "call",
                    match(cxxOperatorCallExpr(callee(cxxMethodDecl(ofClass(
                                                  OptionalIntRecordDecl))))
                              .bind("call"),
                          *S, getASTContext()))) {
       assert(E->getNumArgs() > 0);
       auto *Object = E->getArg(0);
       assert(Object != nullptr);
 
       auto *ObjectLoc =
           Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer);
       assert(ObjectLoc != nullptr);
 
-      auto &ConstructorVal =
-          *cast<StructValue>(Env.createValue(Object->getType()));
+      auto &ConstructorVal = *Env.createValue(Object->getType());
       ConstructorVal.setProperty("has_value", Env.getBoolLiteralValue(true));
       Env.setValue(*ObjectLoc, ConstructorVal);
     }
   }
 
   bool compareEquivalent(QualType Type, const Value &Val1,
                          const Environment &Env1, const Value &Val2,
                          const Environment &Env2) final {
     // Nothing to say about a value that does not model an `OptionalInt`.
     if (!Type->isRecordType() ||
         Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
       return false;
 
-    return cast<StructValue>(&Val1)->getProperty("has_value") ==
-           cast<StructValue>(&Val2)->getProperty("has_value");
+    return Val1.getProperty("has_value") == Val2.getProperty("has_value");
   }
 
   bool merge(QualType Type, const Value &Val1, const Environment &Env1,
              const Value &Val2, const Environment &Env2, Value &MergedVal,
              Environment &MergedEnv) final {
     // Nothing to say about a value that does not model an `OptionalInt`.
     if (!Type->isRecordType() ||
         Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
       return false;
 
-    auto *HasValue1 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val1)->getProperty("has_value"));
+    auto *HasValue1 = cast_or_null<BoolValue>(Val1.getProperty("has_value"));
     if (HasValue1 == nullptr)
       return false;
 
-    auto *HasValue2 = cast_or_null<BoolValue>(
-        cast<StructValue>(&Val2)->getProperty("has_value"));
+    auto *HasValue2 = cast_or_null<BoolValue>(Val2.getProperty("has_value"));
     if (HasValue2 == nullptr)
       return false;
 
     if (HasValue1 == HasValue2)
-      cast<StructValue>(&MergedVal)->setProperty("has_value", *HasValue1);
+      MergedVal.setProperty("has_value", *HasValue1);
     else
-      cast<StructValue>(&MergedVal)->setProperty("has_value", HasValueTop);
+      MergedVal.setProperty("has_value", HasValueTop);
     return true;
   }
 
   BoolValue &HasValueTop;
 };
 
 class WideningTest : public Test {
 protected:
runDataflow(
         const Environment &Env1 = Results[2].second.Env;
         const Environment &Env2 = Results[1].second.Env;
         const Environment &Env3 = Results[0].second.Env;
 
         const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
         ASSERT_THAT(FooDecl, NotNull());
 
         auto GetFooValue = [FooDecl](const Environment &Env) {
-          return cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None));
+          return Env.getValue(*FooDecl, SkipPast::None);
         };
 
         EXPECT_EQ(GetFooValue(Env1)->getProperty("has_value"),
                   &Env1.getBoolLiteralValue(false));
         EXPECT_EQ(GetFooValue(Env2)->getProperty("has_value"),
                   &Env2.getBoolLiteralValue(true));
         EXPECT_EQ(GetFooValue(Env3)->getProperty("has_value"), HasValueTop);
       });
void target(bool Cond) {
       } else {
         Foo = 2;
         /*[[p3]]*/
       }
       (void)0;
       /*[[p4]]*/
     }
   )";
-  runDataflow(
-      Code, [](llvm::ArrayRef<
+  runDataflow(Code,
+              [](llvm::ArrayRef<
                      std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
                      Results,
                  ASTContext &ASTCtx) {
                 ASSERT_THAT(Results, ElementsAre(Pair("p4", _), Pair("p3", _),
                                                  Pair("p2", _), Pair("p1", _)));
                 const Environment &Env1 = Results[3].second.Env;
                 const Environment &Env2 = Results[2].second.Env;
                 const Environment &Env3 = Results[1].second.Env;
                 const Environment &Env4 = Results[0].second.Env;
 
                 const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
                 ASSERT_THAT(FooDecl, NotNull());
 
                 auto GetFooValue = [FooDecl](const Environment &Env) {
-          return cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None));
+                  return Env.getValue(*FooDecl, SkipPast::None);
                 };
 
                 EXPECT_EQ(GetFooValue(Env1)->getProperty("has_value"),
                           &Env1.getBoolLiteralValue(false));
                 EXPECT_EQ(GetFooValue(Env2)->getProperty("has_value"),
                           &Env2.getBoolLiteralValue(true));
                 EXPECT_EQ(GetFooValue(Env3)->getProperty("has_value"),
                           &Env3.getBoolLiteralValue(true));
                 EXPECT_EQ(GetFooValue(Env4)->getProperty("has_value"),
                           &Env4.getBoolLiteralValue(true));
               });
 }
 
 TEST_F(WideningTest, DistinctPointersToTheSameLocationAreEquivalent) {
   std::string Code = R"(
     void target(int Foo, bool Cond) {
       int *Bar = &Foo;
       while (Cond) {
         Bar = &Foo;
runDataflow(Code,
                      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());
 
-                const auto *FooVal =
-                    cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None));
+                const auto *FooVal = Env.getValue(*FooDecl, SkipPast::None);
                 EXPECT_EQ(FooVal->getProperty("has_value"),
                           &Env.getBoolLiteralValue(true));
               });
 }
 
 class FlowConditionTest : public Test {
 protected:
   template <typename Matcher>