Revert "[clang][dataflow] Process terminator condition within transferCFGBlock()."

[martinboehme]

Reverts #77750

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: None (martinboehme)

Changes

Reverts llvm/llvm-project#77750

---

Full diff: https://github.com/llvm/llvm-project/pull/77895.diff

3 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+12-30)
• (modified) clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp (-1)
• (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (-31)

diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 24d1447f09794a..e0a3552a9cde17 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -126,12 +126,19 @@ 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);
-    // 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);
+    // 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);
+    }
 
     bool ConditionValue = true;
     // The condition must be inverted for the successor that encompasses the
@@ -481,31 +488,6 @@ 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 c5594aa3fe9d1f..a60dbe1f61f6d6 100644
--- a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
@@ -123,7 +123,6 @@ 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 6d88e25f77c807..056c4f3383d832 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -6408,35 +6408,4 @@ 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