https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/89596
- **Reapply "[clang][dataflow] Model conditional operator correctly." (#89577)**
- **Fix failing tests for `TransferVisitor::VisitConditionalOperator().`**
>From db32339492774cad8a6ceeb86ca8e13e6fef8c2a Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboe...@google.com>
Date: Mon, 22 Apr 2024 08:53:48 +0000
Subject: [PATCH 1/2] Reapply "[clang][dataflow] Model conditional operator
correctly." (#89577)
This reverts commit 8ff6434546bcb4602bd079f4161f746956905c60.
---
.../FlowSensitive/DataflowEnvironment.h | 15 +++++
.../clang/Analysis/FlowSensitive/Transfer.h | 3 +-
.../FlowSensitive/DataflowEnvironment.cpp | 46 ++++++-------
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 57 +++++++++++-----
.../TypeErasedDataflowAnalysis.cpp | 4 +-
.../Analysis/FlowSensitive/TestingSupport.h | 4 +-
.../Analysis/FlowSensitive/TransferTest.cpp | 66 +++++++++++++++++--
7 files changed, 149 insertions(+), 46 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index d50dba35f8264c..cdf89c7def2c91 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -244,6 +244,21 @@ 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.
///
diff --git a/clang/include/clang/Analysis/FlowSensitive/Transfer.h
b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
index ed148250d8eb29..940025e02100f9 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Transfer.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
@@ -53,7 +53,8 @@ class StmtToEnvMap {
/// Requirements:
///
/// `S` must not be `ParenExpr` or `ExprWithCleanups`.
-void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);
+void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env,
+ Environment::ValueModel &Model);
} // namespace dataflow
} // namespace clang
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 05395e07a7a68c..3cb656adcbdc0c 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -237,13 +237,8 @@ joinLocToVal(const llvm::MapVector<const StorageLocation
*, Value *> &LocToVal,
continue;
assert(It->second != nullptr);
- if (areEquivalentValues(*Val, *It->second)) {
- Result.insert({Loc, Val});
- continue;
- }
-
- if (Value *JoinedVal = joinDistinctValues(
- Loc->getType(), *Val, Env1, *It->second, Env2, JoinedEnv, Model)) {
+ if (Value *JoinedVal = Environment::joinValues(
+ Loc->getType(), Val, Env1, It->second, Env2, JoinedEnv, Model)) {
Result.insert({Loc, JoinedVal});
}
}
@@ -775,27 +770,16 @@ Environment Environment::join(const Environment &EnvA,
const Environment &EnvB,
JoinedEnv.LocForRecordReturnVal = EnvA.LocForRecordReturnVal;
JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc;
- 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`.
+ if (EnvA.CallStack.empty()) {
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);
- if (Value *JoinedVal =
- joinDistinctValues(Func->getReturnType(), *EnvA.ReturnVal, EnvA,
- *EnvB.ReturnVal, EnvB, JoinedEnv, Model))
- JoinedEnv.ReturnVal = JoinedVal;
+ JoinedEnv.ReturnVal =
+ joinValues(Func->getReturnType(), EnvA.ReturnVal, EnvA, EnvB.ReturnVal,
+ EnvB, JoinedEnv, Model);
}
if (EnvA.ReturnLoc == EnvB.ReturnLoc)
@@ -821,6 +805,24 @@ 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);
}
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 2771c8b2e37ebb..f56fd64296cc45 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -124,8 +124,9 @@ namespace {
class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
public:
- TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env)
- : StmtToEnv(StmtToEnv), Env(Env) {}
+ TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
+ Environment::ValueModel &Model)
+ : StmtToEnv(StmtToEnv), Env(Env), Model(Model) {}
void VisitBinaryOperator(const BinaryOperator *S) {
const Expr *LHS = S->getLHS();
@@ -641,17 +642,41 @@ class TransferVisitor : public
ConstStmtVisitor<TransferVisitor> {
}
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.
- // 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()))
+ 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))
Env.setValue(*S, *Val);
}
}
@@ -810,12 +835,14 @@ class TransferVisitor : public
ConstStmtVisitor<TransferVisitor> {
const StmtToEnvMap &StmtToEnv;
Environment &Env;
+ Environment::ValueModel &Model;
};
} // namespace
-void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
- TransferVisitor(StmtToEnv, Env).Visit(&S);
+void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env,
+ Environment::ValueModel &Model) {
+ TransferVisitor(StmtToEnv, Env, Model).Visit(&S);
}
} // namespace dataflow
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 71d5c1a6c4f4a3..12eff4dd4b781d 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -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);
+ InputState.Env, AC.Analysis);
}
/// Built-in transfer function for `CFGInitializer`.
@@ -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);
+ *TerminatorCond, State.Env, AC.Analysis);
// 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
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
index e3c7ff685f5724..3b0e05ed72220e 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -456,7 +456,7 @@ const IndirectFieldDecl *findIndirectFieldDecl(ASTContext
&ASTCtx,
/// Requirements:
///
/// `Name` must be unique in `ASTCtx`.
-template <class LocT>
+template <class LocT = StorageLocation>
LocT &getLocForDecl(ASTContext &ASTCtx, const Environment &Env,
llvm::StringRef Name) {
const ValueDecl *VD = findValueDecl(ASTCtx, Name);
@@ -470,7 +470,7 @@ LocT &getLocForDecl(ASTContext &ASTCtx, const Environment
&Env,
/// Requirements:
///
/// `Name` must be unique in `ASTCtx`.
-template <class ValueT>
+template <class ValueT = Value>
ValueT &getValueForDecl(ASTContext &ASTCtx, const Environment &Env,
llvm::StringRef Name) {
const ValueDecl *VD = findValueDecl(ASTCtx, Name);
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index bb16138126c8f9..3a62bc0c02080f 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5275,6 +5275,67 @@ 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) {
@@ -5522,10 +5583,7 @@ TEST(TransferTest,
ContextSensitiveReturnReferenceWithConditionalOperator) {
auto *Loc = Env.getReturnStorageLocation();
EXPECT_THAT(Loc, NotNull());
- // 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);
+ EXPECT_EQ(Loc, SLoc);
},
{BuiltinOptions{ContextSensitiveOptions{}}});
}
>From 0be8e945e861137c40951350a158dc9a7ba8a338 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboe...@google.com>
Date: Mon, 22 Apr 2024 10:43:13 +0000
Subject: [PATCH 2/2] Fix failing tests for
`TransferVisitor::VisitConditionalOperator().`
- It turns out that thare _are_ legitimate cases where the environment
for one of the branches of the operator can be null, namely when that
branch is dead.
- Fixed `VarDeclInitAssignConditionalOperator` test so that copy
construction happens inside the conditional operator, rather than
outside; in the latter case, we don't produce a value for the field
any more.
---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 7 ++++---
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index f56fd64296cc45..43fdfa5abcbb51 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -646,9 +646,10 @@ class TransferVisitor : public
ConstStmtVisitor<TransferVisitor> {
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);
+ // If the true or false branch is dead, we may not have an environment
for
+ // it. We could handle this specifically by forwarding the value or
+ // location of the live branch, but this case is rare enough that this
+ // probably isn't worth the additional complexity.
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 3a62bc0c02080f..215e208615ac23 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3637,7 +3637,7 @@ TEST(TransferTest, VarDeclInitAssignConditionalOperator) {
};
void target(A Foo, A Bar, bool Cond) {
- A Baz = Cond ? Foo : Bar;
+ A Baz = Cond ? A(Foo) : A(Bar);
// Make sure A::i is modeled.
Baz.i;
/*[[p]]*/
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
