https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/89577
Reverts llvm/llvm-project#89213
This is causing buildbot failures.
>From b0456501b10a3a83e6e6818a050f3fd691972d79 Mon Sep 17 00:00:00 2001
From: martinboehme <mboe...@google.com>
Date: Mon, 22 Apr 2024 09:35:06 +0200
Subject: [PATCH] Revert "[clang][dataflow] Model conditional operator
correctly. (#89213)"
This reverts commit abb958f1610becc0a753ad8f4308a90f85e1338f.
---
.../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, 46 insertions(+), 149 deletions(-)
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{}}});
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
