-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Conversation
@llvm/pr-subscribers-clang-analysis Author: Yitzhak Mandelbaum (ymand) ChangesMakes value equivalence require that the values have no properties, except in Fixes issue #76459. Full diff: https://github.com/llvm/llvm-project/pull/76746.diff 2 Files Affected:
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) {
|
@llvm/pr-subscribers-clang Author: Yitzhak Mandelbaum (ymand) ChangesMakes value equivalence require that the values have no properties, except in Fixes issue #76459. Full diff: https://github.com/llvm/llvm-project/pull/76746.diff 2 Files Affected:
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) {
|
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
✅ With the latest revision this PR passed the C/C++ code formatter. |
Martin, I've completed your requested changes. PTAL. |
There was a problem hiding this 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.
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.
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.