[clang][dataflow] Ensure well-formed flow conditions.

Ensure that the expressions associated with terminators are associated with a
value. Otherwise, we can generate degenerate flow conditions, where both
branches share the same condition.

Differential Revision: https://reviews.llvm.org/D123858

Author: ymand

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

@@ -32,6 +32,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
 namespace dataflow {
@@ -106,10 +107,24 @@ class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> {
     if (Env.getValue(Cond, SkipPast::None) == nullptr)
       transfer(StmtToEnv, Cond, Env);
 
+    // FIXME: The flow condition must be an r-value, so `SkipPast::None` should
+    // suffice.
     auto *Val =
         cast_or_null<BoolValue>(Env.getValue(Cond, SkipPast::Reference));
-    if (Val == nullptr)
-      return;
+    // 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) {
+      // FIXME: Consider introducing a helper for this get-or-create pattern.
+      auto *Loc = Env.getStorageLocation(Cond, SkipPast::None);
+      if (Loc == nullptr) {
+        Loc = &Env.createStorageLocation(Cond);
+        Env.setStorageLocation(Cond, *Loc);
+      }
+      Val = &Env.makeAtomicBoolValue();
+      Env.setValue(*Loc, *Val);
+    }
 
     // The condition must be inverted for the successor that encompasses the
     // "else" branch, if such exists.

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

@@ -878,4 +878,119 @@ TEST_F(FlowConditionTest, Join) {
       });
 }
 
+// Verifies that flow conditions are properly constructed even when the
+// condition is not meaningfully interpreted.
+//
+// Note: currently, arbitrary function calls are uninterpreted, so the test
+// exercises this case. If and when we change that, this test will not add to
+// coverage (although it may still test a valuable case).
+TEST_F(FlowConditionTest, OpaqueFlowConditionMergesToOpaqueBool) {
+  std::string Code = R"(
+    bool foo();
+
+    void target() {
+      bool Bar = true;
+      if (foo())
+        Bar = false;
+      (void)0;
+      /*[[p]]*/
+    }
+  )";
+  runDataflow(
+      Code, [](llvm::ArrayRef<
+                   std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                   Results,
+               ASTContext &ASTCtx) {
+        ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+        const Environment &Env = Results[0].second.Env;
+
+        const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+        ASSERT_THAT(BarDecl, NotNull());
+
+        auto &BarVal =
+            *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference));
+
+        EXPECT_FALSE(Env.flowConditionImplies(BarVal));
+      });
+}
+
+// Verifies that flow conditions are properly constructed even when the
+// condition is not meaningfully interpreted.
+//
+// Note: currently, fields with recursive type calls are uninterpreted (beneath
+// the first instance), so the test exercises this case. If and when we change
+// that, this test will not add to coverage (although it may still test a
+// valuable case).
+TEST_F(FlowConditionTest, OpaqueFieldFlowConditionMergesToOpaqueBool) {
+  std::string Code = R"(
+    struct Rec {
+      Rec* Next;
+    };
+
+    struct Foo {
+      Rec* X;
+    };
+
+    void target(Foo F) {
+      bool Bar = true;
+      if (F.X->Next)
+        Bar = false;
+      (void)0;
+      /*[[p]]*/
+    }
+  )";
+  runDataflow(
+      Code, [](llvm::ArrayRef<
+                   std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                   Results,
+               ASTContext &ASTCtx) {
+        ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+        const Environment &Env = Results[0].second.Env;
+
+        const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+        ASSERT_THAT(BarDecl, NotNull());
+
+        auto &BarVal =
+            *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference));
+
+        EXPECT_FALSE(Env.flowConditionImplies(BarVal));
+      });
+}
+
+// Verifies that flow conditions are properly constructed even when the
+// condition is not meaningfully interpreted. Adds to above by nesting the
+// interestnig case inside a normal branch. This protects against degenerate
+// solutions which only test for empty flow conditions, for example.
+TEST_F(FlowConditionTest, OpaqueFlowConditionInsideBranchMergesToOpaqueBool) {
+  std::string Code = R"(
+    bool foo();
+
+    void target(bool Cond) {
+      bool Bar = true;
+      if (Cond) {
+        if (foo())
+          Bar = false;
+        (void)0;
+        /*[[p]]*/
+      }
+    }
+  )";
+  runDataflow(
+      Code, [](llvm::ArrayRef<
+                   std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                   Results,
+               ASTContext &ASTCtx) {
+        ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+        const Environment &Env = Results[0].second.Env;
+
+        const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+        ASSERT_THAT(BarDecl, NotNull());
+
+        auto &BarVal =
+            *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference));
+
+        EXPECT_FALSE(Env.flowConditionImplies(BarVal));
+      });
+}
+
 } // namespace