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

[martinboehme]

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.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

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/77750.diff

3 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+30-12)
• (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 faf83a8920d4ea..b2b1acd288bd9f 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -126,19 +126,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
@@ -489,6 +482,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 a60dbe1f61f6d6..c5594aa3fe9d1f 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 056c4f3383d832..6d88e25f77c807 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

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ymand]

CFGElement is cheap to construct. Can you just wrap the expression and pass it on?

[martinboehme]

Hm, that's an idea!

I'd still like to leave this to a future patch, as it's a further change in behavior. Note that the code we had so far also only ran the builtin transfer; this FIXME just documents this previously undocumented deficiency, so we're not regressing in any way.

I'd like to make this change separately so that we can separate out any unwanted side-effects of the two changes.

[Xazax-hun]

Once @ymand's comment is resolved, it looks good to me. Thanks!

[martinboehme]

Reverting: Causes build bots to fail because TerminatorVisitor::StmtToEnv is now unused.