Skip to content

Commit

Permalink
Revert "[clang][dataflow] Model conditional operator correctly. (#89213…
Browse files Browse the repository at this point in the history 
…)"

This reverts commit abb958f.
  • Loading branch information
@martinboehme
martinboehme committed Apr 22, 2024
1 parent abb958f commit b045650
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 149 deletions.
15 changes: 0 additions & 15 deletions clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,21 +244,6 @@ class Environment {
Environment::ValueModel &Model,
ExprJoinBehavior ExprBehavior);

/// Returns a value that approximates both `Val1` and `Val2`, or null if no
/// such value can be produced.
///
/// `Env1` and `Env2` can be used to query child values and path condition
/// implications of `Val1` and `Val2` respectively. The joined value will be
/// produced in `JoinedEnv`.
///
/// Requirements:
///
/// `Val1` and `Val2` must model values of type `Type`.
static Value *joinValues(QualType Ty, Value *Val1, const Environment &Env1,
Value *Val2, const Environment &Env2,
Environment &JoinedEnv,
Environment::ValueModel &Model);

/// Widens the environment point-wise, using `PrevEnv` as needed to inform the
/// approximation.
///
Expand Down
3 changes: 1 addition & 2 deletions clang/include/clang/Analysis/FlowSensitive/Transfer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ class StmtToEnvMap {
/// Requirements:
///
/// `S` must not be `ParenExpr` or `ExprWithCleanups`.
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env,
Environment::ValueModel &Model);
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);

} // namespace dataflow
} // namespace clang
Expand Down
46 changes: 22 additions & 24 deletions clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,13 @@ joinLocToVal(const llvm::MapVector<const StorageLocation *, Value *> &LocToVal,
continue;
assert(It->second != nullptr);

if (Value *JoinedVal = Environment::joinValues(
Loc->getType(), Val, Env1, It->second, Env2, JoinedEnv, Model)) {
if (areEquivalentValues(*Val, *It->second)) {
Result.insert({Loc, Val});
continue;
}

if (Value *JoinedVal = joinDistinctValues(
Loc->getType(), *Val, Env1, *It->second, Env2, JoinedEnv, Model)) {
Result.insert({Loc, JoinedVal});
}
}
Expand Down Expand Up @@ -770,16 +775,27 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
JoinedEnv.LocForRecordReturnVal = EnvA.LocForRecordReturnVal;
JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc;

if (EnvA.CallStack.empty()) {
if (EnvA.ReturnVal == nullptr || EnvB.ReturnVal == nullptr) {
// `ReturnVal` might not always get set -- for example if we have a return
// statement of the form `return some_other_func()` and we decide not to
// analyze `some_other_func()`.
// In this case, we can't say anything about the joined return value -- we
// don't simply want to propagate the return value that we do have, because
// it might not be the correct one.
// This occurs for example in the test `ContextSensitiveMutualRecursion`.
JoinedEnv.ReturnVal = nullptr;
} else if (areEquivalentValues(*EnvA.ReturnVal, *EnvB.ReturnVal)) {
JoinedEnv.ReturnVal = EnvA.ReturnVal;
} else {
assert(!EnvA.CallStack.empty());
// FIXME: Make `CallStack` a vector of `FunctionDecl` so we don't need this
// cast.
auto *Func = dyn_cast<FunctionDecl>(EnvA.CallStack.back());
assert(Func != nullptr);
JoinedEnv.ReturnVal =
joinValues(Func->getReturnType(), EnvA.ReturnVal, EnvA, EnvB.ReturnVal,
EnvB, JoinedEnv, Model);
if (Value *JoinedVal =
joinDistinctValues(Func->getReturnType(), *EnvA.ReturnVal, EnvA,
*EnvB.ReturnVal, EnvB, JoinedEnv, Model))
JoinedEnv.ReturnVal = JoinedVal;
}

if (EnvA.ReturnLoc == EnvB.ReturnLoc)
Expand All @@ -805,24 +821,6 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
return JoinedEnv;
}

Value *Environment::joinValues(QualType Ty, Value *Val1,
const Environment &Env1, Value *Val2,
const Environment &Env2, Environment &JoinedEnv,
Environment::ValueModel &Model) {
if (Val1 == nullptr || Val2 == nullptr)
// We can't say anything about the joined value -- even if one of the values
// is non-null, we don't want to simply propagate it, because it would be
// too specific: Because the other value is null, that means we have no
// information at all about the value (i.e. the value is unconstrained).
return nullptr;

if (areEquivalentValues(*Val1, *Val2))
// Arbitrarily return one of the two values.
return Val1;

return joinDistinctValues(Ty, *Val1, Env1, *Val2, Env2, JoinedEnv, Model);
}

StorageLocation &Environment::createStorageLocation(QualType Type) {
return DACtx->createStorageLocation(Type);
}
Expand Down
57 changes: 15 additions & 42 deletions clang/lib/Analysis/FlowSensitive/Transfer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,8 @@ namespace {

class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
public:
TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
Environment::ValueModel &Model)
: StmtToEnv(StmtToEnv), Env(Env), Model(Model) {}
TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env)
: StmtToEnv(StmtToEnv), Env(Env) {}

