[analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

[Szelethus]

Summary

Based on the discussion in D82598#2171312. Thanks @NoQ!

Let's talk about how we got here. D82598 is titled "Get rid of statement liveness, because such a thing doesn't exist", and indeed, expressions express a value, non-expression statements don't.

if (a && get() || []{ return true; }())
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ has a value
    ~ has a value
    ~~~~~~~~~~ has a value
                  ~~~~~~~~~~~~~~~~~~~~ has a value
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ doesn't have a value

That is simple enough, so it would only make sense if we only assigned symbolic values to expressions in the static analyzer. Yet the interface checkers can access presents, among other strange things, the following two methods:

ProgramState::BindExpr(const Stmt *S, const LocationContext *LCtx, SVal V, bool Invalidate=true)
ProgramState::getSVal(const Stmt *S, const LocationContext *LCtx)

So, what gives? Turns out, we make an exception for ReturnStmt (which we'll leave for another time) and ObjCForCollectionStmt. For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one (CXXForRangeStmt has an implicit one!). In its absence, we assigned the actual statement with a concrete 1 or 0 to indicate whether there are any more iterations left. However, this is wildly incorrect, its just simply not true that the for statement has a value of 1 or 0, we can't calculate its liveness because that doesn't make any sense either, so this patch turns it into a GDM trait.

This patch isn't quite done (its nowhere near up to my standards in terms of documentation), but I wanted to publish it to avoid sinking too much work into a doomed patch.

[martong]

For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one

Shouldn't we try to fix the ObjCForCollectionStmt in the AST rather? By adding the condition to that as a (sub)expression? There are already many discrepancies and inconsistencies between supposedly similar AST nodes, and people do fix them occasionally (e.g. D81787).
To be honest, it seems a bit off to store some parts of the liveness info in the GDM while other parts not in the GDM ...

[Szelethus]

In D86736#2244365, @martong wrote:

For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one

Shouldn't we try to fix the ObjCForCollectionStmt in the AST rather? By adding the condition to that as a (sub)expression? There are already many discrepancies and inconsistencies between supposedly similar AST nodes, and people do fix them occasionally (e.g. D81787).

To be fair, before I say anything, I don't know much about Objective C, but I don't think this is a bug. This is just how for each loops are in it. The C++ standard specifies that ranged based for loops must be equivalent to a pre-C++11 loop, hence the implicit condition in the AST. I think this is the same kind of annoyance we chatted about regarding virtuality in the AST -- even if a method is virtual, it may not show up as such in the AST if the keyword itself is absent.

To be honest, it seems a bit off to store some parts of the liveness info in the GDM while other parts not in the GDM ...

There is no liveness to talk about here, it just simply doesn't make sense. This is just a property: "Does this loop have any more iterations left?". While the earlier hack looks convincing that this has something more to it, it really doesn't. In fact, this patch demonstrates it quite well: we only need to keep track of this information until we make a state split, so theoretically, we could just pass this along as a parameter. The only thing holding me back from that solution is that it would be a lot more disruptive change, and would require a change to the checkBranchCondition callback.

[martong]

In D86736#2247035, @Szelethus wrote:

In D86736#2244365, @martong wrote:

For any other loops, in order to know whether we should analyze another iteration, among other things, we evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because it simply doesn't have one

Shouldn't we try to fix the ObjCForCollectionStmt in the AST rather? By adding the condition to that as a (sub)expression? There are already many discrepancies and inconsistencies between supposedly similar AST nodes, and people do fix them occasionally (e.g. D81787).

To be fair, before I say anything, I don't know much about Objective C, but I don't think this is a bug. This is just how for each loops are in it. The C++ standard specifies that ranged based for loops must be equivalent to a pre-C++11 loop, hence the implicit condition in the AST. I think this is the same kind of annoyance we chatted about regarding virtuality in the AST -- even if a method is virtual, it may not show up as such in the AST if the keyword itself is absent.

To be honest, it seems a bit off to store some parts of the liveness info in the GDM while other parts not in the GDM ...

There is no liveness to talk about here, it just simply doesn't make sense. This is just a property: "Does this loop have any more iterations left?". While the earlier hack looks convincing that this has something more to it, it really doesn't. In fact, this patch demonstrates it quite well: we only need to keep track of this information until we make a state split, so theoretically, we could just pass this along as a parameter. The only thing holding me back from that solution is that it would be a lot more disruptive change, and would require a change to the checkBranchCondition callback.

Ok, fair enough.

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

327 ↗

(On Diff #288434)

Why it is not enough to simply have ObjCForCollectionStmt* as a key?

[steakhal]

I don't have much to say, but to keep up the good work xD
I will follow this and the rest of your patches @Szelethus.

[Szelethus]

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

327 ↗

(On Diff #288434)

Environment is the data structure we use to bind values to statements (which would ideally be expressions only). The problem is that a statement on its own isn't enough to identify where we are in the analysis:

void traverse(Graph &G) {
 // SubG may need to be modeled in different stackframes, but has the same Stmt*
 for (Graph &SubG : G->successors()) {
    traverse(SubG);
  }
}

LocationContext can be thought of as a stack function calls (or anything with a distinct stack frame). Indeed, if two (Stmt *, LocationContext *) pairs are equal, we're rebinding the same expression (another loop iteration, goto), if only the statements are the same, we're not in the same function call (stack frame).

Of course ObjCForCollectionStmt can not have a value, that is the whole point of the patch, but we still need a LocationContext to correctly identify precisely which loop we're talking about.

[martong]

I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days.

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

327 ↗

(On Diff #288434)

Yeah, I did not think about stack frames, it makes perfect sense what you write, thanks for the explanation!

[Szelethus]

In D86736#2249920, @martong wrote:

I don't have anymore immediate concerns, but I will need more time to comb through the rest of the patch in more details... hopefully I can do that in the following days.

Thank you so much! I fear my project might take a turn, so this definitely isn't a high priority patch.

[NoQ]

This looks correct to me. Thanks a lot. This hack was surprisingly crude and i'm very glad that we now consistently use expressions in our Environment.

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

329 ↗

(On Diff #288434)

I'd really prefer this to live in ExprEngine.cpp, like C++ constructor traits, and not be exposed in the ProgramState API, because as of now this is an implementation hack that other clients of ProgramState don't beed to know about.

The actual non-placeholder implementation would i guess be interesting. The loop boils down to repeatedly sending certain messages to the collection. So i guess we could evalCall them in the checker that'll deal with modeling these collections.

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

498–499

IIRC "everything is" is the valid English.

[Szelethus]

In D86736#2257880, @NoQ wrote:

i'm very glad that we now consistently use expressions in our Environment.

*except for ReturnStmt, but I haven't dug myself into that pit just yet :) Thanks!

[Szelethus]

Reinstate the assert removed in rG032b78a0762bee129f33e4255ada6d374aa70c71, add a test. Enforce that Environment only makes an exception for ReturnStmt in terms of non-expression statements (and hope that isn't going to stay that way for long).

edit: And most importantly, move the new interface to ExprEngine.

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

private:
   /// constructing into an existing region.
   const CXXConstructExpr *findDirectConstructorForCurrentCFGElement();
 
   /// Common code that handles either a CXXConstructExpr or a
   /// CXXInheritedCtorInitExpr.
   void handleConstructor(const Expr *E, ExplodedNode *Pred,
                          ExplodedNodeSet &Dst);
 
+public:
+  /// Note whether this loop has any more iteratios to model. These methods are
+  /// essentially an interface for a GDM trait. Further reading in
+  /// ExprEngine::VisitObjCForCollectionStmt().
+  LLVM_NODISCARD static ProgramStateRef
+  setWhetherHasMoreIteration(ProgramStateRef State,
+                             const ObjCForCollectionStmt *O,
+                             const LocationContext *LC, bool HasMoreIteraton);
+
+  LLVM_NODISCARD static ProgramStateRef
+  removeIterationState(ProgramStateRef State, const ObjCForCollectionStmt *O,
+                       const LocationContext *LC);
+
+  LLVM_NODISCARD static bool hasMoreIteration(ProgramStateRef State,
+                                              const ObjCForCollectionStmt *O,
+                                              const LocationContext *LC);
+private:
   /// Store the location of a C++ object corresponding to a statement
   /// until the statement is actually encountered. For example, if a DeclStmt
   /// has CXXConstructExpr as its initializer, the object would be considered
   /// to be "under construction" between CXXConstructExpr and DeclStmt.
   /// This allows, among other things, to keep bindings to variable's fields
   /// made within the constructor alive until its declaration actually
   /// goes into scope.
   static ProgramStateRef

clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

static bool alreadyExecutedAtLeastOneLoopIteration(const ExplodedNode *N,
   return false;
 }
 
 void ObjCLoopChecker::checkPostStmt(const ObjCForCollectionStmt *FCS,
                                     CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // Check if this is the branch for the end of the loop.
-  SVal CollectionSentinel = C.getSVal(FCS);
-  if (CollectionSentinel.isZeroConstant()) {
+  if (!ExprEngine::hasMoreIteration(State, FCS, C.getLocationContext())) {
     if (!alreadyExecutedAtLeastOneLoopIteration(C.getPredecessor(), FCS))
       State = assumeCollectionNonEmpty(C, State, FCS, /*Assumption*/false);
 
   // Otherwise, this is a branch that goes through the loop body.
   } else {
     State = checkCollectionNonNil(C, State, FCS);
     State = checkElementNonNil(C, State, FCS);
     State = assumeCollectionNonEmpty(C, State, FCS, /*Assumption*/true);

clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp

 //=== UndefBranchChecker.cpp -----------------------------------*- C++ -*--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines UndefBranchChecker, which checks for undefined branch
 // condition.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/StmtObjC.h"
+#include "clang/AST/Type.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include <utility>
 
 using namespace clang;
bool MatchesCriteria(const Expr *Ex) {
       return St->getSVal(Ex, LCtx).isUndef();
     }
   };
 
 public:
   void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const;
 };
 
-}
+} // namespace
 
 void UndefBranchChecker::checkBranchCondition(const Stmt *Condition,
                                               CheckerContext &Ctx) const {
+  // ObjCForCollection is a loop, but has no actual condition.
+  if (isa<ObjCForCollectionStmt>(Condition))
+    return;
   SVal X = Ctx.getSVal(Condition);
   if (X.isUndef()) {
     // Generate a sink node, which implicitly marks both outgoing branches as
     // infeasible.
     ExplodedNode *N = Ctx.generateErrorNode();
     if (N) {
       if (!BT)
         BT.reset(new BuiltinBug(

clang/lib/StaticAnalyzer/Core/Environment.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/Environment.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Stmt.h"
+#include "clang/AST/StmtObjC.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/JsonSupport.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
if (X) {
     return V;
   }
   return UnknownVal();
 }
 
 SVal Environment::getSVal(const EnvironmentEntry &Entry,
                           SValBuilder& svalBuilder) const {
   const Stmt *S = Entry.getStmt();
+  assert(!isa<ObjCForCollectionStmt>(S) &&
+         "Use ExprEngine::hasMoreIteration()!");
+  assert((isa<Expr>(S) || isa<ReturnStmt>(S)) &&
+         "Environment can only argue about Exprs, since only they express "
+         "a value! Any non-expression statement stored in Environment is a "
+         "result of a hack!");
   const LocationContext *LCtx = Entry.getLocationContext();
 
   switch (S->getStmtClass()) {
   case Stmt::CXXBindTemporaryExprClass:
   case Stmt::ExprWithCleanupsClass:
   case Stmt::GenericSelectionExprClass:
   case Stmt::OpaqueValueExprClass:
   case Stmt::ConstantExprClass:
EBMapRef(NewEnv.ExprBindings.getRootWithoutRetain(),
              F.getTreeFactory());
 
   // Iterate over the block-expr bindings.
   for (Environment::iterator I = Env.begin(), E = Env.end();
        I != E; ++I) {
     const EnvironmentEntry &BlkExpr = I.getKey();
     const SVal &X = I.getData();
 
-    if (SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext())) {
+    const bool IsBlkExprLive =
+        SymReaper.isLive(BlkExpr.getStmt(), BlkExpr.getLocationContext());
+
+    assert((isa<Expr>(BlkExpr.getStmt()) || !IsBlkExprLive) &&
+           "Only Exprs can be live, LivenessAnalysis argues about the liveness "
+           "of *values*!");
+
+    if (IsBlkExprLive) {
       // Copy the binding to the new map.
       EBMapRef = EBMapRef.add(BlkExpr, X);
 
       // Mark all symbols in the block expr's value live.
       RSScaner.scan(X);
     }
   }
 

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (!CS)
       continue;
     const Stmt *LastStmt = CS->getStmt();
     assert(LastStmt == Condition || LastStmt == getRightmostLeaf(Condition));
     return LastStmt;
   }
   llvm_unreachable("could not resolve condition");
 }
 
+using ObjCForLctxPair =
+    std::pair<const ObjCForCollectionStmt *, const LocationContext *>;
+
+REGISTER_MAP_WITH_PROGRAMSTATE(ObjCForHasMoreIterations, ObjCForLctxPair, bool)
+
+ProgramStateRef ExprEngine::setWhetherHasMoreIteration(
+    ProgramStateRef State, const ObjCForCollectionStmt *O,
+    const LocationContext *LC, bool HasMoreIteraton) {
+  assert(!State->contains<ObjCForHasMoreIterations>({O, LC}));
+  return State->set<ObjCForHasMoreIterations>({O, LC}, HasMoreIteraton);
+}
+
+ProgramStateRef
+ExprEngine::removeIterationState(ProgramStateRef State,
+                                 const ObjCForCollectionStmt *O,
+                                 const LocationContext *LC) {
+  assert(State->contains<ObjCForHasMoreIterations>({O, LC}));
+  return State->remove<ObjCForHasMoreIterations>({O, LC});
+}
+
+bool ExprEngine::hasMoreIteration(ProgramStateRef State,
+                                  const ObjCForCollectionStmt *O,
+                                  const LocationContext *LC) {
+  assert(State->contains<ObjCForHasMoreIterations>({O, LC}));
+  return *State->get<ObjCForHasMoreIterations>({O, LC});
+}
+
+/// Split the state on whether there are any more iterations left for this loop.
+/// Returns a (HasMoreIteration, HasNoMoreIteration) pair, or None when the
+/// acquisition of the loop condition value failed.
+static Optional<std::pair<ProgramStateRef, ProgramStateRef>>
+assumeCondition(const Stmt *Condition, ExplodedNode *N) {
+  ProgramStateRef State = N->getState();
+  if (const auto *ObjCFor = dyn_cast<ObjCForCollectionStmt>(Condition)) {
+    bool HasMoreIteraton =
+        ExprEngine::hasMoreIteration(State, ObjCFor, N->getLocationContext());
+    // Checkers have already ran on branch conditions, so the current
+    // information as to whether the loop has more iteration becomes outdated
+    // after this point.
+    State = ExprEngine::removeIterationState(State, ObjCFor,
+                                             N->getLocationContext());
+    if (HasMoreIteraton)
+      return std::pair<ProgramStateRef, ProgramStateRef>{State, nullptr};
+    else
+      return std::pair<ProgramStateRef, ProgramStateRef>{nullptr, State};
+  }
+  SVal X = State->getSVal(Condition, N->getLocationContext());
+
+  if (X.isUnknownOrUndef()) {
+    // Give it a chance to recover from unknown.
+    if (const auto *Ex = dyn_cast<Expr>(Condition)) {
+      if (Ex->getType()->isIntegralOrEnumerationType()) {
+        // Try to recover some path-sensitivity.  Right now casts of symbolic
+        // integers that promote their values are currently not tracked well.
+        // If 'Condition' is such an expression, try and recover the
+        // underlying value and use that instead.
+        SVal recovered =
+            RecoverCastedSymbol(State, Condition, N->getLocationContext(),
+                                N->getState()->getStateManager().getContext());
+
+        if (!recovered.isUnknown()) {
+          X = recovered;
+        }
+      }
+    }
+  }
+
+  // If the condition is still unknown, give up.
+  if (X.isUnknownOrUndef())
+    return None;
+
+  DefinedSVal V = X.castAs<DefinedSVal>();
+
+  ProgramStateRef StTrue, StFalse;
+  return State->assume(V);
+}
+
 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.");
void ExprEngine::processBranch(const Stmt *Condition,
   ExplodedNodeSet CheckersOutSet;
   getCheckerManager().runCheckersForBranchCondition(Condition, CheckersOutSet,
                                                     Pred, *this);
   // We generated only sinks.
   if (CheckersOutSet.empty())
     return;
 
   BranchNodeBuilder builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
-  for (const auto PredI : CheckersOutSet) {
-    if (PredI->isSink())
+  for (ExplodedNode *PredN : CheckersOutSet) {
+    if (PredN->isSink())
       continue;
 
-    ProgramStateRef PrevState = PredI->getState();
-    SVal X = PrevState->getSVal(Condition, PredI->getLocationContext());
-
-    if (X.isUnknownOrUndef()) {
-      // Give it a chance to recover from unknown.
-      if (const auto *Ex = dyn_cast<Expr>(Condition)) {
-        if (Ex->getType()->isIntegralOrEnumerationType()) {
-          // Try to recover some path-sensitivity.  Right now casts of symbolic
-          // integers that promote their values are currently not tracked well.
-          // If 'Condition' is such an expression, try and recover the
-          // underlying value and use that instead.
-          SVal recovered = RecoverCastedSymbol(PrevState, Condition,
-                                               PredI->getLocationContext(),
-                                               getContext());
+    ProgramStateRef PrevState = PredN->getState();
 
-          if (!recovered.isUnknown()) {
-            X = recovered;
-          }
-        }
-      }
-    }
-
-    // If the condition is still unknown, give up.
-    if (X.isUnknownOrUndef()) {
-      builder.generateNode(PrevState, true, PredI);
-      builder.generateNode(PrevState, false, PredI);
+    ProgramStateRef StTrue, StFalse;
+    if (const auto KnownCondValueAssumption = assumeCondition(Condition, PredN))
+      std::tie(StTrue, StFalse) = *KnownCondValueAssumption;
+    else {
+      assert(!isa<ObjCForCollectionStmt>(Condition));
+      builder.generateNode(PrevState, true, PredN);
+      builder.generateNode(PrevState, false, PredN);
       continue;
     }
-
-    DefinedSVal V = X.castAs<DefinedSVal>();
-
-    ProgramStateRef StTrue, StFalse;
-    std::tie(StTrue, StFalse) = PrevState->assume(V);
+    if (StTrue && StFalse)
+      assert(!isa<ObjCForCollectionStmt>(Condition));;
 
     // Process the true branch.
     if (builder.isFeasible(true)) {
       if (StTrue)
-        builder.generateNode(StTrue, true, PredI);
+        builder.generateNode(StTrue, true, PredN);
       else
         builder.markInfeasible(true);
     }
 
     // Process the false branch.
     if (builder.isFeasible(false)) {
       if (StFalse)
-        builder.generateNode(StFalse, false, PredI);
+        builder.generateNode(StFalse, false, PredN);
       else
         builder.markInfeasible(false);
     }
   }
   currBldrCtx = nullptr;
 }
 
 /// The GDM component containing the set of global variables which have been

clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

static void populateObjCForDestinationSet(
     const ObjCForCollectionStmt *S, const Stmt *elem, SVal elementV,
     SymbolManager &SymMgr, const NodeBuilderContext *currBldrCtx,
     StmtNodeBuilder &Bldr, bool hasElements) {
 
   for (ExplodedNode *Pred : dstLocation) {
     ProgramStateRef state = Pred->getState();
     const LocationContext *LCtx = Pred->getLocationContext();
 
-    SVal hasElementsV = svalBuilder.makeTruthVal(hasElements);
-
-    // FIXME: S is not an expression. We should not be binding values to it.
-    ProgramStateRef nextState = state->BindExpr(S, LCtx, hasElementsV);
+    ProgramStateRef nextState =
+        ExprEngine::setWhetherHasMoreIteration(state, S, LCtx, hasElements);
 
     if (auto MV = elementV.getAs<loc::MemRegionVal>())
       if (const auto *R = dyn_cast<TypedValueRegion>(MV->getRegion())) {
         // FIXME: The proper thing to do is to really iterate over the
         //  container.  We will do this with dispatch logic to the store.
         //  For now, just 'conjure' up a symbolic value.
         QualType T = R->getValueType();
         assert(Loc::isLocType(T));
void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S,
 
   // ObjCForCollectionStmts are processed in two places.  This method
   // handles the case where an ObjCForCollectionStmt* occurs as one of the
   // statements within a basic block.  This transfer function does two things:
   //
   //  (1) binds the next container value to 'element'.  This creates a new
   //      node in the ExplodedGraph.
   //
-  //  (2) binds the value 0/1 to the ObjCForCollectionStmt* itself, indicating
-  //      whether or not the container has any more elements.  This value
-  //      will be tested in ProcessBranch.  We need to explicitly bind
-  //      this value because a container can contain nil elements.
+  //  (2) note whether the collection has any more elements (or in other words,
+  //      whether the loop has more iterations). This will be tested in
+  //      processBranch.
   //
   // FIXME: Eventually this logic should actually do dispatches to
   //   'countByEnumeratingWithState:objects:count:' (NSFastEnumeration).
   //   This will require simulating a temporary NSFastEnumerationState, either
   //   through an SVal or through the use of MemRegions.  This value can
   //   be affixed to the ObjCForCollectionStmt* instead of 0/1; when the loop
   //   terminates we reclaim the temporary (it goes out of scope) and we
   //   we can test if the SVal is 0 or if the MemRegion is null (depending

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

 //===- SymbolManager.h - Management of Symbolic Values --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines SymbolManager, a class that manages symbolic values
 //  created for use by ExprEngine and related classes.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/StmtObjC.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "llvm/ADT/FoldingSet.h"
SymbolReaper::isLive(const Stmt *ExprVal, const LocationContext *ELCtx) const {
   if (LCtx != ELCtx) {
     // If the reaper's location context is a parent of the expression's
     // location context, then the expression value is now "out of scope".
     if (LCtx->isParentOf(ELCtx))
       return false;
     return true;
   }
 
-  // If no statement is provided, everything is this and parent contexts is live.
+  // If no statement is provided, everything in this and parent contexts is
+  // live.
   if (!Loc)
     return true;
 
   return LCtx->getAnalysis<RelaxedLiveVariables>()->isLive(Loc, ExprVal);
 }
 
 bool SymbolReaper::isLive(const VarRegion *VR, bool includeStoreBindings) const{
   const StackFrameContext *VarContext = VR->getStackFrame();

clang/test/Analysis/objc-live-crash.mm

+// RUN: %clang --analyze %s -fblocks
+
+// https://reviews.llvm.org/D82598#2171312
+
+@interface Item
+// ...
+@end
+
+@interface Collection
+// ...
+@end
+
+typedef void (^Blk)();
+
+struct RAII {
+  Blk blk;
+
+public:
+  RAII(Blk blk): blk(blk) {}
+  ~RAII() { blk(); }
+};
+
+void foo(Collection *coll) {
+  RAII raii(^{});
+  for (Item *item in coll) {}
+  int i;
+  {
+    int j;
+  }
+}