Skip to content

Commit e95134b

Browse files
martinboehmemartinboehme
committed
[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
1 parent 1b334a2 commit e95134b

File tree

8 files changed

+39
-54
lines changed

8 files changed

+39
-54
lines changed

‎clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -499,20 +499,14 @@ class Environment {
499499
/// Equivalent to `getValue(getStorageLocation(E, SP))` if `E` is assigned a
500500
/// storage location in the environment, otherwise returns null.
501501
///
502-
/// The `SP` parameter has no effect.
503-
///
504-
/// This function is deprecated; prefer `getValueStrict()`. For details, see
505-
/// https://discourse.llvm.org/t/70086.
506-
Value *getValue(const Expr &E, SkipPast SP) const;
502+
/// The `SP` parameter is deprecated and has no effect. New callers should
503+
/// avoid passing this parameter.
504+
Value *getValue(const Expr &E, SkipPast SP = SkipPast::None) const;
507505

508506
/// Returns the `Value` assigned to the prvalue `E` in the environment, or
509507
/// null if `E` isn't assigned a value in the environment.
510508
///
511-
/// This function is the preferred alternative to
512-
/// `getValue(const Expr &, SkipPast)`. Once the migration to strict handling
513-
/// of value categories is complete (see https://discourse.llvm.org/t/70086),
514-
/// `getValue()` will be removed and this function will be renamed to
515-
/// `getValue()`.
509+
/// This function is deprecated. Call `getValue(E)` instead.
516510
///
517511
/// Requirements:
518512
///

‎clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -686,7 +686,7 @@ void Environment::setValueStrict(const Expr &E, Value &Val) {
686686
assert(E.isPRValue());
687687

688688
if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
689-
if (auto *ExistingVal = cast_or_null<StructValue>(getValueStrict(E)))
689+
if (auto *ExistingVal = cast_or_null<StructValue>(getValue(E)))
690690
assert(&ExistingVal->getAggregateLoc() == &StructVal->getAggregateLoc());
691691
if (StorageLocation *ExistingLoc = getStorageLocation(E, SkipPast::None))
692692
assert(ExistingLoc == &StructVal->getAggregateLoc());
@@ -724,9 +724,7 @@ Value *Environment::getValue(const Expr &E, SkipPast SP) const {
724724

725725
Value *Environment::getValueStrict(const Expr &E) const {
726726
assert(E.isPRValue());
727-
Value *Val = getValue(E, SkipPast::None);
728-
729-
return Val;
727+
return getValue(E);
730728
}
731729

732730
Value *Environment::createValue(QualType Type) {
@@ -859,7 +857,7 @@ StorageLocation &Environment::createObjectInternal(const VarDecl *D,
859857
// assert that `InitExpr` is interpreted, rather than supplying a
860858
// default value (assuming we don't update the environment API to return
861859
// references).
862-
Val = getValueStrict(*InitExpr);
860+
Val = getValue(*InitExpr);
863861
if (!Val)
864862
Val = createValue(Ty);
865863

@@ -964,8 +962,7 @@ StructValue &refreshStructValue(const Expr &Expr, Environment &Env) {
964962
assert(Expr.getType()->isRecordType());
965963

966964
if (Expr.isPRValue()) {
967-
if (auto *ExistingVal =
968-
cast_or_null<StructValue>(Env.getValueStrict(Expr))) {
965+
if (auto *ExistingVal = cast_or_null<StructValue>(Env.getValue(Expr))) {
969966
auto &NewVal = Env.create<StructValue>(ExistingVal->getAggregateLoc());
970967
Env.setValueStrict(Expr, NewVal);
971968
return NewVal;

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ auto isComparisonOperatorCall(L lhs_arg_matcher, R rhs_arg_matcher) {
238238

239239
/// Ensures that `Expr` is mapped to a `BoolValue` and returns its formula.
240240
const Formula &forceBoolValue(Environment &Env, const Expr &Expr) {
241-
auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr, SkipPast::None));
241+
auto *Value = cast_or_null<BoolValue>(Env.getValue(Expr));
242242
if (Value != nullptr)
243243
return Value->formula();
244244

@@ -403,8 +403,7 @@ StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
403403
void initializeOptionalReference(const Expr *OptionalExpr,
404404
const MatchFinder::MatchResult &,
405405
LatticeTransferState &State) {
406-
if (auto *OptionalVal =
407-
State.Env.getValue(*OptionalExpr, SkipPast::Reference)) {
406+
if (auto *OptionalVal = State.Env.getValue(*OptionalExpr)) {
408407
if (OptionalVal->getProperty("has_value") == nullptr) {
409408
setHasValue(*OptionalVal, State.Env.makeAtomicBoolValue());
410409
}
@@ -430,7 +429,7 @@ bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) {
430429
}
431430

432431
Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) {
433-
Value *Val = Env.getValue(E, SkipPast::Reference);
432+
Value *Val = Env.getValue(E);
434433
if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
435434
return Env.getValue(PointerVal->getPointeeLoc());
436435
return Val;
@@ -579,8 +578,7 @@ BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
579578

580579
// This is a constructor/assignment call for `optional<T>` with argument of
581580
// type `optional<U>` such that `T` is constructible from `U`.
582-
if (auto *HasValueVal =
583-
getHasValue(State.Env, State.Env.getValue(E, SkipPast::Reference)))
581+
if (auto *HasValueVal = getHasValue(State.Env, State.Env.getValue(E)))
584582
return *HasValueVal;
585583
return State.Env.makeAtomicBoolValue();
586584
}
@@ -714,10 +712,8 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr,
714712
Environment &Env = State.Env;
715713
auto &A = Env.arena();
716714
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
717-
if (auto *LHasVal = getHasValue(
718-
Env, Env.getValue(*CmpExpr->getArg(0), SkipPast::Reference)))
719-
if (auto *RHasVal = getHasValue(
720-
Env, Env.getValue(*CmpExpr->getArg(1), SkipPast::Reference))) {
715+
if (auto *LHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(0))))
716+
if (auto *RHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(1)))) {
721717
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
722718
CmpValue = &A.makeNot(*CmpValue);
723719
Env.addToFlowCondition(evaluateEquality(A, *CmpValue, LHasVal->formula(),
@@ -729,7 +725,7 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
729725
const clang::Expr *E, Environment &Env) {
730726
auto &A = Env.arena();
731727
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
732-
if (auto *HasVal = getHasValue(Env, Env.getValue(*E, SkipPast::Reference))) {
728+
if (auto *HasVal = getHasValue(Env, Env.getValue(*E))) {
733729
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
734730
CmpValue = &A.makeNot(*CmpValue);
735731
Env.addToFlowCondition(

‎clang/lib/Analysis/FlowSensitive/Transfer.cpp

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt &S) const {
4949

5050
static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
5151
Environment &Env) {
52-
Value *LHSValue = Env.getValueStrict(LHS);
53-
Value *RHSValue = Env.getValueStrict(RHS);
52+
Value *LHSValue = Env.getValue(LHS);
53+
Value *RHSValue = Env.getValue(RHS);
5454

5555
if (LHSValue == RHSValue)
5656
return Env.getBoolLiteralValue(true);
@@ -91,7 +91,7 @@ static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
9191
}
9292

9393
static void propagateValue(const Expr &From, const Expr &To, Environment &Env) {
94-
if (auto *Val = Env.getValueStrict(From))
94+
if (auto *Val = Env.getValue(From))
9595
Env.setValueStrict(To, *Val);
9696
}
9797

@@ -133,7 +133,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
133133
if (LHSLoc == nullptr)
134134
break;
135135

136-
auto *RHSVal = Env.getValueStrict(*RHS);
136+
auto *RHSVal = Env.getValue(*RHS);
137137
if (RHSVal == nullptr)
138138
break;
139139

@@ -266,7 +266,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
266266
// model that with a fresh value in the environment, unless it's already a
267267
// boolean.
268268
if (auto *SubExprVal =
269-
dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr)))
269+
dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr)))
270270
Env.setValueStrict(*S, *SubExprVal);
271271
else
272272
// FIXME: If integer modeling is added, then update this code to create
@@ -350,7 +350,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
350350
switch (S->getOpcode()) {
351351
case UO_Deref: {
352352
const auto *SubExprVal =
353-
cast_or_null<PointerValue>(Env.getValueStrict(*SubExpr));
353+
cast_or_null<PointerValue>(Env.getValue(*SubExpr));
354354
if (SubExprVal == nullptr)
355355
break;
356356

@@ -367,8 +367,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
367367
break;
368368
}
369369
case UO_LNot: {
370-
auto *SubExprVal =
371-
dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr));
370+
auto *SubExprVal = dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr));
372371
if (SubExprVal == nullptr)
373372
break;
374373

@@ -417,7 +416,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
417416
return;
418417

419418
if (Ret->isPRValue()) {
420-
auto *Val = Env.getValueStrict(*Ret);
419+
auto *Val = Env.getValue(*Ret);
421420
if (Val == nullptr)
422421
return;
423422

@@ -491,8 +490,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
491490

492491
if (S->isElidable()) {
493492
Env.setStorageLocation(*S, *ArgLoc);
494-
} else if (auto *ArgVal = cast_or_null<StructValue>(
495-
Env.getValue(*Arg, SkipPast::Reference))) {
493+
} else if (auto *ArgVal = cast_or_null<StructValue>(Env.getValue(*Arg))) {
496494
auto &Val = *cast<StructValue>(Env.createValue(S->getType()));
497495
Env.setValueStrict(*S, Val);
498496
copyRecord(ArgVal->getAggregateLoc(), Val.getAggregateLoc(), Env);
@@ -592,7 +590,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
592590
const Expr *SubExpr = S->getSubExpr();
593591
assert(SubExpr != nullptr);
594592

595-
Value *SubExprVal = Env.getValueStrict(*SubExpr);
593+
Value *SubExprVal = Env.getValue(*SubExpr);
596594
if (SubExprVal == nullptr)
597595
return;
598596

@@ -707,17 +705,17 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
707705
// corresponding environment.
708706
if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
709707
if (auto *Val =
710-
dyn_cast_or_null<BoolValue>(SubExprEnv->getValueStrict(SubExpr)))
708+
dyn_cast_or_null<BoolValue>(SubExprEnv->getValue(SubExpr)))
711709
return *Val;
712710

713711
// The sub-expression may lie within a basic block that isn't reachable,
714712
// even if we need it to evaluate the current (reachable) expression
715713
// (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
716714
// within the current environment and then try to get the value that gets
717715
// assigned to it.
718-
if (Env.getValueStrict(SubExpr) == nullptr)
716+
if (Env.getValue(SubExpr) == nullptr)
719717
Visit(&SubExpr);
720-
if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValueStrict(SubExpr)))
718+
if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValue(SubExpr)))
721719
return *Val;
722720

723721
// If the value of `SubExpr` is still unknown, we create a fresh symbolic

‎clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,10 @@ class TerminatorVisitor
125125
private:
126126
TerminatorVisitorRetTy extendFlowCondition(const Expr &Cond) {
127127
// The terminator sub-expression might not be evaluated.
128-
if (Env.getValueStrict(Cond) == nullptr)
128+
if (Env.getValue(Cond) == nullptr)
129129
transfer(StmtToEnv, Cond, Env);
130130

131-
auto *Val = cast_or_null<BoolValue>(Env.getValueStrict(Cond));
131+
auto *Val = cast_or_null<BoolValue>(Env.getValue(Cond));
132132
// Value merging depends on flow conditions from different environments
133133
// being mutually exclusive -- that is, they cannot both be true in their
134134
// entirety (even if they may share some clauses). So, we need *some* value
@@ -407,7 +407,7 @@ builtinTransferInitializer(const CFGInitializer &Elt,
407407
return;
408408

409409
ParentLoc->setChild(*Member, InitExprLoc);
410-
} else if (auto *InitExprVal = Env.getValueStrict(*InitExpr)) {
410+
} else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
411411
if (Member->getType()->isRecordType()) {
412412
auto *InitValStruct = cast<StructValue>(InitExprVal);
413413
// FIXME: Rather than performing a copy here, we should really be

‎clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ getValueAndSignProperties(const UnaryOperator *UO,
114114
return {nullptr, {}, {}};
115115

116116
// Value of the unary op.
117-
auto *UnaryOpValue = State.Env.getValueStrict(*UO);
117+
auto *UnaryOpValue = State.Env.getValue(*UO);
118118
if (!UnaryOpValue) {
119119
UnaryOpValue = &State.Env.makeAtomicBoolValue();
120120
State.Env.setValueStrict(*UO, *UnaryOpValue);
@@ -133,7 +133,7 @@ void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
133133
LatticeTransferState &State) {
134134
auto &A = State.Env.arena();
135135
const Formula *Comp;
136-
if (BoolValue *V = cast_or_null<BoolValue>(State.Env.getValueStrict(*BO))) {
136+
if (BoolValue *V = cast_or_null<BoolValue>(State.Env.getValue(*BO))) {
137137
Comp = &V->formula();
138138
} else {
139139
Comp = &A.makeAtomRef(A.makeAtom());
@@ -143,8 +143,8 @@ void transferBinary(const BinaryOperator *BO, const MatchFinder::MatchResult &M,
143143
// FIXME Use this as well:
144144
// auto *NegatedComp = &State.Env.makeNot(*Comp);
145145

146-
auto *LHS = State.Env.getValueStrict(*BO->getLHS());
147-
auto *RHS = State.Env.getValueStrict(*BO->getRHS());
146+
auto *LHS = State.Env.getValue(*BO->getLHS());
147+
auto *RHS = State.Env.getValue(*BO->getRHS());
148148

149149
if (!LHS || !RHS)
150150
return;
@@ -271,7 +271,7 @@ Value *getOrCreateValue(const Expr *E, Environment &Env) {
271271
Env.setValue(*Loc, *Val);
272272
}
273273
} else {
274-
Val = Env.getValueStrict(*E);
274+
Val = Env.getValue(*E);
275275
if (!Val) {
276276
Val = Env.createValue(E->getType());
277277
Env.setValueStrict(*E, *Val);

‎clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5546,7 +5546,7 @@ TEST(TransferTest, BuiltinFunctionModeled) {
55465546
ASTCtx));
55475547

55485548
ASSERT_THAT(ImplicitCast, NotNull());
5549-
EXPECT_THAT(Env.getValueStrict(*ImplicitCast), IsNull());
5549+
EXPECT_THAT(Env.getValue(*ImplicitCast), IsNull());
55505550
});
55515551
}
55525552

‎clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,7 @@ class SpecialBoolAnalysis final
415415
if (const auto *E = selectFirst<CXXConstructExpr>(
416416
"call", match(cxxConstructExpr(HasSpecialBoolType).bind("call"), *S,
417417
getASTContext()))) {
418-
cast<StructValue>(Env.getValueStrict(*E))
418+
cast<StructValue>(Env.getValue(*E))
419419
->setProperty("is_set", Env.getBoolLiteralValue(false));
420420
} else if (const auto *E = selectFirst<CXXMemberCallExpr>(
421421
"call", match(cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(
@@ -572,7 +572,7 @@ class OptionalIntAnalysis final
572572
*S, getASTContext());
573573
if (const auto *E = selectFirst<CXXConstructExpr>(
574574
"construct", Matches)) {
575-
cast<StructValue>(Env.getValueStrict(*E))
575+
cast<StructValue>(Env.getValue(*E))
576576
->setProperty("has_value", Env.getBoolLiteralValue(false));
577577
} else if (const auto *E =
578578
selectFirst<CXXOperatorCallExpr>("operator", Matches)) {

0 commit comments

Comments
 (0)