void VisitBinaryOperator(const BinaryOperator *S) {
const Expr *LHS = S->getLHS();
Expand Down Expand Up @@ -642,41 +641,17 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}

void VisitConditionalOperator(const ConditionalOperator *S) {
const Environment *TrueEnv = StmtToEnv.getEnvironment(*S->getTrueExpr());
const Environment *FalseEnv = StmtToEnv.getEnvironment(*S->getFalseExpr());

if (TrueEnv == nullptr || FalseEnv == nullptr) {
// We should always have an environment as we should visit the true /
// false branches before the conditional operator.
assert(false);
return;
}

if (S->isGLValue()) {
StorageLocation *TrueLoc = TrueEnv->getStorageLocation(*S->getTrueExpr());
StorageLocation *FalseLoc =
FalseEnv->getStorageLocation(*S->getFalseExpr());
if (TrueLoc == FalseLoc && TrueLoc != nullptr)
Env.setStorageLocation(*S, *TrueLoc);
} else if (!S->getType()->isRecordType()) {
// The conditional operator can evaluate to either of the values of the
// two branches. To model this, join these two values together to yield
// the result of the conditional operator.
// Note: Most joins happen in `computeBlockInputState()`, but this case is
// different:
// - `computeBlockInputState()` (which in turn calls `Environment::join()`
// joins values associated with the _same_ expression or storage
// location, then associates the joined value with that expression or
// storage location. This join has nothing to do with transfer --
// instead, it joins together the results of performing transfer on two
// different blocks.
// - Here, we join values associated with _different_ expressions (the
// true and false branch), then associate the joined value with a third
// expression (the conditional operator itself). This join is what it
// means to perform transfer on the conditional operator.
if (Value *Val = Environment::joinValues(
S->getType(), TrueEnv->getValue(*S->getTrueExpr()), *TrueEnv,
FalseEnv->getValue(*S->getFalseExpr()), *FalseEnv, Env, Model))
// 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.
// When we do this, we will need to retrieve the values of the operands from
// the environments for the basic blocks they are computed in, in a similar
// way to how this is done for short-circuited logical operators in
// `getLogicOperatorSubExprValue()`.
if (S->isGLValue())
Env.setStorageLocation(*S, Env.createObject(S->getType()));
else if (!S->getType()->isRecordType()) {
if (Value *Val = Env.createValue(S->getType()))
Env.setValue(*S, *Val);
}
}
Expand Down Expand Up @@ -835,14 +810,12 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {

const StmtToEnvMap &StmtToEnv;
Environment &Env;
Environment::ValueModel &Model;
};

} // namespace

void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env,
Environment::ValueModel &Model) {
TransferVisitor(StmtToEnv, Env, Model).Visit(&S);
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
TransferVisitor(StmtToEnv, Env).Visit(&S);
}

} // namespace dataflow
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ builtinTransferStatement(unsigned CurBlockID, const CFGStmt &Elt,
const Stmt *S = Elt.getStmt();
assert(S != nullptr);
transfer(StmtToEnvMap(AC.ACFG, AC.BlockStates, CurBlockID, InputState), *S,
InputState.Env, AC.Analysis);
InputState.Env);
}

/// Built-in transfer function for `CFGInitializer`.
Expand Down Expand Up @@ -452,7 +452,7 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext &AC,
// terminator condition, but not as a `CFGElement`. The condition of an if
// statement is one such example.
transfer(StmtToEnvMap(AC.ACFG, AC.BlockStates, Block.getBlockID(), State),
*TerminatorCond, State.Env, AC.Analysis);
*TerminatorCond, State.Env);

