[clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.

[mboehme]

Summary

This patch adds a test that crashes without the fix.

[xazax.hun]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5148

Should we also test that the value of the expression is true in the analysis state?

[mboehme]

Add check that the value of the expression is true.

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

5148

Good point -- done.

I've also verified that this test continues to trigger the assert-fail if the fix is not present.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

case BO_Assign: {
       Env.setStorageLocation(*S, *LHSLoc);
       break;
     }
     case BO_LAnd:
     case BO_LOr: {
       auto &Loc = Env.createStorageLocation(*S);
       Env.setStorageLocation(*S, Loc);
 
-      BoolValue *LHSVal = getLogicOperatorSubExprValue(*LHS);
-      // If the LHS was not reachable, this BinaryOperator would also not be
-      // reachable, and we would never get here.
-      assert(LHSVal != nullptr);
-      BoolValue *RHSVal = getLogicOperatorSubExprValue(*RHS);
-      if (RHSVal == nullptr) {
-        // If the RHS isn't reachable and we evaluate this BinaryOperator,
-        // then the value of the LHS must have triggered the short-circuit
-        // logic. This implies that the value of the entire expression must be
-        // equal to the value of the LHS.
-        Env.setValue(Loc, *LHSVal);
-        break;
-      }
+      BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);
+      BoolValue &RHSVal = getLogicOperatorSubExprValue(*RHS);
 
       if (S->getOpcode() == BO_LAnd)
-        Env.setValue(Loc, Env.makeAnd(*LHSVal, *RHSVal));
+        Env.setValue(Loc, Env.makeAnd(LHSVal, RHSVal));
       else
-        Env.setValue(Loc, Env.makeOr(*LHSVal, *RHSVal));
+        Env.setValue(Loc, Env.makeOr(LHSVal, RHSVal));
       break;
     }
     case BO_NE:
     case BO_EQ: {
       auto &LHSEqRHSValue = evaluateBooleanEquality(*LHS, *RHS, Env);
       auto &Loc = Env.createStorageLocation(*S);
       Env.setStorageLocation(*S, Loc);
       Env.setValue(Loc, S->getOpcode() == BO_EQ ? LHSEqRHSValue
void VisitExprWithCleanups(const ExprWithCleanups *S) {
     // basic blocks, however manual traversal to sub-expressions may encounter
     // them. Redirect to the sub-expression.
     auto *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
     Visit(SubExpr);
   }
 
 private:
-  /// If `SubExpr` is reachable, returns a non-null pointer to the value for
-  /// `SubExpr`. If `SubExpr` is not reachable, returns nullptr.
-  BoolValue *getLogicOperatorSubExprValue(const Expr &SubExpr) {
+  /// Returns the value for the sub-expression `SubExpr` of a logic operator.
+  BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
     // `SubExpr` and its parent logic operator might be part of different basic
     // blocks. We try to access the value that is assigned to `SubExpr` in the
     // corresponding environment.
-    const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr);
-    if (!SubExprEnv)
-      return nullptr;
-
+    if (const Environment *SubExprEnv = StmtToEnv.getEnvironment(SubExpr))
       if (auto *Val =
               dyn_cast_or_null<BoolValue>(SubExprEnv->getValueStrict(SubExpr)))
-      return Val;
+        return *Val;
 
-    if (Env.getValueStrict(SubExpr) == nullptr) {
-      // Sub-expressions that are logic operators are not added in basic blocks
-      // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
-      // operator, it may not have been evaluated and assigned a value yet. In
-      // that case, we need to first visit `SubExpr` and then try to get the
-      // value that gets assigned to it.
+    // The sub-expression may lie within a basic block that isn't reachable,
+    // even if we need it to evaluate the current (reachable) expression
+    // (see https://discourse.llvm.org/t/70775). In this case, visit `SubExpr`
+    // within the current environment and then try to get the value that gets
+    // assigned to it.
+    if (Env.getValueStrict(SubExpr) == nullptr)
       Visit(&SubExpr);
-    }
-
     if (auto *Val = dyn_cast_or_null<BoolValue>(Env.getValueStrict(SubExpr)))
-      return Val;
+      return *Val;
 
     // If the value of `SubExpr` is still unknown, we create a fresh symbolic
     // boolean value for it.
-    return &Env.makeAtomicBoolValue();
+    return Env.makeAtomicBoolValue();
   }
 
   // If context sensitivity is enabled, try to analyze the body of the callee
   // `F` of `S`. The type `E` must be either `CallExpr` or `CXXConstructExpr`.
   template <typename E>
   void transferInlineCall(const E *S, const FunctionDecl *F) {
     const auto &Options = Env.getDataflowAnalysisContext().getOptions();
     if (!(Options.ContextSensitiveOpts &&

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         // This doesn't need a body because this test was crashing the framework
         // before handling correctly Unnamed bitfields in `InitListExpr`.
       });
 }
 
+// Repro for a crash that used to occur with chained short-circuiting logical
+// operators.
+TEST(TransferTest, ChainedLogicalOps) {
+  std::string Code = R"(
+    bool target() {
+      bool b = true || false || false || false;
+      // [[p]]
+      return b;
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        auto &B = getValueForDecl<BoolValue>(ASTCtx, Env, "b");
+        EXPECT_TRUE(Env.flowConditionImplies(B));
+      });
+}
+
 // Repro for a crash that used to occur when we call a `noreturn` function
 // within one of the operands of a `&&` or `||` operator.
 TEST(TransferTest, NoReturnFunctionInsideShortCircuitedBooleanOp) {
   std::string Code = R"(
     __attribute__((noreturn)) int doesnt_return();
     bool some_condition();
     void target(bool b1, bool b2) {
       // Neither of these should crash. In addition, if we don't terminate the