Skip to content

[clang][dataflow] Fix bug in Value comparison. #76746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Conversation

ymand
Copy link
Collaborator

@ymand ymand commented Jan 2, 2024

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.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Jan 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2024

@llvm/pr-subscribers-clang-analysis

Author: Yitzhak Mandelbaum (ymand)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/76746.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/FlowSensitive/Value.cpp (+7-3)
  • (modified) clang/unittests/Analysis/FlowSensitive/ValueTest.cpp (+2-2)
diff --git a/clang/lib/Analysis/FlowSensitive/Value.cpp b/clang/lib/Analysis/FlowSensitive/Value.cpp
index 349f873f1e6c4d..50475ef5553036 100644
--- a/clang/lib/Analysis/FlowSensitive/Value.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Value.cpp
@@ -27,9 +27,13 @@ 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 values are distinct and have properties, we don't consider them equal,
+  // leaving equality up to the user model.
+  return &Val1 == &Val2 ||
+         (Val1.getKind() == Val2.getKind() &&
+          // (Val1.properties().empty() && Val2.properties().empty()) &&
+          (isa<TopBoolValue>(&Val1) ||
+           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 c5d18ba74c3ed6..2060b7eb264f74 100644
--- a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
@@ -53,8 +53,8 @@ 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, DifferentKindsNotEquivalent) {

@llvmbot
Copy link
Member

llvmbot commented Jan 2, 2024

@llvm/pr-subscribers-clang

Author: Yitzhak Mandelbaum (ymand)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/76746.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/FlowSensitive/Value.cpp (+7-3)
  • (modified) clang/unittests/Analysis/FlowSensitive/ValueTest.cpp (+2-2)
diff --git a/clang/lib/Analysis/FlowSensitive/Value.cpp b/clang/lib/Analysis/FlowSensitive/Value.cpp
index 349f873f1e6c4d..50475ef5553036 100644
--- a/clang/lib/Analysis/FlowSensitive/Value.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Value.cpp
@@ -27,9 +27,13 @@ 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 values are distinct and have properties, we don't consider them equal,
+  // leaving equality up to the user model.
+  return &Val1 == &Val2 ||
+         (Val1.getKind() == Val2.getKind() &&
+          // (Val1.properties().empty() && Val2.properties().empty()) &&
+          (isa<TopBoolValue>(&Val1) ||
+           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 c5d18ba74c3ed6..2060b7eb264f74 100644
--- a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
@@ -53,8 +53,8 @@ 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, DifferentKindsNotEquivalent) {

copybara-service bot pushed a commit to google/crubit that referenced this pull request Jan 10, 2024
Previously, widening would detect when the widened value would be equivalent to
the previous value, but only when the pointers differed (and hence result in a
Top value). But, the pointers can share the same location, while having
different properties. This CL handles this case as well.

This change is a no-op until llvm/llvm-project#76746 is submitted. That PR will fix a bug that prevented this case from reaching the widening code.

This CL does not include any tests. The change in question affects only performance, not precision or correctness. Benchmark results indicate significant improvements over the baseline of PR 76746.

No detectable performance change (since the PR is not submitted yet):

name                              old cpu/op   new cpu/op   delta
BM_PointerAnalysisCopyPointer     77.6µs ± 1%  77.2µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisIntLoop          210µs ± 3%   209µs ± 1%   ~     (p=1.000 n=3+3)
BM_PointerAnalysisPointerLoop      462µs ± 2%   457µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisBranch           249µs ± 3%   246µs ± 1%   ~     (p=0.400 n=3+3)
BM_PointerAnalysisLoopAndBranch    774µs ± 2%   766µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisTwoLoops         378µs ± 2%   371µs ± 1%   ~     (p=0.200 n=3+3)
BM_PointerAnalysisJoinFilePath    36.6ms ± 1%  36.4ms ± 0%   ~     (p=0.400 n=3+3)
BM_PointerAnalysisCallInLoop      9.08ms ± 2%  9.03ms ± 0%   ~     (p=1.000 n=3+3)

name                              old time/op  new time/op  delta
BM_PointerAnalysisCopyPointer     77.6µs ± 1%  77.3µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisIntLoop          210µs ± 3%   209µs ± 1%   ~     (p=1.000 n=3+3)
BM_PointerAnalysisPointerLoop      462µs ± 2%   457µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisBranch           249µs ± 3%   246µs ± 1%   ~     (p=0.400 n=3+3)
BM_PointerAnalysisLoopAndBranch    774µs ± 2%   766µs ± 1%   ~     (p=0.700 n=3+3)
BM_PointerAnalysisTwoLoops         378µs ± 2%   371µs ± 1%   ~     (p=0.400 n=3+3)
BM_PointerAnalysisJoinFilePath    36.6ms ± 1%  36.4ms ± 0%   ~     (p=0.400 n=3+3)
BM_PointerAnalysisCallInLoop      9.08ms ± 2%  9.04ms ± 0%   ~     (p=1.000 n=3+3)

PiperOrigin-RevId: 597253695
Change-Id: Ieddaba0646296e8d0d6113bf963f138a66970bcd
Copy link

github-actions bot commented Jan 10, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@ymand
Copy link
Collaborator Author

ymand commented Jan 10, 2024

Martin, I've completed your requested changes. PTAL.

Copy link
Contributor

@martinboehme martinboehme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

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 llvm#76459.
@ymand ymand merged commit 65ecbdf into llvm:main Jan 16, 2024
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
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 llvm#76459.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants