[clang][dataflow] Handle more glvalue cases of the ConditionalOperator transfer (#168994)

In the dataflow framework, the builtin transfer function currently only
handles the GLValue result case of ConditionalOperator when the
true and false expression StorageLocations are exactly the same.

Ideally / we have wanted to introduce alias sets to handle when the Locs
are different. However, that is a larger change to the framework
(and we may need to introduce weak updates).

For now, do something simpler to at least handle when the GLValue is
immediately cast to an RValue, by making up a distinct StorageLocation
that holds the join of the true and false expression values (when not a
record). This seems like the most common case, so seems worth covering.
The case when an LValue is needed and can be updated later (and
thus needs a link to the original storage locations) seems more rare,
and we currently do not handle such updates either, so this intermediate
step is no different (for that case).

Author: jvoung

clang/lib/Analysis/FlowSensitive/Transfer.cpp

@@ -769,8 +769,29 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       StorageLocation *TrueLoc = TrueEnv->getStorageLocation(*S->getTrueExpr());
       StorageLocation *FalseLoc =
           FalseEnv->getStorageLocation(*S->getFalseExpr());
-      if (TrueLoc == FalseLoc && TrueLoc != nullptr)
+      if (TrueLoc == FalseLoc && TrueLoc != nullptr) {
         Env.setStorageLocation(*S, *TrueLoc);
+      } else if (!S->getType()->isRecordType()) {
+        // Ideally, we would have something like an "alias set" to say that the
+        // result StorageLocation can be either of the locations from the
+        // TrueEnv or FalseEnv. Then, when this ConditionalOperator is
+        // (a) used in an LValueToRValue cast, the value is the join of all of
+        //     the values in the alias set.
+        // (b) or, used in an assignment to the resulting LValue, the assignment
+        //     *may* update all of the locations in the alias set.
+        // For now, we do the simpler thing of creating a new StorageLocation
+        // and joining the values right away, handling only case (a).
+        // Otherwise, the dataflow framework needs to be updated be able to
+        // represent alias sets and weak updates (for the "may").
+        if (Value *Val = Environment::joinValues(
+                S->getType(), TrueEnv->getValue(*S->getTrueExpr()), *TrueEnv,
+                FalseEnv->getValue(*S->getFalseExpr()), *FalseEnv, Env,
+                Model)) {
+          StorageLocation &Loc = Env.createStorageLocation(*S);
+          Env.setStorageLocation(*S, Loc);
+          Env.setValue(Loc, *Val);
+        }
+      }
     } 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

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

@@ -17,6 +17,7 @@
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/Formula.h"
 #include "clang/Analysis/FlowSensitive/NoopAnalysis.h"
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
@@ -4382,6 +4383,40 @@ TEST(TransferTest, VarDeclInitAssignConditionalOperator) {
       });
 }
 
+TEST(TransferTest, VarDeclInitReferenceAssignConditionalOperator) {
+  std::string Code = R"(
+    struct A {
+      int i;
+    };
+
+    void target(A Foo, A Bar, bool Cond) {
+      A &Baz = Cond ? Foo : Bar;
+      // Make sure A::i is modeled.
+      Baz.i;
+      /*[[p]]*/
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto *FooIVal = cast<IntegerValue>(getFieldValue(
+            &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Foo"), "i",
+            ASTCtx, Env));
+        auto *BarIVal = cast<IntegerValue>(getFieldValue(
+            &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Bar"), "i",
+            ASTCtx, Env));
+        auto *BazIVal = cast<IntegerValue>(getFieldValue(
+            &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Baz"), "i",
+            ASTCtx, Env));
+
+        EXPECT_NE(BazIVal, FooIVal);
+        EXPECT_NE(BazIVal, BarIVal);
+      });
+}
+
 TEST(TransferTest, VarDeclInDoWhile) {
   std::string Code = R"(
     void target(int *Foo) {
@@ -6150,6 +6185,45 @@ TEST(TransferTest, ConditionalOperatorValue) {
       });
 }
 
