[clang] [clang][dataflow] Add `Environment::get<>()`. (PR #76027)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 20 23:34:24 PST 2023
https://github.com/martinboehme updated https://github.com/llvm/llvm-project/pull/76027
>From 9fd1ed96c1b7934754b6fefd7b01a62d4a391d73 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 20 Dec 2023 10:01:10 +0000
Subject: [PATCH 1/3] [clang][dataflow] Add `Environment::get<>()`.
This template function casts the result of `getValue()` or
`getStorageLocation()` to a given subclass of `Value` or `StorageLocation`
(using `cast_or_null`).
It's a common pattern to do something like this:
```cxx
auto *Val = cast_or_null<PointerValue>(Env.getValue(E));
```
This can now be expressed more concisely like this:
```cxx
auto *Val = Env.get<PointerValue>(E);
```
Instead of adding a new method `get()`, I had originally considered simply
adding a template parameter to `getValue()` and `getStorageLocation()` (with a
default argument of `Value` or `StorageLocation`), but this results in an
undesirable repetition at the callsite, e.g.
`getStorageLocation<RecordStorageLocation>(...)`. The `Value` and
`StorageLocation` in the method name adds nothing of value when the template
argument already contains this information, so it seemed best to shorten the
method name to simply `get()`.
---
.../FlowSensitive/DataflowEnvironment.h | 36 +++++++++++++++++++
.../FlowSensitive/DataflowEnvironment.cpp | 14 ++++----
.../Models/UncheckedOptionalAccessModel.cpp | 31 +++++++---------
.../lib/Analysis/FlowSensitive/RecordOps.cpp | 8 ++---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 16 ++++-----
.../TypeErasedDataflowAnalysis.cpp | 2 +-
.../FlowSensitive/SignAnalysisTest.cpp | 2 +-
7 files changed, 66 insertions(+), 43 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 5943af50b6ad8f..2a9f8dce74c0a1 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -289,6 +289,22 @@ class Environment {
/// `E` must be a glvalue or a `BuiltinType::BuiltinFn`
StorageLocation *getStorageLocation(const Expr &E) const;
+ /// Returns the result of casting `getStorageLocation(...)` to a subclass of
+ /// `StorageLocation` (using `cast_or_null<T>`).
+ /// This assert-fails if the result of `getStorageLocation(...)` is not of
+ /// type `T *`; if the storage location is not guaranteed to have type `T *`,
+ /// consider using `dyn_cast_or_null<T>(getStorageLocation(...))` instead.
+ template <typename T>
+ std::enable_if_t<std::is_base_of_v<StorageLocation, T>, T *>
+ get(const ValueDecl &D) const {
+ return cast_or_null<T>(getStorageLocation(D));
+ }
+ template <typename T>
+ std::enable_if_t<std::is_base_of_v<StorageLocation, T>, T *>
+ get(const Expr &E) const {
+ return cast_or_null<T>(getStorageLocation(E));
+ }
+
/// Returns the storage location assigned to the `this` pointee in the
/// environment or null if the `this` pointee has no assigned storage location
/// in the environment.
@@ -457,6 +473,26 @@ class Environment {
/// storage location in the environment, otherwise returns null.
Value *getValue(const Expr &E) const;
+ /// Returns the result of casting `getValue(...)` to a subclass of `Value`
+ /// (using `cast_or_null<T>`).
+ /// This assert-fails if the result of `getValue(...)` is not of type `T *`;
+ /// if the value is not guaranteed to have type `T *`, consider using
+ /// `dyn_cast_or_null<T>(getValue(...))` instead.
+ template <typename T>
+ std::enable_if_t<std::is_base_of_v<Value, T>, T *>
+ get(const StorageLocation &Loc) const {
+ return cast_or_null<T>(getValue(Loc));
+ }
+ template <typename T>
+ std::enable_if_t<std::is_base_of_v<Value, T>, T *>
+ get(const ValueDecl &D) const {
+ return cast_or_null<T>(getValue(D));
+ }
+ template <typename T>
+ std::enable_if_t<std::is_base_of_v<Value, T>, T *> get(const Expr &E) const {
+ return cast_or_null<T>(getValue(E));
+ }
+
// FIXME: should we deprecate the following & call arena().create() directly?
/// Creates a `T` (some subclass of `Value`), forwarding `args` to the
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 93919cd0243d0c..46841bac35ccb3 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -489,8 +489,7 @@ Environment Environment::pushCall(const CallExpr *Call) const {
if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
if (!isa<CXXThisExpr>(Arg))
- Env.ThisPointeeLoc =
- cast<RecordStorageLocation>(getStorageLocation(*Arg));
+ Env.ThisPointeeLoc = get<RecordStorageLocation>(*Arg);
// Otherwise (when the argument is `this`), retain the current
// environment's `ThisPointeeLoc`.
}
@@ -1034,7 +1033,7 @@ RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
if (ImplicitObject == nullptr)
return nullptr;
if (ImplicitObject->getType()->isPointerType()) {
- if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*ImplicitObject)))
+ if (auto *Val = Env.get<PointerValue>(*ImplicitObject))
return &cast<RecordStorageLocation>(Val->getPointeeLoc());
return nullptr;
}
@@ -1048,11 +1047,11 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
if (Base == nullptr)
return nullptr;
if (ME.isArrow()) {
- if (auto *Val = cast_or_null<PointerValue>(Env.getValue(*Base)))
+ if (auto *Val = Env.get<PointerValue>(*Base))
return &cast<RecordStorageLocation>(Val->getPointeeLoc());
return nullptr;
}
- return cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Base));
+ return Env.get<RecordStorageLocation>(*Base);
}
std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
@@ -1077,7 +1076,7 @@ RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
assert(Expr.getType()->isRecordType());
if (Expr.isPRValue()) {
- if (auto *ExistingVal = cast_or_null<RecordValue>(Env.getValue(Expr))) {
+ if (auto *ExistingVal = Env.get<RecordValue>(Expr)) {
auto &NewVal = Env.create<RecordValue>(ExistingVal->getLoc());
Env.setValue(Expr, NewVal);
Env.setValue(NewVal.getLoc(), NewVal);
@@ -1089,8 +1088,7 @@ RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
return NewVal;
}
- if (auto *Loc =
- cast_or_null<RecordStorageLocation>(Env.getStorageLocation(Expr))) {
+ if (auto *Loc = Env.get<RecordStorageLocation>(Expr)) {
auto &NewVal = Env.create<RecordValue>(*Loc);
Env.setValue(*Loc, NewVal);
return NewVal;
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 69ac2c2b82cff2..1d31b22b6d25ff 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -226,7 +226,7 @@ auto isComparisonOperatorCall(L lhs_arg_matcher, R rhs_arg_matcher) {
/// Ensures that `Expr` is mapped to a `BoolValue` and returns its formula.
const Formula &forceBoolValue(Environment &Env, const Expr &Expr) {
- auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr));
+ auto *Value = Env.get<BoolValue>(Expr);
if (Value != nullptr)
return Value->formula();
@@ -267,7 +267,7 @@ BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) {
if (OptionalLoc == nullptr)
return nullptr;
StorageLocation &HasValueLoc = locForHasValue(*OptionalLoc);
- auto *HasValueVal = cast_or_null<BoolValue>(Env.getValue(HasValueLoc));
+ auto *HasValueVal = Env.get<BoolValue>(HasValueLoc);
if (HasValueVal == nullptr) {
HasValueVal = &Env.makeAtomicBoolValue();
Env.setValue(HasValueLoc, *HasValueVal);
@@ -406,7 +406,7 @@ void transferCallReturningOptional(const CallExpr *E,
if (E->isPRValue()) {
Loc = &State.Env.getResultObjectLocation(*E);
} else {
- Loc = cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(*E));
+ Loc = State.Env.get<RecordStorageLocation>(*E);
if (Loc == nullptr) {
Loc = &cast<RecordStorageLocation>(State.Env.createStorageLocation(*E));
State.Env.setStorageLocation(*E, *Loc);
@@ -449,8 +449,7 @@ BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
// This is a constructor/assignment call for `optional<T>` with argument of
// type `optional<U>` such that `T` is constructible from `U`.
- auto *Loc =
- cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(E));
+ auto *Loc = State.Env.get<RecordStorageLocation>(E);
if (auto *HasValueVal = getHasValue(State.Env, Loc))
return *HasValueVal;
return State.Env.makeAtomicBoolValue();
@@ -471,8 +470,7 @@ void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
LatticeTransferState &State) {
assert(E->getNumArgs() > 0);
- if (auto *Loc = cast_or_null<RecordStorageLocation>(
- State.Env.getStorageLocation(*E->getArg(0)))) {
+ if (auto *Loc = State.Env.get<RecordStorageLocation>(*E->getArg(0))) {
createOptionalValue(*Loc, HasValueVal, State.Env);
// Assign a storage location for the whole expression.
@@ -534,18 +532,15 @@ void transferSwapCall(const CXXMemberCallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 1);
- auto *OtherLoc = cast_or_null<RecordStorageLocation>(
- State.Env.getStorageLocation(*E->getArg(0)));
+ auto *OtherLoc = State.Env.get<RecordStorageLocation>(*E->getArg(0));
transferSwap(getImplicitObjectLocation(*E, State.Env), OtherLoc, State.Env);
}
void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 2);
- auto *Arg0Loc = cast_or_null<RecordStorageLocation>(
- State.Env.getStorageLocation(*E->getArg(0)));
- auto *Arg1Loc = cast_or_null<RecordStorageLocation>(
- State.Env.getStorageLocation(*E->getArg(1)));
+ auto *Arg0Loc = State.Env.get<RecordStorageLocation>(*E->getArg(0));
+ auto *Arg1Loc = State.Env.get<RecordStorageLocation>(*E->getArg(1));
transferSwap(Arg0Loc, Arg1Loc, State.Env);
}
@@ -585,11 +580,9 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr,
Environment &Env = State.Env;
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- auto *Arg0Loc = cast_or_null<RecordStorageLocation>(
- Env.getStorageLocation(*CmpExpr->getArg(0)));
+ auto *Arg0Loc = Env.get<RecordStorageLocation>(*CmpExpr->getArg(0));
if (auto *LHasVal = getHasValue(Env, Arg0Loc)) {
- auto *Arg1Loc = cast_or_null<RecordStorageLocation>(
- Env.getStorageLocation(*CmpExpr->getArg(1)));
+ auto *Arg1Loc = Env.get<RecordStorageLocation>(*CmpExpr->getArg(1));
if (auto *RHasVal = getHasValue(Env, Arg1Loc)) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
@@ -603,7 +596,7 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
const clang::Expr *E, Environment &Env) {
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+ auto *Loc = Env.get<RecordStorageLocation>(*E);
if (auto *HasVal = getHasValue(Env, Loc)) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
@@ -616,7 +609,7 @@ void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr,
const clang::Expr *E, Environment &Env) {
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+ auto *Loc = Env.get<RecordStorageLocation>(*E);
if (auto *HasVal = getHasValue(Env, Loc)) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
index caaf443382b02c..a326826db23945 100644
--- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -66,8 +66,8 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
}
}
- RecordValue *SrcVal = cast_or_null<RecordValue>(Env.getValue(Src));
- RecordValue *DstVal = cast_or_null<RecordValue>(Env.getValue(Dst));
+ RecordValue *SrcVal = Env.get<RecordValue>(Src);
+ RecordValue *DstVal = Env.get<RecordValue>(Dst);
DstVal = &Env.create<RecordValue>(Dst);
Env.setValue(Dst, *DstVal);
@@ -127,10 +127,10 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
llvm::StringMap<Value *> Props1, Props2;
- if (RecordValue *Val1 = cast_or_null<RecordValue>(Env1.getValue(Loc1)))
+ if (RecordValue *Val1 = Env1.get<RecordValue>(Loc1))
for (const auto &[Name, Value] : Val1->properties())
Props1[Name] = Value;
- if (RecordValue *Val2 = cast_or_null<RecordValue>(Env2.getValue(Loc2)))
+ if (RecordValue *Val2 = Env2.get<RecordValue>(Loc2))
for (const auto &[Name, Value] : Val2->properties())
Props2[Name] = Value;
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 346469660662e0..55093c2e2cdaf0 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -339,8 +339,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
switch (S->getOpcode()) {
case UO_Deref: {
- const auto *SubExprVal =
- cast_or_null<PointerValue>(Env.getValue(*SubExpr));
+ const auto *SubExprVal = Env.get<PointerValue>(*SubExpr);
if (SubExprVal == nullptr)
break;
@@ -467,8 +466,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
const Expr *Arg = S->getArg(0);
assert(Arg != nullptr);
- auto *ArgLoc =
- cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg));
+ auto *ArgLoc = Env.get<RecordStorageLocation>(*Arg);
if (ArgLoc == nullptr)
return;
@@ -515,14 +513,12 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
RecordStorageLocation *LocSrc = nullptr;
if (Arg1->isPRValue()) {
- if (auto *Val = cast_or_null<RecordValue>(Env.getValue(*Arg1)))
+ if (auto *Val = Env.get<RecordValue>(*Arg1))
LocSrc = &Val->getLoc();
} else {
- LocSrc =
- cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg1));
+ LocSrc = Env.get<RecordStorageLocation>(*Arg1);
}
- auto *LocDst =
- cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*Arg0));
+ auto *LocDst = Env.get<RecordStorageLocation>(*Arg0);
if (LocSrc == nullptr || LocDst == nullptr)
return;
@@ -676,7 +672,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
auto Init = Inits[InitIdx++];
assert(Base.getType().getCanonicalType() ==
Init->getType().getCanonicalType());
- auto* BaseVal = cast_or_null<RecordValue>(Env.getValue(*Init));
+ auto *BaseVal = Env.get<RecordValue>(*Init);
if (!BaseVal)
BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
// Take ownership of the fields of the `RecordValue` for the base class
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 8c9360235da7c1..faf83a8920d4ea 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -130,7 +130,7 @@ class TerminatorVisitor
if (Env.getValue(Cond) == nullptr)
transfer(StmtToEnv, Cond, Env);
- auto *Val = cast_or_null<BoolValue>(Env.getValue(Cond));
+ auto *Val = Env.get<BoolValue>(Cond);
// Value merging depends on flow conditions from different environments
// being mutually exclusive -- that is, they cannot both be true in their
// entirety (even if they may share some clauses). So, we need *some* value
diff --git a/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
index 362b0dea58d6b8..b5fc7bbc431eae 100644
--- a/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
@@ -133,7 +133,7 @@ void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
LatticeTransferState &State) {
auto &A = State.Env.arena();
const Formula *Comp;
- if (BoolValue *V = cast_or_null<BoolValue>(State.Env.getValue(*BO))) {
+ if (BoolValue *V = State.Env.get<BoolValue>(*BO)) {
Comp = &V->formula();
} else {
Comp = &A.makeAtomRef(A.makeAtom());
>From e4cd009b58e9469b638ec6ec5cdb1ccc4791cd35 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Thu, 21 Dec 2023 07:31:26 +0000
Subject: [PATCH 2/3] fixup! [clang][dataflow] Add `Environment::get<>()`.
---
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 46841bac35ccb3..84f4f564ca4836 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -489,7 +489,8 @@ Environment Environment::pushCall(const CallExpr *Call) const {
if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
if (!isa<CXXThisExpr>(Arg))
- Env.ThisPointeeLoc = get<RecordStorageLocation>(*Arg);
+ Env.ThisPointeeLoc =
+ cast<RecordStorageLocation>(getStorageLocation(*Arg));
// Otherwise (when the argument is `this`), retain the current
// environment's `ThisPointeeLoc`.
}
>From ad06cc8b3380020c2cb312ce12901174bd003b81 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Thu, 21 Dec 2023 07:33:09 +0000
Subject: [PATCH 3/3] fixup! [clang][dataflow] Add `Environment::get<>()`.
---
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 84f4f564ca4836..96fe6df88dbb9f 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -489,8 +489,8 @@ Environment Environment::pushCall(const CallExpr *Call) const {
if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
if (!isa<CXXThisExpr>(Arg))
- Env.ThisPointeeLoc =
- cast<RecordStorageLocation>(getStorageLocation(*Arg));
+ Env.ThisPointeeLoc =
+ cast<RecordStorageLocation>(getStorageLocation(*Arg));
// Otherwise (when the argument is `this`), retain the current
// environment's `ThisPointeeLoc`.
}
More information about the cfe-commits
mailing list