[clang] e95134b - [clang][dataflow] Reverse course on `getValue()` deprecation.
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 27 06:15:00 PDT 2023
Author: Martin Braenne
Date: 2023-07-27T13:14:49Z
New Revision: e95134b9cb1885b0da929737858163486a5c399c
URL: https://github.com/llvm/llvm-project/commit/e95134b9cb1885b0da929737858163486a5c399c
DIFF: https://github.com/llvm/llvm-project/commit/e95134b9cb1885b0da929737858163486a5c399c.diff
LOG: [clang][dataflow] Reverse course on `getValue()` deprecation.
In the [value categories RFC](https://discourse.llvm.org/t/70086), I proposed that the end state of the migration should be that `getValue()` should only be legal to call on prvalues.
As a stepping stone, to allow migrating off existing calls to `getValue()`, I proposed introducing `getValueStrict()`, which would already have the new semantics.
However, I've now reconsidered this. Any expression, whether prvalue or glvalue, has a value, so really there isn't any reason to forbid calling `getValue()` on glvalues. I'm therefore removing the deprecation from `getValue()` and transitioning existing `getValueStrict()` calls back to `getValue()`.
The other "strict" accessors are a different case. `setValueStrict()` should only be called on prvalues because glvalues need to have a storage location associated with them; it doesn't make sense to only set a value for them. And, of course, `getStorageLocationStrict()` and `setStorageLocationStrict()` should obviously only be called on glvalues because prvalues don't have storage locations.
Reviewed By: ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D155921
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/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.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 6a6c6856cce95b..460727973ed4c8 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -499,20 +499,14 @@ class Environment {
/// Equivalent to `getValue(getStorageLocation(E, SP))` if `E` is assigned a
/// storage location in the environment, otherwise returns null.
///
- /// The `SP` parameter has no effect.
- ///
- /// This function is deprecated; prefer `getValueStrict()`. For details, see
- /// https://discourse.llvm.org/t/70086.
- Value *getValue(const Expr &E, SkipPast SP) const;
+ /// The `SP` parameter is deprecated and has no effect. New callers should
+ /// avoid passing this parameter.
+ Value *getValue(const Expr &E, SkipPast SP = SkipPast::None) const;
/// Returns the `Value` assigned to the prvalue `E` in the environment, or
/// null if `E` isn't assigned a value in the environment.
///
- /// This function is the preferred alternative to
- /// `getValue(const Expr &, SkipPast)`. Once the migration to strict handling
- /// of value categories is complete (see https://discourse.llvm.org/t/70086),
- /// `getValue()` will be removed and this function will be renamed to
- /// `getValue()`.
+ /// This function is deprecated. Call `getValue(E)` instead.
///
/// Requirements:
///
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d3d5f3338ddf49..903d9ec300e2f6 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -686,7 +686,7 @@ void Environment::setValueStrict(const Expr &E, Value &Val) {
assert(E.isPRValue());
if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
- if (auto *ExistingVal = cast_or_null<StructValue>(getValueStrict(E)))
+ if (auto *ExistingVal = cast_or_null<StructValue>(getValue(E)))
assert(&ExistingVal->getAggregateLoc() == &StructVal->getAggregateLoc());
if (StorageLocation *ExistingLoc = getStorageLocation(E, SkipPast::None))
assert(ExistingLoc == &StructVal->getAggregateLoc());
@@ -724,9 +724,7 @@ Value *Environment::getValue(const Expr &E, SkipPast SP) const {
Value *Environment::getValueStrict(const Expr &E) const {
assert(E.isPRValue());
- Value *Val = getValue(E, SkipPast::None);
-
- return Val;
+ return getValue(E);
}
Value *Environment::createValue(QualType Type) {
@@ -859,7 +857,7 @@ StorageLocation &Environment::createObjectInternal(const VarDecl *D,
// assert that `InitExpr` is interpreted, rather than supplying a
// default value (assuming we don't update the environment API to return
// references).
- Val = getValueStrict(*InitExpr);
+ Val = getValue(*InitExpr);
if (!Val)
Val = createValue(Ty);
@@ -964,8 +962,7 @@ StructValue &refreshStructValue(const Expr &Expr, Environment &Env) {
assert(Expr.getType()->isRecordType());
if (Expr.isPRValue()) {
- if (auto *ExistingVal =
- cast_or_null<StructValue>(Env.getValueStrict(Expr))) {
+ if (auto *ExistingVal = cast_or_null<StructValue>(Env.getValue(Expr))) {
auto &NewVal = Env.create<StructValue>(ExistingVal->getAggregateLoc());
Env.setValueStrict(Expr, NewVal);
return NewVal;
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e80eba506e5383..14afb3f7a669ca 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -238,7 +238,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, SkipPast::None));
+ auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr));
if (Value != nullptr)
return Value->formula();
@@ -403,8 +403,7 @@ StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
void initializeOptionalReference(const Expr *OptionalExpr,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- if (auto *OptionalVal =
- State.Env.getValue(*OptionalExpr, SkipPast::Reference)) {
+ if (auto *OptionalVal = State.Env.getValue(*OptionalExpr)) {
if (OptionalVal->getProperty("has_value") == nullptr) {
setHasValue(*OptionalVal, State.Env.makeAtomicBoolValue());
}
@@ -430,7 +429,7 @@ bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) {
}
Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) {
- Value *Val = Env.getValue(E, SkipPast::Reference);
+ Value *Val = Env.getValue(E);
if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
return Env.getValue(PointerVal->getPointeeLoc());
return Val;
@@ -579,8 +578,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`.
- if (auto *HasValueVal =
- getHasValue(State.Env, State.Env.getValue(E, SkipPast::Reference)))
+ if (auto *HasValueVal = getHasValue(State.Env, State.Env.getValue(E)))
return *HasValueVal;
return State.Env.makeAtomicBoolValue();
}
@@ -714,10 +712,8 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr,
Environment &Env = State.Env;
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- if (auto *LHasVal = getHasValue(
- Env, Env.getValue(*CmpExpr->getArg(0), SkipPast::Reference)))
- if (auto *RHasVal = getHasValue(
- Env, Env.getValue(*CmpExpr->getArg(1), SkipPast::Reference))) {
+ if (auto *LHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(0))))
+ if (auto *RHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(1)))) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
Env.addToFlowCondition(evaluateEquality(A, *CmpValue, LHasVal->formula(),
@@ -729,7 +725,7 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
const clang::Expr *E, Environment &Env) {
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- if (auto *HasVal = getHasValue(Env, Env.getValue(*E, SkipPast::Reference))) {
+ if (auto *HasVal = getHasValue(Env, Env.getValue(*E))) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
Env.addToFlowCondition(
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 8f8f807a3a4b22..87a4fb801cc173 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -49,8 +49,8 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt &S) const {
static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
Environment &Env) {
- Value *LHSValue = Env.getValueStrict(LHS);
- Value *RHSValue = Env.getValueStrict(RHS);
+ Value *LHSValue = Env.getValue(LHS);
+ Value *RHSValue = Env.getValue(RHS);
if (LHSValue == RHSValue)
return Env.getBoolLiteralValue(true);
@@ -91,7 +91,7 @@ static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
}
static void propagateValue(const Expr &From, const Expr &To, Environment &Env) {
- if (auto *Val = Env.getValueStrict(From))
+ if (auto *Val = Env.getValue(From))
Env.setValueStrict(To, *Val);
}
@@ -133,7 +133,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (LHSLoc == nullptr)
break;
- auto *RHSVal = Env.getValueStrict(*RHS);
+ auto *RHSVal = Env.getValue(*RHS);
if (RHSVal == nullptr)
break;
@@ -266,7 +266,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// model that with a fresh value in the environment, unless it's already a
// boolean.
if (auto *SubExprVal =
- dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr)))
+ dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr)))
Env.setValueStrict(*S, *SubExprVal);
else
// FIXME: If integer modeling is added, then update this code to create
@@ -350,7 +350,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
switch (S->getOpcode()) {
case UO_Deref: {
const auto *SubExprVal =
- cast_or_null<PointerValue>(Env.getValueStrict(*SubExpr));
+ cast_or_null<PointerValue>(Env.getValue(*SubExpr));
if (SubExprVal == nullptr)
break;
@@ -367,8 +367,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
break;
}
case UO_LNot: {
- auto *SubExprVal =
- dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr));
+ auto *SubExprVal = dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr));
if (SubExprVal == nullptr)
break;
@@ -417,7 +416,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
if (Ret->isPRValue()) {
- auto *Val = Env.getValueStrict(*Ret);
+ auto *Val = Env.getValue(*Ret);
if (Val == nullptr)
return;
@@ -491,8 +490,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (S->isElidable()) {
Env.setStorageLocation(*S, *ArgLoc);
- } else if (auto *ArgVal = cast_or_null<StructValue>(
- Env.getValue(*Arg, SkipPast::Reference))) {
+ } else if (auto *ArgVal = cast_or_null<StructValue>(Env.getValue(*Arg))) {
auto &Val = *cast<StructValue>(Env.createValue(S->getType()));
Env.setValueStrict(*S, Val);
copyRecord(ArgVal->getAggregateLoc(), Val.getAggregateLoc(), Env);
@@ -592,7 +590,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);
- Value *SubExprVal = Env.getValueStrict(*SubExpr);
+ Value *SubExprVal = Env.getValue(*SubExpr);
if (SubExprVal == nullptr)
return;
@@ -707,7 +705,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// corresponding environment.
if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
if (auto *Val =
- dyn_cast_or_null<BoolValue>(SubExprEnv->getValueStrict(SubExpr)))
+ dyn_cast_or_null<BoolValue>(SubExprEnv->getValue(SubExpr)))
return *Val;
// The sub-expression may lie within a basic block that isn't reachable,
@@ -715,9 +713,9 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
// within the current environment and then try to get the value that gets
// assigned to it.
- if (Env.getValueStrict(SubExpr) == nullptr)
+ if (Env.getValue(SubExpr) == nullptr)
Visit(&SubExpr);
- if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValueStrict(SubExpr)))
+ if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValue(SubExpr)))
return *Val;
// If the value of `SubExpr` is still unknown, we create a fresh symbolic
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 1b68d76ffc8cd8..bd2074374e1fbc 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -125,10 +125,10 @@ class TerminatorVisitor
private:
TerminatorVisitorRetTy extendFlowCondition(const Expr &Cond) {
// The terminator sub-expression might not be evaluated.
- if (Env.getValueStrict(Cond) == nullptr)
+ if (Env.getValue(Cond) == nullptr)
transfer(StmtToEnv, Cond, Env);
- auto *Val = cast_or_null<BoolValue>(Env.getValueStrict(Cond));
+ auto *Val = cast_or_null<BoolValue>(Env.getValue(Cond));
// Value merging depends on flow conditions from
diff erent 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
@@ -407,7 +407,7 @@ builtinTransferInitializer(const CFGInitializer &Elt,
return;
ParentLoc->setChild(*Member, InitExprLoc);
- } else if (auto *InitExprVal = Env.getValueStrict(*InitExpr)) {
+ } else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
if (Member->getType()->isRecordType()) {
auto *InitValStruct = cast<StructValue>(InitExprVal);
// FIXME: Rather than performing a copy here, we should really be
diff --git a/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
index 481bcd15443f8d..ef83dddcfa57ed 100644
--- a/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
@@ -114,7 +114,7 @@ getValueAndSignProperties(const UnaryOperator *UO,
return {nullptr, {}, {}};
// Value of the unary op.
- auto *UnaryOpValue = State.Env.getValueStrict(*UO);
+ auto *UnaryOpValue = State.Env.getValue(*UO);
if (!UnaryOpValue) {
UnaryOpValue = &State.Env.makeAtomicBoolValue();
State.Env.setValueStrict(*UO, *UnaryOpValue);
@@ -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.getValueStrict(*BO))) {
+ if (BoolValue *V = cast_or_null<BoolValue>(State.Env.getValue(*BO))) {
Comp = &V->formula();
} else {
Comp = &A.makeAtomRef(A.makeAtom());
@@ -143,8 +143,8 @@ void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
// FIXME Use this as well:
// auto *NegatedComp = &State.Env.makeNot(*Comp);
- auto *LHS = State.Env.getValueStrict(*BO->getLHS());
- auto *RHS = State.Env.getValueStrict(*BO->getRHS());
+ auto *LHS = State.Env.getValue(*BO->getLHS());
+ auto *RHS = State.Env.getValue(*BO->getRHS());
if (!LHS || !RHS)
return;
@@ -271,7 +271,7 @@ Value *getOrCreateValue(const Expr *E, Environment &Env) {
Env.setValue(*Loc, *Val);
}
} else {
- Val = Env.getValueStrict(*E);
+ Val = Env.getValue(*E);
if (!Val) {
Val = Env.createValue(E->getType());
Env.setValueStrict(*E, *Val);
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 57c8a5f3589bc4..33e9f06bc6439b 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5546,7 +5546,7 @@ TEST(TransferTest, BuiltinFunctionModeled) {
ASTCtx));
ASSERT_THAT(ImplicitCast, NotNull());
- EXPECT_THAT(Env.getValueStrict(*ImplicitCast), IsNull());
+ EXPECT_THAT(Env.getValue(*ImplicitCast), IsNull());
});
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 44d6fec962ab5e..83faed42bad030 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -415,7 +415,7 @@ class SpecialBoolAnalysis final
if (const auto *E = selectFirst<CXXConstructExpr>(
"call", match(cxxConstructExpr(HasSpecialBoolType).bind("call"), *S,
getASTContext()))) {
- cast<StructValue>(Env.getValueStrict(*E))
+ cast<StructValue>(Env.getValue(*E))
->setProperty("is_set", Env.getBoolLiteralValue(false));
} else if (const auto *E = selectFirst<CXXMemberCallExpr>(
"call", match(cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(
@@ -572,7 +572,7 @@ class OptionalIntAnalysis final
*S, getASTContext());
if (const auto *E = selectFirst<CXXConstructExpr>(
"construct", Matches)) {
- cast<StructValue>(Env.getValueStrict(*E))
+ cast<StructValue>(Env.getValue(*E))
->setProperty("has_value", Env.getBoolLiteralValue(false));
} else if (const auto *E =
selectFirst<CXXOperatorCallExpr>("operator", Matches)) {
More information about the cfe-commits
mailing list