[clang][dataflow] Generalize custom comparison to return tri-value result.

Currently, the API for a model's custom value comparison returns a
boolean. Therefore, models cannot distinguish between situations where the
values are recognized by the model and different and those where the values are
just not recognized.  This patch changes the return value to a tri-valued enum,
allowing models to express "don't know".

This patch is essentially a NFC -- no practical differences result from this
change in this patch. But, it prepares for future patches (particularly,
upcoming patches for widening) which will take advantage of the new flexibility.

Differential Revision: https://reviews.llvm.org/D137334

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index efea46b..e362d79 100644 (file)
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -48,6 +48,13 @@ enum class SkipPast {
   ReferenceThenPointer,
 };
 
+/// Indicates the result of a tentative comparison.
+enum class ComparisonResult {
+  Same,
+  Different,
+  Unknown,
+};
+
 /// Holds the state of the program (store and heap) at a given program point.
 ///
 /// WARNING: Symbolic values that are created by the environment for static
@@ -62,7 +69,11 @@ public:
   public:
     virtual ~ValueModel() = default;
 
-    /// Returns true if and only if `Val1` is equivalent to `Val2`.
+    /// Returns:
+    ///   `Same`: `Val1` is equivalent to `Val2`, according to the model.
+    ///   `Different`: `Val1` is distinct from `Val2`, according to the model.
+    ///   `Unknown`: The model can't determine a relationship between `Val1` and
+    ///    `Val2`.
     ///
     /// Requirements:
     ///
@@ -72,16 +83,16 @@ public:
     ///
     ///  `Val1` and `Val2` must be assigned to the same storage location in
     ///  `Env1` and `Env2` respectively.
-    virtual bool compareEquivalent(QualType Type, const Value &Val1,
-                                   const Environment &Env1, const Value &Val2,
-                                   const Environment &Env2) {
+    virtual ComparisonResult compare(QualType Type, const Value &Val1,
+                                     const Environment &Env1, const Value &Val2,
+                                     const Environment &Env2) {
       // FIXME: Consider adding QualType to StructValue and removing the Type
       // argument here.
       //
-      // FIXME: default to a sound comparison and/or expand the comparison logic
-      // built into the framework to support broader forms of equivalence than
-      // strict pointer equality.
-      return true;
+      // FIXME: default to a sound comparison (`Unknown`) and/or expand the
+      // comparison logic built into the framework to support broader forms of
+      // equivalence than strict pointer equality.
+      return ComparisonResult::Same;
     }
 
     /// Modifies `MergedVal` to approximate both `Val1` and `Val2`. This could

diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 66aabb5..b053a10 100644 (file)
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -54,9 +54,9 @@ public:
 
   void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env);
 
-  bool compareEquivalent(QualType Type, const Value &Val1,
-                         const Environment &Env1, const Value &Val2,
-                         const Environment &Env2) override;
+  ComparisonResult compare(QualType Type, const Value &Val1,
+                           const Environment &Env1, const Value &Val2,
+                           const Environment &Env2) override;
 
   bool merge(QualType Type, const Value &Val1, const Environment &Env1,
              const Value &Val2, const Environment &Env2, Value &MergedVal,

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0b098c4..ab1241d 100644 (file)
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -295,8 +295,8 @@ bool Environment::equivalentTo(const Environment &Other,
     assert(It->second != nullptr);
 
     if (!areEquivalentValues(*Val, *It->second) &&
-        !Model.compareEquivalent(Loc->getType(), *Val, *this, *It->second,
-                                 Other))
+        Model.compare(Loc->getType(), *Val, *this, *It->second, Other) !=
+            ComparisonResult::Same)
       return false;
   }
 

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 1ffd886..1a41cfa 100644 (file)
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@ QualType stripReference(QualType Type) {
 }
 
 /// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
   if (!Type->isRecordType())
     return false;
   // FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@ bool IsOptionalType(QualType Type) {
 /// For example, if `Type` is `optional<optional<int>>`, the result of this
 /// function will be 2.
 int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
