Skip to content

Commit

Permalink
[clang][dataflow] Reverse course on getValue() deprecation.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@martinboehme
martinboehme committed Jul 27, 2023
1 parent 1b334a2 commit e95134b
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 54 deletions.
14 changes: 4 additions & 10 deletions clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
///
Expand Down
11 changes: 4 additions & 7 deletions clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand Down
18 changes: 7 additions & 11 deletions clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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());
}
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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(),
Expand All @@ -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(
Expand Down
28 changes: 13 additions & 15 deletions clang/lib/Analysis/FlowSensitive/Transfer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -707,17 +705,17 @@ 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,
// even if we need it to evaluate the current (reachable) expression
// (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
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5546,7 +5546,7 @@ TEST(TransferTest, BuiltinFunctionModeled) {
ASTCtx));

ASSERT_THAT(ImplicitCast, NotNull());
EXPECT_THAT(Env.getValueStrict(*ImplicitCast), IsNull());
EXPECT_THAT(Env.getValue(*ImplicitCast), IsNull());
});
}

Expand Down
4 changes: 2 additions & 2 deletions clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)) {
Expand Down

0 comments on commit e95134b

Please sign in to comment.