+TEST(TransferTest, ConditionalOperatorValuesTested) {
+  // We should be able to show that the result of the conditional operator,
+  // JoinResultMustBeB1, must be equal to B1, because the condition is checking
+  // `B1 == B2` and selecting B1 on the false branch, or B2 on the true branch.
+  // Similarly, for JoinResultMustBeB2 == B2.
+  // Note that the conditional operator involves a join of two *different*
+  // glvalues, before casting the lvalue to an rvalue, which may affect the
+  // implementation of the transfer function, and thus affect whether or not we
+  // can prove that IsB1 == B1.
+  std::string Code = R"(
+    void target(bool B1, bool B2) {
+      bool JoinResultMustBeB1 = (B1 == B2) ? B2 : B1;
+      bool JoinResultMustBeB2 = (B1 == B2) ? 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 &JoinResultMustBeB1 =
+            getValueForDecl<BoolValue>(ASTCtx, Env, "JoinResultMustBeB1");
+        auto &JoinResultMustBeB2 =
+            getValueForDecl<BoolValue>(ASTCtx, Env, "JoinResultMustBeB2");
+
+        const Formula &MustBeB1_Eq_B1 =
+            Env.arena().makeEquals(JoinResultMustBeB1.formula(), B1.formula());
+        EXPECT_TRUE(Env.proves(MustBeB1_Eq_B1));
+
+        const Formula &MustBeB2_Eq_B2 =
+            Env.arena().makeEquals(JoinResultMustBeB2.formula(), B2.formula());
+        EXPECT_TRUE(Env.proves(MustBeB2_Eq_B2));
+      });
+}
+
 TEST(TransferTest, ConditionalOperatorLocation) {
   std::string Code = R"(
     void target(bool Cond, int I1, int I2) {
@@ -6177,6 +6251,66 @@ TEST(TransferTest, ConditionalOperatorLocation) {
       });
 }
 
+TEST(TransferTest, ConditionalOperatorLocationUpdatedAfter) {
+  // We don't currently model a Conditional Operator with an LValue result
+  // as having aliases to the LHS and RHS (if it isn't just the same LValue
+  // on both sides). We also don't model that the update "may" happen
+  // (a weak update). So, we don't consider the LHS and RHS as being weakly
+  // updated at [[after_diff]].
+  std::string Code = R"(
+    void target(bool Cond, bool B1, bool B2) {
+      (void)0;
+      // [[before_same]]
+      (Cond ? B1 : B1) = !B1;
+      // [[after_same]]
+      (Cond ? B1 : B2) = !B1;
+      // [[after_diff]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        Environment BeforeSameEnv =
+            getEnvironmentAtAnnotation(Results, "before_same").fork();
+        Environment AfterSameEnv =
+            getEnvironmentAtAnnotation(Results, "after_same").fork();
+        Environment AfterDiffEnv =
+            getEnvironmentAtAnnotation(Results, "after_diff").fork();
+
+        auto &BeforeSameB1 =
+            getValueForDecl<BoolValue>(ASTCtx, BeforeSameEnv, "B1");
+        auto &AfterSameB1 =
+            getValueForDecl<BoolValue>(ASTCtx, AfterSameEnv, "B1");
+        auto &AfterDiffB1 =
+            getValueForDecl<BoolValue>(ASTCtx, AfterDiffEnv, "B1");
+
+        EXPECT_NE(&BeforeSameB1, &AfterSameB1);
+        EXPECT_NE(&BeforeSameB1, &AfterDiffB1);
+        // FIXME: The formula for AfterSameB1 should be different from
+        // AfterDiffB1 to reflect that B1 may be updated.
+        EXPECT_EQ(&AfterSameB1, &AfterDiffB1);
+
+        // The value of B1 is definitely different from before_same vs
+        // after_same.
+        const Formula &B1ChangedForSame =
+            AfterSameEnv.arena().makeNot(AfterSameEnv.arena().makeEquals(
+                AfterSameB1.formula(), BeforeSameB1.formula()));
+        EXPECT_TRUE(AfterSameEnv.allows(B1ChangedForSame));
+        EXPECT_TRUE(AfterSameEnv.proves(B1ChangedForSame));
+
+        const Formula &B1ChangedForDiff =
+            AfterDiffEnv.arena().makeNot(AfterDiffEnv.arena().makeEquals(
+                AfterDiffB1.formula(), AfterSameB1.formula()));
+        // FIXME: It should be possible that B1 *may* be updated, so it may be
+        // that AfterSameB1 != AfterDiffB1 or AfterSameB1 == AfterDiffB1.
+        EXPECT_FALSE(AfterSameEnv.allows(B1ChangedForDiff));
+        // proves() should be false, since B1 may or may not have changed
+        // depending on `Cond`.
+        EXPECT_FALSE(AfterSameEnv.proves(B1ChangedForDiff));
+      });
+}
+
 TEST(TransferTest, ConditionalOperatorOnConstantExpr) {
   // This is a regression test: We used to crash when a `ConstantExpr` was used
   // in the branches of a conditional operator.