-  if (!IsOptionalType(Type))
+  if (!isOptionalType(Type))
     return 0;
   return 1 + countOptionalWrappers(
                  ASTCtx,
@@ -720,12 +720,14 @@ void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
   TransferMatchSwitch(*Elt, getASTContext(), State);
 }
 
-bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
-                                                     const Value &Val1,
-                                                     const Environment &Env1,
-                                                     const Value &Val2,
-                                                     const Environment &Env2) {
-  return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2);
+ComparisonResult UncheckedOptionalAccessModel::compare(
+    QualType Type, const Value &Val1, const Environment &Env1,
+    const Value &Val2, const Environment &Env2) {
+  if (!isOptionalType(Type))
+    return ComparisonResult::Unknown;
+  return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2)
+             ? ComparisonResult::Same
+             : ComparisonResult::Different;
 }
 
 bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
@@ -734,7 +736,7 @@ bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
                                          const Environment &Env2,
                                          Value &MergedVal,
                                          Environment &MergedEnv) {
-  if (!IsOptionalType(Type))
+  if (!isOptionalType(Type))
     return true;
 
   auto &HasValueVal = MergedEnv.makeAtomicBoolValue();

diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index e36f207..8e0e27e 100644 (file)
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,27 @@ public:
     }
   }
 
-  bool compareEquivalent(QualType Type, const Value &Val1,
-                         const Environment &Env1, const Value &Val2,
-                         const Environment &Env2) override {
+  ComparisonResult compare(QualType Type, const Value &Val1,
+                           const Environment &Env1, const Value &Val2,
+                           const Environment &Env2) override {
     const auto *Decl = Type->getAsCXXRecordDecl();
     if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
         Decl->getName() != "SpecialBool")
-      return false;
+      return ComparisonResult::Unknown;
 
     auto *IsSet1 = cast_or_null<BoolValue>(Val1.getProperty("is_set"));
+    auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
     if (IsSet1 == nullptr)
-      return true;
+      return IsSet2 == nullptr ? ComparisonResult::Same
+                               : ComparisonResult::Different;
 
-    auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
     if (IsSet2 == nullptr)
-      return false;
+      return ComparisonResult::Different;
 
     return Env1.flowConditionImplies(*IsSet1) ==
-           Env2.flowConditionImplies(*IsSet2);
+                   Env2.flowConditionImplies(*IsSet2)
+               ? ComparisonResult::Same
+               : ComparisonResult::Different;
   }
 
   // Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@ public:
     }
   }
 
-  bool compareEquivalent(QualType Type, const Value &Val1,
-                         const Environment &Env1, const Value &Val2,
-                         const Environment &Env2) override {
+  ComparisonResult compare(QualType Type, const Value &Val1,
+                           const Environment &Env1, const Value &Val2,
+                           const Environment &Env2) override {
     // Nothing to say about a value that does not model an `OptionalInt`.
     if (!Type->isRecordType() ||
         Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
-      return false;
+      return ComparisonResult::Unknown;
 
     auto *Prop1 = Val1.getProperty("has_value");
     auto *Prop2 = Val2.getProperty("has_value");
     assert(Prop1 != nullptr && Prop2 != nullptr);
-    return areEquivalentValues(*Prop1, *Prop2);
+    return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
+                                               : ComparisonResult::Different;
   }
 
   bool merge(QualType Type, const Value &Val1, const Environment &Env1,
@@ -1182,12 +1186,12 @@ public:
     }
   }
 
-  bool compareEquivalent(QualType Type, const Value &Val1,
-                         const Environment &Env1, const Value &Val2,
-                         const Environment &Env2) override {
+  ComparisonResult compare(QualType Type, const Value &Val1,
+                           const Environment &Env1, const Value &Val2,
+                           const Environment &Env2) override {
     // Changes to a sound approximation, which allows us to test whether we can
     // (soundly) converge for some loops.
-    return false;
+    return ComparisonResult::Unknown;
   }
 };