Revert "[analyzer] Don't assume third iteration in loops (llvm#119388)"

This reverts commit bb27d5e.

DO NOT MERGE, this is only for comparison.

Author: NagyDonat

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h

@@ -126,14 +126,6 @@ class CoreEngine {
   ExplodedNode *generateCallExitBeginNode(ExplodedNode *N,
                                           const ReturnStmt *RS);
 
-  /// Helper function called by `HandleBranch()`. If the currently handled
-  /// branch corresponds to a loop, this returns the number of already
-  /// completed iterations in that loop, otherwise the return value is
-  /// `std::nullopt`. Note that this counts _all_ earlier iterations, including
-  /// ones that were performed within an earlier iteration of an outer loop.
-  std::optional<unsigned> getCompletedIterationCount(const CFGBlock *B,
-                                                     ExplodedNode *Pred) const;
-
 public:
   /// Construct a CoreEngine object to analyze the provided CFG.
   CoreEngine(ExprEngine &exprengine,

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

@@ -321,14 +321,14 @@ class ExprEngine {
                                NodeBuilderWithSinks &nodeBuilder,
                                ExplodedNode *Pred);
 
-  /// ProcessBranch - Called by CoreEngine. Used to generate successor nodes by
-  /// processing the 'effects' of a branch condition. If the branch condition
-  /// is a loop condition, IterationsCompletedInLoop is the number of completed
-  /// iterations (otherwise it's std::nullopt).
-  void processBranch(const Stmt *Condition, NodeBuilderContext &BuilderCtx,
-                     ExplodedNode *Pred, ExplodedNodeSet &Dst,
-                     const CFGBlock *DstT, const CFGBlock *DstF,
-                     std::optional<unsigned> IterationsCompletedInLoop);
+  /// ProcessBranch - Called by CoreEngine.  Used to generate successor
+  ///  nodes by processing the 'effects' of a branch condition.
+  void processBranch(const Stmt *Condition,
+                     NodeBuilderContext& BuilderCtx,
+                     ExplodedNode *Pred,
+                     ExplodedNodeSet &Dst,
+                     const CFGBlock *DstT,
+                     const CFGBlock *DstF);
 
   /// Called by CoreEngine.
   /// Used to generate successor nodes for temporary destructors depending
@@ -592,8 +592,6 @@ class ExprEngine {
   void evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst, ExplodedNodeSet &Src,
                                     const Expr *Ex);
 
-  bool didEagerlyAssumeBifurcateAt(ProgramStateRef State, const Expr *Ex) const;
-
   static std::pair<const ProgramPointTag *, const ProgramPointTag *>
   getEagerlyAssumeBifurcationTags();
 

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

@@ -477,8 +477,7 @@ void CoreEngine::HandleBranch(const Stmt *Cond, const Stmt *Term,
   NodeBuilderContext Ctx(*this, B, Pred);
   ExplodedNodeSet Dst;
   ExprEng.processBranch(Cond, Ctx, Pred, Dst, *(B->succ_begin()),
-                        *(B->succ_begin() + 1),
-                        getCompletedIterationCount(B, Pred));
+                       *(B->succ_begin() + 1));
   // Enqueue the new frontier onto the worklist.
   enqueue(Dst);
 }
@@ -625,30 +624,6 @@ ExplodedNode *CoreEngine::generateCallExitBeginNode(ExplodedNode *N,
   return isNew ? Node : nullptr;
 }
 
-std::optional<unsigned>
-CoreEngine::getCompletedIterationCount(const CFGBlock *B,
-                                       ExplodedNode *Pred) const {
-  const LocationContext *LC = Pred->getLocationContext();
-  BlockCounter Counter = WList->getBlockCounter();
-  unsigned BlockCount =
-      Counter.getNumVisited(LC->getStackFrame(), B->getBlockID());
-
-  const Stmt *Term = B->getTerminatorStmt();
-  if (isa<ForStmt, WhileStmt, CXXForRangeStmt>(Term)) {
-    assert(BlockCount >= 1 &&
-           "Block count of currently analyzed block must be >= 1");
-    return BlockCount - 1;
-  }
-  if (isa<DoStmt>(Term)) {
-    // In a do-while loop one iteration happens before the first evaluation of
-    // the loop condition, so we don't subtract one.
-    return BlockCount;
-  }
-  // ObjCForCollectionStmt is skipped intentionally because the current
-  // application of the iteration counts is not relevant for it.
-  return std::nullopt;
-}
-
 void CoreEngine::enqueue(ExplodedNodeSet &Set) {
   for (const auto I : Set)
     WList->enqueue(I);

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -2776,10 +2776,12 @@ assumeCondition(const Stmt *Condition, ExplodedNode *N) {
   return State->assume(V);
 }
 
-void ExprEngine::processBranch(
-    const Stmt *Condition, NodeBuilderContext &BldCtx, ExplodedNode *Pred,
-    ExplodedNodeSet &Dst, const CFGBlock *DstT, const CFGBlock *DstF,
-    std::optional<unsigned> IterationsCompletedInLoop) {
+void ExprEngine::processBranch(const Stmt *Condition,
+                               NodeBuilderContext& BldCtx,
+                               ExplodedNode *Pred,
+                               ExplodedNodeSet &Dst,
+                               const CFGBlock *DstT,
+                               const CFGBlock *DstF) {
   assert((!Condition || !isa<CXXBindTemporaryExpr>(Condition)) &&
          "CXXBindTemporaryExprs are handled by processBindTemporary.");
   const LocationContext *LCtx = Pred->getLocationContext();
@@ -2822,63 +2824,11 @@ void ExprEngine::processBranch(
     if (StTrue && StFalse)
       assert(!isa<ObjCForCollectionStmt>(Condition));
 
-    // We want to ensure consistent behavior between `eagerly-assume=false`,
-    // when the state split is always performed by the `assumeCondition()`
-    // call within this function and `eagerly-assume=true` (the default), when
-    // some conditions (comparison operators, unary negation) can trigger a
-    // state split before this callback. There are some contrived corner cases
-    // that behave differently with and without `eagerly-assume`, but I don't
-    // know about an example that could plausibly appear in "real" code.
-    bool BothFeasible =
-        (StTrue && StFalse) ||
-        didEagerlyAssumeBifurcateAt(PrevState, dyn_cast<Expr>(Condition));
-
-    if (StTrue) {
-      // In a loop, if both branches are feasible (i.e. the analyzer doesn't
-      // understand the loop condition) and two iterations have already been
-      // completed, then don't assume a third iteration because it is a
-      // redundant execution path (unlikely to be different from earlier loop
-      // exits) and can cause false positives if e.g. the loop iterates over a
-      // two-element structure with an opaque condition.
-      //
-      // The iteration count "2" is hardcoded because it's the natural limit:
-      // * the fact that the programmer wrote a loop (and not just an `if`)
-      //   implies that they thought that the loop body might be executed twice;
-      // * however, there are situations where the programmer knows that there
-      //   are at most two iterations but writes a loop that appears to be
-      //   generic, because there is no special syntax for "loop with at most
-      //   two iterations". (This pattern is common in FFMPEG and appears in
-      //   many other projects as well.)
-      bool CompletedTwoIterations = IterationsCompletedInLoop.value_or(0) >= 2;
-      bool SkipTrueBranch = BothFeasible && CompletedTwoIterations;
-
-      // FIXME: This "don't assume third iteration" heuristic partially
-      // conflicts with the widen-loop analysis option (which is off by
-      // default). If we intend to support and stabilize the loop widening,
-      // we must ensure that it 'plays nicely' with this logic.
-      if (!SkipTrueBranch || AMgr.options.ShouldWidenLoops)
-        Builder.generateNode(StTrue, true, PredN);
-    }
-
-    if (StFalse) {
-      // In a loop, if both branches are feasible (i.e. the analyzer doesn't
-      // understand the loop condition), we are before the first iteration and
-      // the analyzer option `assume-at-least-one-iteration` is set to `true`,
-      // then avoid creating the execution path where the loop is skipped.
-      //
-      // In some situations this "loop is skipped" execution path is an
-      // important corner case that may evade the notice of the developer and
-      // hide significant bugs -- however, there are also many situations where
-      // it's guaranteed that at least one iteration will happen (e.g. some
-      // data structure is always nonempty), but the analyzer cannot realize
-      // this and will produce false positives when it assumes that the loop is
-      // skipped.
-      bool BeforeFirstIteration = IterationsCompletedInLoop == std::optional{0};
-      bool SkipFalseBranch = BothFeasible && BeforeFirstIteration &&
-                             AMgr.options.ShouldAssumeAtLeastOneIteration;
-      if (!SkipFalseBranch)
-        Builder.generateNode(StFalse, false, PredN);
-    }
+    if (StTrue)
+      Builder.generateNode(StTrue, true, PredN);
+
+    if (StFalse)
+      Builder.generateNode(StFalse, false, PredN);
   }
   currBldrCtx = nullptr;
 }
@@ -3819,12 +3769,6 @@ ExprEngine::getEagerlyAssumeBifurcationTags() {
   return std::make_pair(&TrueTag, &FalseTag);
 }
 
-/// If the last EagerlyAssume attempt was successful (i.e. the true and false
-/// cases were both feasible), this state trait stores the expression where it
-/// happened; otherwise this holds nullptr.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(LastEagerlyAssumeExprIfSuccessful,
-                                 const Expr *)
-
 void ExprEngine::evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst,
                                               ExplodedNodeSet &Src,
                                               const Expr *Ex) {
@@ -3840,19 +3784,13 @@ void ExprEngine::evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst,
     }
 
     ProgramStateRef State = Pred->getState();
-    State = State->set<LastEagerlyAssumeExprIfSuccessful>(nullptr);
     SVal V = State->getSVal(Ex, Pred->getLocationContext());
     std::optional<nonloc::SymbolVal> SEV = V.getAs<nonloc::SymbolVal>();
     if (SEV && SEV->isExpression()) {
       const auto &[TrueTag, FalseTag] = getEagerlyAssumeBifurcationTags();
 
       auto [StateTrue, StateFalse] = State->assume(*SEV);
 
-      if (StateTrue && StateFalse) {
-        StateTrue = StateTrue->set<LastEagerlyAssumeExprIfSuccessful>(Ex);
-        StateFalse = StateFalse->set<LastEagerlyAssumeExprIfSuccessful>(Ex);
-      }
-
       // First assume that the condition is true.
       if (StateTrue) {
         SVal Val = svalBuilder.makeIntVal(1U, Ex->getType());
@@ -3870,11 +3808,6 @@ void ExprEngine::evalEagerlyAssumeBifurcation(ExplodedNodeSet &Dst,
   }
 }
 
-bool ExprEngine::didEagerlyAssumeBifurcateAt(ProgramStateRef State,
-                                             const Expr *Ex) const {
-  return Ex && State->get<LastEagerlyAssumeExprIfSuccessful>() == Ex;
-}
-
 void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
                                  ExplodedNodeSet &Dst) {
   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);