[clang][dataflow] Fix bug in Value comparison.
#76746
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@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) {
|
Xazax-hun
reviewed
Jan 2, 2024
Xazax-hun
approved these changes
Jan 3, 2024
martinboehme
requested changes
Jan 8, 2024
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Martin, I've completed your requested changes. PTAL. |
martinboehme
approved these changes
Jan 12, 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.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.