Skip to content

Commit

Permalink
[clang][dataflow] Use Strict accessors in more places in Transfer.cpp.
Browse files Browse the repository at this point in the history 
This patch handles the straightforward cases. Upcoming separate patches will handle the cases that are more subtle.

This patch is part of the ongoing migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details).

Depends On D150653

Reviewed By: sammccall, ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D150655
  • Loading branch information
@martinboehme
martinboehme committed May 22, 2023
1 parent 0d95b20 commit 5a16665
Showing 1 changed file with 52 additions and 46 deletions.
98 changes: 52 additions & 46 deletions clang/lib/Analysis/FlowSensitive/Transfer.cpp
View file
Expand Up @@ -48,10 +48,8 @@ const Environment *StmtToEnvMap::getEnvironment(const Stmt &S) const {

static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
Environment &Env) {
if (auto *LHSValue =
dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
if (auto *RHSValue =
dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
if (auto *LHSValue = dyn_cast_or_null<BoolValue>(Env.getValueStrict(LHS)))
if (auto *RHSValue = dyn_cast_or_null<BoolValue>(Env.getValueStrict(RHS)))
return Env.makeIff(*LHSValue, *RHSValue);

return Env.makeAtomicBoolValue();
Expand Down Expand Up @@ -121,9 +119,7 @@ static BoolValue &unpackValue(BoolValue &V, Environment &Env) {
// value, if any unpacking occured. Also, does the lvalue-to-rvalue conversion,
// by skipping past the reference.
static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
// FIXME: this is too flexible: it _allows_ a reference, while it should
// _require_ one, since lvalues should always be wrapped in `ReferenceValue`.
auto *Loc = Env.getStorageLocation(E, SkipPast::Reference);
auto *Loc = Env.getStorageLocationStrict(E);
if (Loc == nullptr)
return nullptr;
auto *Val = Env.getValue(*Loc);
Expand All @@ -139,6 +135,29 @@ static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
return &UnpackedVal;
}

static void propagateValue(const Expr &From, const Expr &To, Environment &Env) {
if (auto *Val = Env.getValueStrict(From))
Env.setValueStrict(To, *Val);
}

static void propagateStorageLocation(const Expr &From, const Expr &To,
Environment &Env) {
if (auto *Loc = Env.getStorageLocationStrict(From))
Env.setStorageLocationStrict(To, *Loc);
}

// Forwards the value or storage location of `From` to `To` in cases where
// `From` may be either a glvalue or a prvalue. `To` must be a glvalue iff
// `From` is a glvalue.
static void propagateValueOrStorageLocation(const Expr &From, const Expr &To,
Environment &Env) {
assert(From.isGLValue() == To.isGLValue());
if (From.isGLValue())
propagateStorageLocation(From, To, Env);
else
propagateValue(From, To, Env);
}

namespace {

class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
Expand All @@ -155,13 +174,11 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {

switch (S->getOpcode()) {
case BO_Assign: {
auto *LHSLoc = Env.getStorageLocation(*LHS, SkipPast::Reference);
auto *LHSLoc = Env.getStorageLocationStrict(*LHS);
if (LHSLoc == nullptr)
break;

// No skipping should be necessary, because any lvalues should have
// already been stripped off in evaluating the LValueToRValue cast.
auto *RHSVal = Env.getValue(*RHS, SkipPast::None);
auto *RHSVal = Env.getValueStrict(*RHS);
if (RHSVal == nullptr)
break;

Expand Down Expand Up @@ -276,7 +293,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
}

if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None))
if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
Env.setValue(Loc, *InitExprVal);

if (Env.getValue(Loc) == nullptr) {
Expand Down Expand Up @@ -443,7 +460,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}
case UO_LNot: {
auto *SubExprVal =
dyn_cast_or_null<BoolValue>(Env.getValue(*SubExpr, SkipPast::None));
dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr));
if (SubExprVal == nullptr)
break;

Expand Down Expand Up @@ -653,19 +670,13 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);

auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
if (SubExprLoc == nullptr)
return;

Env.setStorageLocation(*S, *SubExprLoc);
propagateValue(*SubExpr, *S, Env);
}
}

void VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *S) {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
if (Value *Val = Env.createValue(S->getType()))
Env.setValue(Loc, *Val);
Env.setValueStrict(*S, *Val);
}

void VisitCallExpr(const CallExpr *S) {
Expand Down Expand Up @@ -703,45 +714,43 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);

auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
if (SubExprLoc == nullptr)
Value *SubExprVal = Env.getValueStrict(*SubExpr);
if (SubExprVal == nullptr)
return;

Env.setStorageLocation(*S, *SubExprLoc);
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocationStrict(*S, Loc);
Env.setValue(Loc, *SubExprVal);
}

void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *S) {
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);

auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
if (SubExprLoc == nullptr)
return;

Env.setStorageLocation(*S, *SubExprLoc);
propagateValue(*SubExpr, *S, Env);
}

void VisitCXXStaticCastExpr(const CXXStaticCastExpr *S) {
if (S->getCastKind() == CK_NoOp) {
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);

auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
if (SubExprLoc == nullptr)
return;

Env.setStorageLocation(*S, *SubExprLoc);
propagateValueOrStorageLocation(*SubExpr, *S, Env);
}
}

void VisitConditionalOperator(const ConditionalOperator *S) {
// FIXME: Revisit this once flow conditions are added to the framework. For
// `a = b ? c : d` we can add `b => a == c && !b => a == d` to the flow
// condition.
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
if (Value *Val = Env.createValue(S->getType()))
Env.setValue(Loc, *Val);
if (S->isGLValue()) {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocationStrict(*S, Loc);
if (Value *Val = Env.createValue(S->getType()))
Env.setValue(Loc, *Val);
} else if (Value *Val = Env.createValue(S->getType())) {
Env.setValueStrict(*S, *Val);
}
}

void VisitInitListExpr(const InitListExpr *S) {
Expand Down Expand Up @@ -780,9 +789,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}

void VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *S) {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
Env.setValue(Loc, Env.getBoolLiteralValue(S->getValue()));
Env.setValueStrict(*S, Env.getBoolLiteralValue(S->getValue()));
}

void VisitParenExpr(const ParenExpr *S) {
Expand Down Expand Up @@ -814,11 +821,11 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (!SubExprEnv)
return nullptr;

if (auto *Val = dyn_cast_or_null<BoolValue>(
SubExprEnv->getValue(SubExpr, SkipPast::Reference)))
if (auto *Val =
dyn_cast_or_null<BoolValue>(SubExprEnv->getValueStrict(SubExpr)))
return Val;

if (Env.getStorageLocation(SubExpr, SkipPast::None) == nullptr) {
if (Env.getValueStrict(SubExpr) == nullptr) {
// Sub-expressions that are logic operators are not added in basic blocks
// (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
// operator, it may not have been evaluated and assigned a value yet. In
Expand All @@ -827,8 +834,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
Visit(&SubExpr);
}

if (auto *Val = dyn_cast_or_null<BoolValue>(
Env.getValue(SubExpr, SkipPast::Reference)))
if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValueStrict(SubExpr)))
return Val;

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

0 comments on commit 5a16665

Please sign in to comment.