[clang] 48bc715 - [clang][dataflow] Eliminate `SkipPast::ReferenceThenPointer`.
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Sun May 14 21:33:39 PDT 2023
Author: Martin Braenne
Date: 2023-05-15T04:33:29Z
New Revision: 48bc71505e03694caac6afb2431ff1157a2382a8
URL: https://github.com/llvm/llvm-project/commit/48bc71505e03694caac6afb2431ff1157a2382a8
DIFF: https://github.com/llvm/llvm-project/commit/48bc71505e03694caac6afb2431ff1157a2382a8.diff
LOG: [clang][dataflow] Eliminate `SkipPast::ReferenceThenPointer`.
As a replacement, we provide the accessors `getImplicitObjectLocation()` and
`getBaseObjectLocation()`, which are higher-level constructs that cover the use
cases in which `SkipPast::ReferenceThenPointer` was typically used.
Unfortunately, it isn't possible to use these accessors in
UncheckedOptionalAccessModel.cpp; I've added a FIXME to the code explaining the
details. I initially attempted to resolve the issue as part of this patch, but
it turned out to be non-trivial to fix. Instead, I have therefore added a
lower-level replacement for `SkipPast::ReferenceThenPointer` that is used only
within this file.
The wider context of this change is that `SkipPast` will be going away entirely.
See also the RFC at https://discourse.llvm.org/t/70086.
Reviewed By: ymandel, gribozavr2
Differential Revision: https://reviews.llvm.org/D149838
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index c23e0db7f82d3..d734ae5c66fe8 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -46,9 +46,6 @@ enum class SkipPast {
None,
/// An optional reference should be skipped past.
Reference,
- /// An optional reference should be skipped past, then an optional pointer
- /// should be skipped past.
- ReferenceThenPointer,
};
/// Indicates the result of a tentative comparison.
@@ -477,6 +474,19 @@ class Environment {
AtomicBoolValue *FlowConditionToken;
};
+/// Returns the storage location for the implicit object of a
+/// `CXXMemberCallExpr`, or null if none is defined in the environment.
+/// Dereferences the pointer if the member call expression was written using
+/// `->`.
+AggregateStorageLocation *
+getImplicitObjectLocation(const CXXMemberCallExpr &MCE, const Environment &Env);
+
+/// Returns the storage location for the base object of a `MemberExpr`, or null
+/// if none is defined in the environment. Dereferences the pointer if the
+/// member expression was written using `->`.
+AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
+ const Environment &Env);
+
} // namespace dataflow
} // namespace clang
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0d269f503f4eb..7b1944f273cf0 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -782,11 +782,6 @@ StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const {
if (auto *Val = dyn_cast_or_null<ReferenceValue>(getValue(Loc)))
return Val->getReferentLoc();
return Loc;
- case SkipPast::ReferenceThenPointer:
- StorageLocation &LocPastRef = skip(Loc, SkipPast::Reference);
- if (auto *Val = dyn_cast_or_null<PointerValue>(getValue(LocPastRef)))
- return Val->getPointeeLoc();
- return LocPastRef;
}
llvm_unreachable("bad SkipPast kind");
}
@@ -828,5 +823,39 @@ void Environment::dump() const {
dump(llvm::dbgs());
}
+AggregateStorageLocation *
+getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
+ const Environment &Env) {
+ Expr *ImplicitObject = MCE.getImplicitObjectArgument();
+ if (ImplicitObject == nullptr)
+ return nullptr;
+ StorageLocation *Loc =
+ Env.getStorageLocation(*ImplicitObject, SkipPast::Reference);
+ if (Loc == nullptr)
+ return nullptr;
+ if (ImplicitObject->getType()->isPointerType()) {
+ if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Loc)))
+ return &cast<AggregateStorageLocation>(Val->getPointeeLoc());
+ return nullptr;
+ }
+ return cast<AggregateStorageLocation>(Loc);
+}
+
+AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
+ const Environment &Env) {
+ Expr *Base = ME.getBase();
+ if (Base == nullptr)
+ return nullptr;
+ StorageLocation *Loc = Env.getStorageLocation(*Base, SkipPast::Reference);
+ if (Loc == nullptr)
+ return nullptr;
+ if (ME.isArrow()) {
+ if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Loc)))
+ return &cast<AggregateStorageLocation>(Val->getPointeeLoc());
+ return nullptr;
+ }
+ return cast<AggregateStorageLocation>(Loc);
+}
+
} // namespace dataflow
} // namespace clang
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e306b55753752..b46a57f47f982 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -372,10 +372,26 @@ bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) {
return HasValueVal != nullptr && Env.flowConditionImplies(*HasValueVal);
}
+StorageLocation *maybeSkipPointer(StorageLocation *Loc,
+ const Environment &Env) {
+ if (Loc == nullptr)
+ return nullptr;
+ if (auto *Val = dyn_cast_or_null<PointerValue>(Env.getValue(*Loc)))
+ return &Val->getPointeeLoc();
+ return Loc;
+}
+
+Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) {
+ Value *Val = Env.getValue(E, SkipPast::Reference);
+ if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
+ return Env.getValue(PointerVal->getPointeeLoc());
+ return Val;
+}
+
void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
LatticeTransferState &State) {
if (auto *OptionalVal =
- State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) {
+ getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr)
if (auto *Loc = maybeInitializeOptionalValueMember(
UnwrapExpr->getType(), *OptionalVal, State.Env))
@@ -396,8 +412,8 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (auto *HasValueVal = getHasValue(
- State.Env, State.Env.getValue(*CallExpr->getImplicitObjectArgument(),
- SkipPast::ReferenceThenPointer))) {
+ State.Env, getValueBehindPossiblePointer(
+ *CallExpr->getImplicitObjectArgument(), State.Env))) {
auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr);
State.Env.setValue(CallExprLoc, *HasValueVal);
State.Env.setStorageLocation(*CallExpr, CallExprLoc);
@@ -419,8 +435,7 @@ void transferValueOrImpl(const clang::Expr *ValueOrPredExpr,
->getImplicitObjectArgument();
auto *HasValueVal = getHasValue(
- State.Env,
- State.Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer));
+ State.Env, getValueBehindPossiblePointer(*ObjectArgumentExpr, State.Env));
if (HasValueVal == nullptr)
return;
@@ -472,8 +487,8 @@ void transferCallReturningOptional(const CallExpr *E,
void assignOptionalValue(const Expr &E, Environment &Env,
BoolValue &HasValueVal) {
- if (auto *OptionalLoc =
- Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) {
+ if (auto *OptionalLoc = maybeSkipPointer(
+ Env.getStorageLocation(E, SkipPast::Reference), Env)) {
Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal));
}
}
@@ -550,13 +565,11 @@ void transferNulloptAssignment(const CXXOperatorCallExpr *E,
transferAssignment(E, State.Env.getBoolLiteralValue(false), State);
}
-void transferSwap(const Expr &E1, SkipPast E1Skip, const Expr &E2,
+void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2,
Environment &Env) {
// We account for cases where one or both of the optionals are not modeled,
// either lacking associated storage locations, or lacking values associated
// to such storage locations.
- auto *Loc1 = Env.getStorageLocation(E1, E1Skip);
- auto *Loc2 = Env.getStorageLocation(E2, SkipPast::Reference);
if (Loc1 == nullptr) {
if (Loc2 != nullptr)
@@ -590,14 +603,20 @@ void transferSwapCall(const CXXMemberCallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 1);
- transferSwap(*E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer,
- *E->getArg(0), State.Env);
+ transferSwap(maybeSkipPointer(
+ State.Env.getStorageLocation(*E->getImplicitObjectArgument(),
+ SkipPast::Reference),
+ State.Env),
+ State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
+ State.Env);
}
void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 2);
- transferSwap(*E->getArg(0), SkipPast::Reference, *E->getArg(1), State.Env);
+ transferSwap(State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
+ State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference),
+ State.Env);
}
void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &,
@@ -756,6 +775,17 @@ auto buildTransferMatchSwitch() {
})
// optional::operator*, optional::operator->
+ // FIXME: This does something slightly strange for `operator->`.
+ // `transferUnwrapCall()` may create a new value of type `T` for the
+ // `optional<T>`, and it associates that value with `E`. In the case of
+ // `operator->`, `E` is a pointer. As a result, we associate an
+ // expression of pointer type with a storage location of non-pointer type
+ // `T`. This can confound other code that expects expressions of
+ // pointer type to be associated with `PointerValue`s, such as the
+ // centrally provided accessors `getImplicitObjectLocation()` and
+ // `getBaseObjectLocation()`, and this is the reason we need to use our
+ // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead
+ // of these accessors.
.CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
[](const CallExpr *E,
const MatchFinder::MatchResult &,
@@ -837,8 +867,7 @@ auto buildTransferMatchSwitch() {
std::vector<SourceLocation> diagnoseUnwrapCall(const Expr *UnwrapExpr,
const Expr *ObjectExpr,
const Environment &Env) {
- if (auto *OptionalVal =
- Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) {
+ if (auto *OptionalVal = getValueBehindPossiblePointer(*ObjectExpr, Env)) {
auto *Prop = OptionalVal->getProperty("has_value");
if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
if (Env.flowConditionImplies(*HasValueVal))
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 50eb7e9011bda..142dd2f21a5c6 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -542,10 +542,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}
}
- // The receiver can be either a value or a pointer to a value. Skip past the
- // indirection to handle both cases.
- auto *BaseLoc = cast_or_null<AggregateStorageLocation>(
- Env.getStorageLocation(*S->getBase(), SkipPast::ReferenceThenPointer));
+ AggregateStorageLocation *BaseLoc = getBaseObjectLocation(*S, Env);
if (BaseLoc == nullptr)
return;
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index a8b0c6bd6a087..83b9f33493d0b 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -372,8 +372,7 @@ class SpecialBoolAnalysis final
auto *Object = E->getImplicitObjectArgument();
assert(Object != nullptr);
- auto *ObjectLoc =
- Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer);
+ auto *ObjectLoc = getImplicitObjectLocation(*E, Env);
assert(ObjectLoc != nullptr);
auto &ConstructorVal = *Env.createValue(Object->getType());
@@ -532,8 +531,7 @@ class OptionalIntAnalysis final
auto *Object = E->getArg(0);
assert(Object != nullptr);
- auto *ObjectLoc =
- Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer);
+ auto *ObjectLoc = Env.getStorageLocation(*Object, SkipPast::Reference);
assert(ObjectLoc != nullptr);
auto &ConstructorVal = *Env.createValue(Object->getType());
More information about the cfe-commits
mailing list