Reapply "[clang][dataflow] Model conditional operator correctly." with fixes #89596
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) Changes
Full diff: https://github.com/llvm/llvm-project/pull/89596.diff 7 Files Affected:
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..43fdfa5abcbb51 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,42 @@ 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) {
+ // 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;
+ }
+
+ 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 +836,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..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]]*/
@@ -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{}}});
}
|
martinboehme
changed the title
ymand
approved these changes
Apr 22, 2024
Xazax-hun
approved these changes
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I reverted #89213 beause it was causing buildbots to fail with assertion failures.
Embarrassingly, it turns out I had been running tests locally in
Releasemode, i.e. withassert()compiled away.This PR re-lands #89213 with fixes for the failing assertions.