[clang] [clang][dataflow] Model conditional operator correctly. (PR #89213)

[via cfe-commits]

================
@@ -657,17 +658,22 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
   }
 
   void VisitConditionalOperator(const ConditionalOperator *S) {
-    // FIXME: Revisit this once flow conditions are added to the framework. For
-    // `a = b ? c : d` we can add `b => a == c && !b => a == d` to the flow
-    // condition.
-    // When we do this, we will need to retrieve the values of the operands from
-    // the environments for the basic blocks they are computed in, in a similar
-    // way to how this is done for short-circuited logical operators in
-    // `getLogicOperatorSubExprValue()`.
-    if (S->isGLValue())
-      Env.setStorageLocation(*S, Env.createObject(S->getType()));
-    else if (!S->getType()->isRecordType()) {
-      if (Value *Val = Env.createValue(S->getType()))
+    const Environment *TrueEnv = StmtToEnv.getEnvironment(*S->getTrueExpr());
+    const Environment *FalseEnv = StmtToEnv.getEnvironment(*S->getFalseExpr());
+
+    if (TrueEnv == nullptr || FalseEnv == nullptr)
+      return;
+
+    if (S->isGLValue()) {
+      StorageLocation *TrueLoc = TrueEnv->getStorageLocation(*S->getTrueExpr());
+      StorageLocation *FalseLoc =
+          FalseEnv->getStorageLocation(*S->getFalseExpr());
+      if (TrueLoc == FalseLoc && TrueLoc != nullptr)
+        Env.setStorageLocation(*S, *TrueLoc);
+    } else if (!S->getType()->isRecordType()) {
+      if (Value *Val = Environment::joinValues(
----------------
martinboehme wrote:

I understand your reaction; generally, we want to perform joins in `computeBlockInputState()`. But the conditional operator is a special case.

Consider: In `computeBlockInputState()` (which calls through to `Environment::join()`, we join values that are associated with the _same_ expression or the same storage location, and then we associate the joined value with that same expression or storage location in the joined environment.

For the conditional operator, we want to join values that are associated with _different_ expressions (the two branches of the conditional operator), and then we associate the joined value with a third expression (the conditional operator itself). This join is what it means to perform transfer on the conditional operator.

Here's a simple example ([godbolt](https://godbolt.org/z/7ddnKo7Eh)) that hopefully clarifies this:

```cxx
int f(bool b, int i, int j) {
  return b ? i : j;
}
```

Here's the CFG:

```
 [B5 (ENTRY)]
   Succs (1): B4

 [B1]
   1: [B4.2] ? [B2.1] : [B3.1]
   2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   3: return [B1.2];
   Preds (2): B2 B3
   Succs (1): B0

 [B2]
   1: i
   Preds (1): B4
   Succs (1): B1

 [B3]
   1: j
   Preds (1): B4
   Succs (1): B1

 [B4]
   1: b
   2: [B4.1] (ImplicitCastExpr, LValueToRValue, _Bool)
   T: [B4.2] ? ... : ...
   Preds (1): B5
   Succs (2): B2 B3

 [B0 (EXIT)]
   Preds (1): B1
```

The expressions whose values we are joining are `i` ([B2.1]) and `j` ([B3.1]). The joined value is associated with the conditional operator ([B1.1]).

What would it look like if we wanted to do this join within `computeBlockInputState()`?

*  We would have to put code that is specific to `ConditionalOperator` in `computeBlockInputState()`. This would be incongruous, as `computeBlockInputState()` is otherwise completely general -- it doesn't contain any code that's specific to a particular statement kind.
*  We would be associating the joined value with the expression [B1.1] in the _input_ state of [B1], i.e. before we have started performing transfer on [B1]. This seems wrong: [B1.1] is an expression in [B1], and we should set its value when we transfer [B1], not before. (Put differently: If we put the logic for this in `computeBlockInputState()`, what would there be left for `TransferVisitor::VisitConditionalOperator()` to do?)

I hope this makes sense. If not, maybe it would be easiest to do a quick VC to discuss?

https://github.com/llvm/llvm-project/pull/89213