llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) <details> <summary>Changes</summary> In particular, it's important that we create the "fallback" atomic at this point (which we produce if the transfer function didn't produce a value for the expression) so that it is placed in the correct environment. Previously, we processed the terminator condition in the `TerminatorVisitor`, which put the fallback atomic in a copy of the environment that is produced as input for the _successor_ block, rather than the environment for the block containing the expression for which we produce the fallback atomic. As a result, we produce different fallback atomics every time we process the successor block, and hence we don't have a consistent representation of the terminator condition in the flow condition. This patch includes a test (authored by ymand@) that fails without the fix. --- Full diff: https://github.com/llvm/llvm-project/pull/78127.diff 3 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+33-19) - (modified) clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp (+1) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+31) ``````````diff diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index faf83a8920d4ead..0b5371f1c601a43 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -75,9 +75,8 @@ using TerminatorVisitorRetTy = std::pair<const Expr *, bool>; class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor, TerminatorVisitorRetTy> { public: - TerminatorVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env, - int BlockSuccIdx) - : StmtToEnv(StmtToEnv), Env(Env), BlockSuccIdx(BlockSuccIdx) {} + TerminatorVisitor(Environment &Env, int BlockSuccIdx) + : Env(Env), BlockSuccIdx(BlockSuccIdx) {} TerminatorVisitorRetTy VisitIfStmt(const IfStmt *S) { auto *Cond = S->getCond(); @@ -126,19 +125,12 @@ class TerminatorVisitor private: TerminatorVisitorRetTy extendFlowCondition(const Expr &Cond) { - // The terminator sub-expression might not be evaluated. - if (Env.getValue(Cond) == nullptr) - transfer(StmtToEnv, Cond, Env); - auto *Val = Env.get<BoolValue>(Cond); - // Value merging depends on flow conditions from different environments - // being mutually exclusive -- that is, they cannot both be true in their - // entirety (even if they may share some clauses). So, we need *some* value - // for the condition expression, even if just an atom. - if (Val == nullptr) { - Val = &Env.makeAtomicBoolValue(); - Env.setValue(Cond, *Val); - } + // In transferCFGBlock(), we ensure that we always have a `Value` for the + // terminator condition, so assert this. + // We consciously assert ourselves instead of asserting via `cast()` so + // that we get a more meaningful line number if the assertion fails. + assert(Val != nullptr); bool ConditionValue = true; // The condition must be inverted for the successor that encompasses the @@ -152,7 +144,6 @@ class TerminatorVisitor return {&Cond, ConditionValue}; } - const StmtToEnvMap &StmtToEnv; Environment &Env; int BlockSuccIdx; }; @@ -335,10 +326,8 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) { // when the terminator is taken. Copy now. TypeErasedDataflowAnalysisState Copy = MaybePredState->fork(); - const StmtToEnvMap StmtToEnv(AC.CFCtx, AC.BlockStates); auto [Cond, CondValue] = - TerminatorVisitor(StmtToEnv, Copy.Env, - blockIndexInPredecessor(*Pred, Block)) + TerminatorVisitor(Copy.Env, blockIndexInPredecessor(*Pred, Block)) .Visit(PredTerminatorStmt); if (Cond != nullptr) // FIXME: Call transferBranchTypeErased even if BuiltinTransferOpts @@ -489,6 +478,31 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext &AC, } AC.Log.recordState(State); } + + // If we have a terminator, evaluate its condition. + // This `Expr` may not appear as a `CFGElement` anywhere else, and it's + // important that we evaluate it here (rather than while processing the + // terminator) so that we put the corresponding value in the right + // environment. + if (const Expr *TerminatorCond = + dyn_cast_or_null<Expr>(Block.getTerminatorCondition())) { + if (State.Env.getValue(*TerminatorCond) == nullptr) + // FIXME: This only runs the builtin transfer, not the analysis-specific + // transfer. Fixing this isn't trivial, as the analysis-specific transfer + // takes a `CFGElement` as input, but some expressions only show up as a + // terminator condition, but not as a `CFGElement`. The condition of an if + // statement is one such example. + transfer(StmtToEnvMap(AC.CFCtx, AC.BlockStates), *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 + // when we extend the flow condition, it actually changes. + if (State.Env.getValue(*TerminatorCond) == nullptr) + State.Env.setValue(*TerminatorCond, State.Env.makeAtomicBoolValue()); + AC.Log.recordState(State); + } + return State; } diff --git a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp index a60dbe1f61f6d6e..c5594aa3fe9d1fe 100644 --- a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp @@ -123,6 +123,7 @@ recordState(Elements=1, Branches=0, Joins=0) enterElement(b (ImplicitCastExpr, LValueToRValue, _Bool)) transfer() recordState(Elements=2, Branches=0, Joins=0) +recordState(Elements=2, Branches=0, Joins=0) enterBlock(3, false) transferBranch(0) diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 056c4f3383d8322..6d88e25f77c8075 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -6408,4 +6408,35 @@ TEST(TransferTest, DifferentReferenceLocInJoin) { }); } +// This test verifies correct modeling of a relational dependency that goes +// through unmodeled functions (the simple `cond()` in this case). +TEST(TransferTest, ConditionalRelation) { + std::string Code = R"( + bool cond(); + void target() { + bool a = true; + bool b = true; + if (cond()) { + a = false; + if (cond()) { + b = false; + } + } + (void)0; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + auto &A = Env.arena(); + auto &VarA = getValueForDecl<BoolValue>(ASTCtx, Env, "a").formula(); + auto &VarB = getValueForDecl<BoolValue>(ASTCtx, Env, "b").formula(); + + EXPECT_FALSE(Env.allows(A.makeAnd(VarA, A.makeNot(VarB)))); + }); +} + } // namespace `````````` </details> https://github.com/llvm/llvm-project/pull/78127 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
