Revert "[clang][dataflow] Model conditional operator correctly." #89577
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
…)" This reverts commit abb958f.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) ChangesReverts llvm/llvm-project#89213 This is causing buildbot failures. Full diff: https://github.com/llvm/llvm-project/pull/89577.diff 7 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index cdf89c7def2c91..d50dba35f8264c 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -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.
///
diff --git a/clang/include/clang/Analysis/FlowSensitive/Transfer.h b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
index 940025e02100f9..ed148250d8eb29 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Transfer.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
@@ -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
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 3cb656adcbdc0c..05395e07a7a68c 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -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});
}
}
@@ -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)
@@ -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);
}
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index f56fd64296cc45..2771c8b2e37ebb 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -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();
@@ -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);
}
}
@@ -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
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 12eff4dd4b781d..71d5c1a6c4f4a3 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, AC.Analysis);
+ InputState.Env);
}
/// 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, 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
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
index 3b0e05ed72220e..e3c7ff685f5724 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 = StorageLocation>
+template <class LocT>
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 = Value>
+template <class ValueT>
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 3a62bc0c02080f..bb16138126c8f9 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -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) {
@@ -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{}}});
}
|
|
Postmortem reveals that I inadvertently had my build configured with |
martinboehme
added a commit
to martinboehme/llvm-project
that referenced
this pull request
Apr 22, 2024
This was referenced 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.
Reverts #89213
This is causing buildbot failures.