Skip to content

Commit f3fbd21

Browse files
authored
[clang][dataflow] Strengthen pointer comparison. (#75170)
- Instead of comparing the identity of the `PointerValue`s, compare the underlying `StorageLocation`s. - If the `StorageLocation`s are the same, return a definite "true" as the result of the comparison. Before, if the `PointerValue`s were different, we would return an atom, even if the storage locations themselves were the same. - If the `StorageLocation`s are different, return an atom (as before). Pointers that have different storage locations may still alias, so we can't return a definite "false" in this case. The application-level gains from this are relatively modest. For the Crubit nullability check running on an internal codebase, this change reduces the number of functions on which the SAT solver times out from 223 to 221; the number of "pointer expression not modeled" errors reduces from 3815 to 3778. Still, it seems that the gain in precision is generally worthwhile. @Xazax-hun inspired me to think about this with his [comments](#73860 (review)) on a different PR.
1 parent d9f2b93 commit f3fbd21

File tree

2 files changed

+96
-0
lines changed

2 files changed

+96
-0
lines changed

clang/lib/Analysis/FlowSensitive/Transfer.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,14 @@ static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
6868
if (auto *RHSBool = dyn_cast_or_null<BoolValue>(RHSValue))
6969
return Env.makeIff(*LHSBool, *RHSBool);
7070

71+
if (auto *LHSPtr = dyn_cast_or_null<PointerValue>(LHSValue))
72+
if (auto *RHSPtr = dyn_cast_or_null<PointerValue>(RHSValue))
73+
// If the storage locations are the same, the pointers definitely compare
74+
// the same. If the storage locations are different, they may still alias,
75+
// so we fall through to the case below that returns an atom.
76+
if (&LHSPtr->getPointeeLoc() == &RHSPtr->getPointeeLoc())
77+
return Env.getBoolLiteralValue(true);
78+
7179
return Env.makeAtomicBoolValue();
7280
}
7381

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4709,6 +4709,94 @@ TEST(TransferTest, BooleanInequality) {
47094709
});
47104710
}
47114711

4712+
TEST(TransferTest, PointerEquality) {
4713+
std::string Code = R"cc(
4714+
void target() {
4715+
int i = 0;
4716+
int i_other = 0;
4717+
int *p1 = &i;
4718+
int *p2 = &i;
4719+
int *p_other = &i_other;
4720+
int *null = nullptr;
4721+
4722+
bool p1_eq_p1 = (p1 == p1);
4723+
bool p1_eq_p2 = (p1 == p2);
4724+
bool p1_eq_p_other = (p1 == p_other);
4725+
4726+
bool p1_eq_null = (p1 == null);
4727+
bool p1_eq_nullptr = (p1 == nullptr);
4728+
bool null_eq_nullptr = (null == nullptr);
4729+
bool nullptr_eq_nullptr = (nullptr == nullptr);
4730+
4731+
// We won't duplicate all of the tests above with `!=`, as we know that
4732+
// the implementation simply negates the result of the `==` comparison.
4733+
// Instaed, just spot-check one case.
4734+
bool p1_ne_p1 = (p1 != p1);
4735+
4736+
(void)0; // [[p]]
4737+
}
4738+
)cc";
4739+
runDataflow(
4740+
Code,
4741+
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
4742+
ASTContext &ASTCtx) {
4743+
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
4744+
4745+
// Check the we have indeed set things up so that `p1` and `p2` have
4746+
// different pointer values.
4747+
EXPECT_NE(&getValueForDecl<PointerValue>(ASTCtx, Env, "p1"),
4748+
&getValueForDecl<PointerValue>(ASTCtx, Env, "p2"));
4749+
4750+
EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p1"),
4751+
&Env.getBoolLiteralValue(true));
4752+
EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p2"),
4753+
&Env.getBoolLiteralValue(true));
4754+
EXPECT_TRUE(isa<AtomicBoolValue>(
4755+
getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_p_other")));
4756+
4757+
EXPECT_TRUE(isa<AtomicBoolValue>(
4758+
getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_null")));
4759+
EXPECT_TRUE(isa<AtomicBoolValue>(
4760+
getValueForDecl<BoolValue>(ASTCtx, Env, "p1_eq_nullptr")));
4761+
EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "null_eq_nullptr"),
4762+
&Env.getBoolLiteralValue(true));
4763+
EXPECT_EQ(
4764+
&getValueForDecl<BoolValue>(ASTCtx, Env, "nullptr_eq_nullptr"),
4765+
&Env.getBoolLiteralValue(true));
4766+
4767+
EXPECT_EQ(&getValueForDecl<BoolValue>(ASTCtx, Env, "p1_ne_p1"),
4768+
&Env.getBoolLiteralValue(false));
4769+
});
4770+
}
4771+
4772+
TEST(TransferTest, PointerEqualityUnionMembers) {
4773+
std::string Code = R"cc(
4774+
union U {
4775+
int i1;
4776+
int i2;
4777+
};
4778+
void target() {
4779+
U u;
4780+
bool i1_eq_i2 = (&u.i1 == &u.i2);
4781+
4782+
(void)0; // [[p]]
4783+
}
4784+
)cc";
4785+
runDataflow(
4786+
Code,
4787+
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
4788+
ASTContext &ASTCtx) {
4789+
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
4790+
4791+
// FIXME: By the standard, `u.i1` and `u.i2` should have the same
4792+
// address, but we don't yet model this property of union members
4793+
// correctly. The result is therefore weaker than it could be (just an
4794+
// atom rather than a true literal), though not wrong.
4795+
EXPECT_TRUE(isa<AtomicBoolValue>(
4796+
getValueForDecl<BoolValue>(ASTCtx, Env, "i1_eq_i2")));
4797+
});
4798+
}
4799+
47124800
TEST(TransferTest, IntegerLiteralEquality) {
47134801
std::string Code = R"(
47144802
void target() {

0 commit comments

Comments
 (0)