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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
