[clang] f653d14 - [clang][dataflow] Various refactorings to UncheckedOptionalAccessModel.
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 9 23:46:05 PDT 2023
Author: Martin Braenne
Date: 2023-07-10T06:45:55Z
New Revision: f653d14065a362c98114082c4e9a3b1ede7a90f5
URL: https://github.com/llvm/llvm-project/commit/f653d14065a362c98114082c4e9a3b1ede7a90f5
DIFF: https://github.com/llvm/llvm-project/commit/f653d14065a362c98114082c4e9a3b1ede7a90f5.diff
LOG: [clang][dataflow] Various refactorings to UncheckedOptionalAccessModel.
These are intended to ease an upcoming change that will eliminate the duplication between `AggregateStorageLocation` and `StructValue` (see https://discourse.llvm.org/t/70086 for details), but many of the changes also have value in their own right.
Depends On D154586
Reviewed By: ymandel, gribozavr2
Differential Revision: https://reviews.llvm.org/D154597
Added:
Modified:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index a239d54674e96c..91aa9d11e751cd 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -255,12 +255,22 @@ void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) {
/// Creates a symbolic value for an `optional` value using `HasValueVal` as the
/// symbolic value of its "has_value" property.
-StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
+StructValue &createOptionalValue(BoolValue &HasValueVal, Environment &Env) {
auto &OptionalVal = Env.create<StructValue>();
setHasValue(OptionalVal, HasValueVal);
return OptionalVal;
}
+/// Creates a symbolic value for an `optional` value at an existing storage
+/// location. Uses `HasValueVal` as the symbolic value of the "has_value"
+/// property.
+StructValue &createOptionalValue(AggregateStorageLocation &Loc,
+ BoolValue &HasValueVal, Environment &Env) {
+ StructValue &OptionalVal = createOptionalValue(HasValueVal, Env);
+ Env.setValue(Loc, OptionalVal);
+ return OptionalVal;
+}
+
/// Returns the symbolic value that represents the "has_value" property of the
/// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
BoolValue *getHasValue(Environment &Env, Value *OptionalVal) {
@@ -422,15 +432,6 @@ 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))
@@ -467,7 +468,7 @@ void transferMakeOptionalCall(const CallExpr *E,
auto &Loc = State.Env.createStorageLocation(*E);
State.Env.setStorageLocation(*E, Loc);
State.Env.setValue(
- Loc, createOptionalValue(State.Env, State.Env.getBoolLiteralValue(true)));
+ Loc, createOptionalValue(State.Env.getBoolLiteralValue(true), State.Env));
}
void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
@@ -544,15 +545,12 @@ void transferCallReturningOptional(const CallExpr *E,
auto &Loc = State.Env.createStorageLocation(*E);
State.Env.setStorageLocation(*E, Loc);
State.Env.setValue(
- Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+ Loc, createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env));
}
-void assignOptionalValue(const Expr &E, Environment &Env,
- BoolValue &HasValueVal) {
- if (auto *OptionalLoc = maybeSkipPointer(
- Env.getStorageLocation(E, SkipPast::Reference), Env)) {
- Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal));
- }
+void constructOptionalValue(const Expr &E, Environment &Env,
+ BoolValue &HasValueVal) {
+ Env.setValueStrict(E, createOptionalValue(HasValueVal, Env));
}
/// Returns a symbolic value for the "has_value" property of an `optional<T>`
@@ -590,25 +588,23 @@ void transferValueOrConversionConstructor(
LatticeTransferState &State) {
assert(E->getNumArgs() > 0);
- assignOptionalValue(*E, State.Env,
- valueOrConversionHasValue(*E->getConstructor(),
- *E->getArg(0), MatchRes,
- State));
+ constructOptionalValue(*E, State.Env,
+ valueOrConversionHasValue(*E->getConstructor(),
+ *E->getArg(0), MatchRes,
+ State));
}
void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
LatticeTransferState &State) {
assert(E->getNumArgs() > 0);
- auto *OptionalLoc =
- State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
- if (OptionalLoc == nullptr)
- return;
-
- State.Env.setValue(*OptionalLoc, createOptionalValue(State.Env, HasValueVal));
+ if (auto *Loc = cast<AggregateStorageLocation>(
+ State.Env.getStorageLocationStrict(*E->getArg(0)))) {
+ createOptionalValue(*Loc, HasValueVal, State.Env);
- // Assign a storage location for the whole expression.
- State.Env.setStorageLocation(*E, *OptionalLoc);
+ // Assign a storage location for the whole expression.
+ State.Env.setStorageLocationStrict(*E, *Loc);
+ }
}
void transferValueOrConversionAssignment(
@@ -627,19 +623,19 @@ void transferNulloptAssignment(const CXXOperatorCallExpr *E,
transferAssignment(E, State.Env.getBoolLiteralValue(false), State);
}
-void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2,
- Environment &Env) {
+void transferSwap(AggregateStorageLocation *Loc1,
+ AggregateStorageLocation *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.
if (Loc1 == nullptr) {
if (Loc2 != nullptr)
- Env.setValue(*Loc2, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+ createOptionalValue(*Loc2, Env.makeAtomicBoolValue(), Env);
return;
}
if (Loc2 == nullptr) {
- Env.setValue(*Loc1, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+ createOptionalValue(*Loc1, Env.makeAtomicBoolValue(), Env);
return;
}
@@ -649,36 +645,35 @@ void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2,
// allows for local reasoning about the value. To avoid the above, we would
// need *lazy* value allocation.
// FIXME: allocate values lazily, instead of just creating a fresh value.
- auto *Val1 = Env.getValue(*Loc1);
- if (Val1 == nullptr)
- Val1 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
+ BoolValue *BoolVal1 = getHasValue(Env, Env.getValue(*Loc1));
+ if (BoolVal1 == nullptr)
+ BoolVal1 = &Env.makeAtomicBoolValue();
- auto *Val2 = Env.getValue(*Loc2);
- if (Val2 == nullptr)
- Val2 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
+ BoolValue *BoolVal2 = getHasValue(Env, Env.getValue(*Loc2));
+ if (BoolVal2 == nullptr)
+ BoolVal2 = &Env.makeAtomicBoolValue();
- Env.setValue(*Loc1, *Val2);
- Env.setValue(*Loc2, *Val1);
+ createOptionalValue(*Loc1, *BoolVal2, Env);
+ createOptionalValue(*Loc2, *BoolVal1, Env);
}
void transferSwapCall(const CXXMemberCallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 1);
- transferSwap(maybeSkipPointer(
- State.Env.getStorageLocation(*E->getImplicitObjectArgument(),
- SkipPast::Reference),
- State.Env),
- State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
- State.Env);
+ auto *OtherLoc = cast_or_null<AggregateStorageLocation>(
+ State.Env.getStorageLocationStrict(*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);
- transferSwap(State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference),
- State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference),
- State.Env);
+ auto *Arg0Loc = cast_or_null<AggregateStorageLocation>(
+ State.Env.getStorageLocationStrict(*E->getArg(0)));
+ auto *Arg1Loc = cast_or_null<AggregateStorageLocation>(
+ State.Env.getStorageLocationStrict(*E->getArg(1)));
+ transferSwap(Arg0Loc, Arg1Loc, State.Env);
}
void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &,
@@ -697,7 +692,7 @@ void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &,
Value *ValArg = State.Env.getValue(*LocArg);
if (ValArg == nullptr)
- ValArg = &createOptionalValue(State.Env, State.Env.makeAtomicBoolValue());
+ ValArg = &createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env);
// Create a new storage location
LocRet = &State.Env.createStorageLocation(*E);
@@ -798,24 +793,24 @@ auto buildTransferMatchSwitch() {
isOptionalInPlaceConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E, State.Env,
- State.Env.getBoolLiteralValue(true));
+ constructOptionalValue(*E, State.Env,
+ State.Env.getBoolLiteralValue(true));
})
// nullopt_t::nullopt_t
.CaseOfCFGStmt<CXXConstructExpr>(
isNulloptConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E, State.Env,
- State.Env.getBoolLiteralValue(false));
+ constructOptionalValue(*E, State.Env,
+ State.Env.getBoolLiteralValue(false));
})
// optional::optional(nullopt_t)
.CaseOfCFGStmt<CXXConstructExpr>(
isOptionalNulloptConstructor(),
[](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E, State.Env,
- State.Env.getBoolLiteralValue(false));
+ constructOptionalValue(*E, State.Env,
+ State.Env.getBoolLiteralValue(false));
})
// optional::optional (value/conversion)
.CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
@@ -867,8 +862,11 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithName("emplace"),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
- State.Env.getBoolLiteralValue(true));
+ if (AggregateStorageLocation *Loc =
+ getImplicitObjectLocation(*E, State.Env)) {
+ createOptionalValue(*Loc, State.Env.getBoolLiteralValue(true),
+ State.Env);
+ }
})
// optional::reset
@@ -876,8 +874,11 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithName("reset"),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- assignOptionalValue(*E->getImplicitObjectArgument(), State.Env,
- State.Env.getBoolLiteralValue(false));
+ if (AggregateStorageLocation *Loc =
+ getImplicitObjectLocation(*E, State.Env)) {
+ createOptionalValue(*Loc, State.Env.getBoolLiteralValue(false),
+ State.Env);
+ }
})
// optional::swap
@@ -1043,7 +1044,7 @@ Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev,
if (isa<TopBoolValue>(CurrentHasVal))
return &Current;
}
- return &createOptionalValue(CurrentEnv, CurrentEnv.makeTopBoolValue());
+ return &createOptionalValue(CurrentEnv.makeTopBoolValue(), CurrentEnv);
case ComparisonResult::Unknown:
return nullptr;
}
More information about the cfe-commits
mailing list