[clang] 65ecbdf - [clang][dataflow] Fix bug in `Value` comparison. (#76746)

[via cfe-commits]

Author: Yitzhak Mandelbaum
Date: 2024-01-16T13:03:49-05:00
New Revision: 65ecbdf61f5a3fb53f05abc610b90a8671f93730

URL: https://github.com/llvm/llvm-project/commit/65ecbdf61f5a3fb53f05abc610b90a8671f93730
DIFF: https://github.com/llvm/llvm-project/commit/65ecbdf61f5a3fb53f05abc610b90a8671f93730.diff

LOG: [clang][dataflow] Fix bug in `Value` comparison. (#76746)

Makes value equivalence require that the values have no properties,
except in
the case of equivalence by pointer equality (if the pointers are equal,
nothing
else is checked).

Fixes issue #76459.

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Value.cpp
    clang/unittests/Analysis/FlowSensitive/ValueTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Value.cpp b/clang/lib/Analysis/FlowSensitive/Value.cpp
index 349f873f1e6c4d9..7fad6deb0e918fa 100644
--- a/clang/lib/Analysis/FlowSensitive/Value.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Value.cpp
@@ -27,9 +27,17 @@ static bool areEquivalentIndirectionValues(const Value &Val1,
 }
 
 bool areEquivalentValues(const Value &Val1, const Value &Val2) {
-  return &Val1 == &Val2 || (Val1.getKind() == Val2.getKind() &&
-                            (isa<TopBoolValue>(&Val1) ||
-                             areEquivalentIndirectionValues(Val1, Val2)));
+  if (&Val1 == &Val2)
+    return true;
+  if (Val1.getKind() != Val2.getKind())
+    return false;
+  // If values are distinct and have properties, we don't consider them equal,
+  // leaving equality up to the user model.
+  if (!Val1.properties().empty() || !Val2.properties().empty())
+    return false;
+  if (isa<TopBoolValue>(&Val1))
+    return true;
+  return areEquivalentIndirectionValues(Val1, Val2);
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const Value &Val) {

diff  --git a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
index c5d18ba74c3ed6b..b07328d2a562153 100644
--- a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
@@ -45,7 +45,20 @@ TEST(ValueTest, TopsEquivalent) {
   EXPECT_TRUE(areEquivalentValues(V2, V1));
 }
 
-TEST(ValueTest, EquivalentValuesWithDifferentPropsEquivalent) {
+// The framework does not (currently) consider equivalence for values with
+// properties, leaving such to individual analyses.
+TEST(ValueTest, ValuesWithSamePropsDifferent) {
+  Arena A;
+  TopBoolValue Prop(A.makeAtomRef(Atom(0)));
+  TopBoolValue V1(A.makeAtomRef(Atom(2)));
+  TopBoolValue V2(A.makeAtomRef(Atom(3)));
+  V1.setProperty("foo", Prop);
+  V2.setProperty("foo", Prop);
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
+}
+
+TEST(ValueTest, ValuesWithDifferentPropsDifferent) {
   Arena A;
   TopBoolValue Prop1(A.makeAtomRef(Atom(0)));
   TopBoolValue Prop2(A.makeAtomRef(Atom(1)));
@@ -53,8 +66,19 @@ TEST(ValueTest, EquivalentValuesWithDifferentPropsEquivalent) {
   TopBoolValue V2(A.makeAtomRef(Atom(3)));
   V1.setProperty("foo", Prop1);
   V2.setProperty("bar", Prop2);
-  EXPECT_TRUE(areEquivalentValues(V1, V2));
-  EXPECT_TRUE(areEquivalentValues(V2, V1));
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
+}
+
+TEST(ValueTest, ValuesWithDifferentNumberPropsDifferent) {
+  Arena A;
+  TopBoolValue Prop(A.makeAtomRef(Atom(0)));
+  TopBoolValue V1(A.makeAtomRef(Atom(2)));
+  TopBoolValue V2(A.makeAtomRef(Atom(3)));
+  // Only set a property on `V1`.
+  V1.setProperty("foo", Prop);
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
 }
 
 TEST(ValueTest, DifferentKindsNotEquivalent) {