// If the transfer function didn't produce a value, create an atom so that
// we have *some* value for the condition expression. This ensures that
Expand Down
4 changes: 2 additions & 2 deletions clang/unittests/Analysis/FlowSensitive/TestingSupport.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ const IndirectFieldDecl *findIndirectFieldDecl(ASTContext &ASTCtx,
/// Requirements:
///
/// `Name` must be unique in `ASTCtx`.
template <class LocT = StorageLocation>
template <class LocT>
LocT &getLocForDecl(ASTContext &ASTCtx, const Environment &Env,
llvm::StringRef Name) {
const ValueDecl *VD = findValueDecl(ASTCtx, Name);
Expand All @@ -470,7 +470,7 @@ LocT &getLocForDecl(ASTContext &ASTCtx, const Environment &Env,
/// Requirements:
///
/// `Name` must be unique in `ASTCtx`.
template <class ValueT = Value>
template <class ValueT>
ValueT &getValueForDecl(ASTContext &ASTCtx, const Environment &Env,
llvm::StringRef Name) {
const ValueDecl *VD = findValueDecl(ASTCtx, Name);
Expand Down
66 changes: 4 additions & 62 deletions clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5275,67 +5275,6 @@ TEST(TransferTest, BinaryOperatorComma) {
});
}

TEST(TransferTest, ConditionalOperatorValue) {
std::string Code = R"(
void target(bool Cond, bool B1, bool B2) {
bool JoinSame = Cond ? B1 : B1;
bool JoinDifferent = Cond ? B1 : B2;
// [[p]]
}
)";
runDataflow(
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();

auto &B1 = getValueForDecl<BoolValue>(ASTCtx, Env, "B1");
auto &B2 = getValueForDecl<BoolValue>(ASTCtx, Env, "B2");
auto &JoinSame = getValueForDecl<BoolValue>(ASTCtx, Env, "JoinSame");
auto &JoinDifferent =
getValueForDecl<BoolValue>(ASTCtx, Env, "JoinDifferent");

EXPECT_EQ(&JoinSame, &B1);

const Formula &JoinDifferentEqB1 =
Env.arena().makeEquals(JoinDifferent.formula(), B1.formula());
EXPECT_TRUE(Env.allows(JoinDifferentEqB1));
EXPECT_FALSE(Env.proves(JoinDifferentEqB1));

const Formula &JoinDifferentEqB2 =
Env.arena().makeEquals(JoinDifferent.formula(), B2.formula());
EXPECT_TRUE(Env.allows(JoinDifferentEqB2));
EXPECT_FALSE(Env.proves(JoinDifferentEqB1));
});
}

TEST(TransferTest, ConditionalOperatorLocation) {
std::string Code = R"(
void target(bool Cond, int I1, int I2) {
int &JoinSame = Cond ? I1 : I1;
int &JoinDifferent = Cond ? I1 : I2;
// [[p]]
}
)";
runDataflow(
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();

StorageLocation &I1 = getLocForDecl(ASTCtx, Env, "I1");
StorageLocation &I2 = getLocForDecl(ASTCtx, Env, "I2");
StorageLocation &JoinSame = getLocForDecl(ASTCtx, Env, "JoinSame");
StorageLocation &JoinDifferent =
getLocForDecl(ASTCtx, Env, "JoinDifferent");

EXPECT_EQ(&JoinSame, &I1);

EXPECT_NE(&JoinDifferent, &I1);
EXPECT_NE(&JoinDifferent, &I2);
});
}

TEST(TransferTest, IfStmtBranchExtendsFlowCondition) {
std::string Code = R"(
void target(bool Foo) {
Expand Down Expand Up @@ -5583,7 +5522,10 @@ TEST(TransferTest, ContextSensitiveReturnReferenceWithConditionalOperator) {
auto *Loc = Env.getReturnStorageLocation();
EXPECT_THAT(Loc, NotNull());

EXPECT_EQ(Loc, SLoc);
// TODO: We would really like to make this stronger assertion, but that
// doesn't work because we don't propagate values correctly through
// the conditional operator yet.
// EXPECT_EQ(Loc, SLoc);
},
{BuiltinOptions{ContextSensitiveOptions{}}});
}
Expand Down

0 comments on commit b045650

Please sign in to comment.