[clang] [clang][dataflow] Strengthen pointer comparison. (PR #75170)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 12 04:22:45 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: None (martinboehme)
<details>
<summary>Changes</summary>
- Instead of comparing the identity of the `PointerValue`s, compare the
underlying `StorageLocation`s.
- If the `StorageLocation`s are different, return a definite "false" as the
result of the comparison. Before, if the `PointerValue`s were different, the
best we could do was to return an atom (because the `StorageLocation`s might
still be the same).
On an internal codebase, this change reduces SAT solver timeouts by over 20% and
"maximum iterations reached" errors by over 50%. In addition, it obviously
improves the precision of the analysis.
@<!-- -->Xazax-hun inspired me to think about this with his
[comments](https://github.com/llvm/llvm-project/pull/73860#pullrequestreview-1761484615)
on a different PR.
The one thing where the new code currently does the wrong thing is when
comparing the addresses of different members of a union. By the language
standard, all members of a union should have the same address, but we currently
model them with different `StorageLocation`s, and so with this change, we will
return false when comparing the addreses.
I propose that this is acceptable because is unlikely to affect the behavior of
real-world code in meaningful ways.
With this change, the test TransferTest.DifferentReferenceLocInJoin started to
fail because the code under test no longer set up the desired state where a
variable of reference type is mapped to two different storage locations in
environments being joined. Rather than trying to modify this test to set up the
test condition again, I have chosen to replace the test with an equivalent
test in DataflowEnvironmentTest.cpp that sets up the test condition directly;
because this test is more direct, it will also be less brittle in the face of
future changes.
---
Full diff: https://github.com/llvm/llvm-project/pull/75170.diff
3 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+5)
- (modified) clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp (+42)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+60-44)
``````````diff
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index bbf5f12359bc70..55db15e03b39cb 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -60,6 +60,11 @@ static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
if (auto *RHSBool = dyn_cast_or_null<BoolValue>(RHSValue))
return Env.makeIff(*LHSBool, *RHSBool);
+ if (auto *LHSPtr = dyn_cast_or_null<PointerValue>(LHSValue))
+ if (auto *RHSPtr = dyn_cast_or_null<PointerValue>(RHSValue))
+ return Env.getBoolLiteralValue(&LHSPtr->getPointeeLoc() ==
+ &RHSPtr->getPointeeLoc());
+
return Env.makeAtomicBoolValue();
}
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 003434a58b1075..de18f7de02c12d 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -24,6 +24,7 @@ namespace {
using namespace clang;
using namespace dataflow;
+using ::clang::dataflow::test::findValueDecl;
using ::clang::dataflow::test::getFieldValue;
using ::testing::Contains;
using ::testing::IsNull;
@@ -152,6 +153,47 @@ TEST_F(EnvironmentTest, JoinRecords) {
}
}
+TEST_F(EnvironmentTest, DifferentReferenceLocInJoin) {
+ // This tests the case where the storage location for a reference-type
+ // variable is different for two states being joined. We used to believe this
+ // could not happen and therefore had an assertion disallowing this; this test
+ // exists to demonstrate that we can handle this condition without a failing
+ // assertion. See also the discussion here:
+ // https://discourse.llvm.org/t/70086/6
+
+ using namespace ast_matchers;
+
+ std::string Code = R"cc(
+ void f(int &ref) {}
+ )cc";
+
+ auto Unit =
+ tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+ auto &Context = Unit->getASTContext();
+
+ ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+ const ValueDecl *Ref = findValueDecl(Context, "ref");
+
+ Environment Env1(DAContext);
+ StorageLocation &Loc1 = Env1.createStorageLocation(Context.IntTy);
+ Env1.setStorageLocation(*Ref, Loc1);
+
+ Environment Env2(DAContext);
+ StorageLocation &Loc2 = Env2.createStorageLocation(Context.IntTy);
+ Env2.setStorageLocation(*Ref, Loc2);
+
+ EXPECT_NE(&Loc1, &Loc2);
+
+ Environment::ValueModel Model;
+ Environment EnvJoined = Environment::join(Env1, Env2, Model);
+
+ // Joining environments with different storage locations for the same
+ // declaration results in the declaration being removed from the joined
+ // environment.
+ EXPECT_EQ(EnvJoined.getStorageLocation(*Ref), nullptr);
+}
+
TEST_F(EnvironmentTest, InitGlobalVarsFun) {
using namespace ast_matchers;
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 8da55953a32986..a4b47f25f36908 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3893,6 +3893,66 @@ TEST(TransferTest, BooleanInequality) {
});
}
+TEST(TransferTest, PointerEquality) {
+ std::string Code = R"(
+ void target() {
+ int i = 0;
+ int i_other = 0;
+ int *p1 = &i;
+ int *p2 = &i;
+ int *p_other = &i_other;
+ int *null = nullptr;
+
+ bool p1_eq_p1 = (p1 == p1);
+ bool p1_eq_p2 = (p1 == p2);
+ bool p1_eq_p_other = (p1 == p_other);
+
+ bool p1_eq_null = (p1 == null);
+ bool p1_eq_nullptr = (p1 == nullptr);
+ bool null_eq_nullptr = (null == nullptr);
+ bool nullptr_eq_nullptr = (nullptr == nullptr);
+
+ // We won't duplicate all of the tests above with `!=`, as we know that
+ // the implementation simply negates the result of the `==` comparison.
+ // Instaed, just spot-check one case.
+ bool p1_ne_p_other = (p1 != p_other);
+
+ (void)0; // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ // Check the we have indeed set things up so that `p1` and `p2` have
+ // different pointer values.
+ EXPECT_NE(&getValueForDecl<PointerValue>(ASTCtx, Env, "p1"),
+ &getValueForDecl<PointerValue>(ASTCtx, Env, "p2"));
+
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p1"),
+ &Env.getBoolLiteralValue(true));
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p2"),
+ &Env.getBoolLiteralValue(true));
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p_other"),
+ &Env.getBoolLiteralValue(false));
+
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_null"),
+ &Env.getBoolLiteralValue(false));
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_nullptr"),
+ &Env.getBoolLiteralValue(false));
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "null_eq_nullptr"),
+ &Env.getBoolLiteralValue(true));
+ EXPECT_EQ(
+ &getValueForDecl<BoolValue>(ASTCtx, Env, "nullptr_eq_nullptr"),
+ &Env.getBoolLiteralValue(true));
+
+ EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_ne_p_other"),
+ &Env.getBoolLiteralValue(true));
+ });
+}
+
TEST(TransferTest, IntegerLiteralEquality) {
std::string Code = R"(
void target() {
@@ -6315,48 +6375,4 @@ TEST(TransferTest, LambdaCaptureThis) {
});
}
-TEST(TransferTest, DifferentReferenceLocInJoin) {
- // This test triggers a case where the storage location for a reference-type
- // variable is different for two states being joined. We used to believe this
- // could not happen and therefore had an assertion disallowing this; this test
- // exists to demonstrate that we can handle this condition without a failing
- // assertion. See also the discussion here:
- // https://discourse.llvm.org/t/70086/6
- std::string Code = R"(
- namespace std {
- template <class T> struct initializer_list {
- const T* begin();
- const T* end();
- };
- }
-
- void target(char* p, char* end) {
- while (p != end) {
- if (*p == ' ') {
- p++;
- continue;
- }
-
- auto && range = {1, 2};
- for (auto b = range.begin(), e = range.end(); b != e; ++b) {
- }
- (void)0;
- // [[p]]
- }
- }
- )";
- runDataflow(
- Code,
- [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
- ASTContext &ASTCtx) {
- const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
- // Joining environments with different storage locations for the same
- // declaration results in the declaration being removed from the joined
- // environment.
- const ValueDecl *VD = findValueDecl(ASTCtx, "range");
- ASSERT_EQ(Env.getStorageLocation(*VD), nullptr);
- });
-}
-
} // namespace
``````````
</details>
https://github.com/llvm/llvm-project/pull/75170
More information about the cfe-commits
mailing list