[PATCH] D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:822-831
+    assignToContainer(C, CE, LVal, Cont);
+    if (!(LPos = getIteratorPosition(State, LVal)))
       return;
-    State = saveComparison(State, Condition, LVal, RVal, Op == OO_EqualEqual);
-    C.addTransition(State);
-  } else if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
-    if ((State = processComparison(
-             State, getRegionOrSymbol(LVal), getRegionOrSymbol(RVal),
-             (Op == OO_EqualEqual) == (TruthVal->getValue() != 0)))) {
+  } else if (!RPos) {
+    assignToContainer(C, CE, RVal, Cont);
+    if (!(RPos = getIteratorPosition(State, RVal)))
+      return;
----------------
NoQ wrote:
> Welcome to the `addTransition()` hell!
> 
> Each of the `assignToContainer()` may add a transition, and then `processComparison()` also adds transitions. I suspect that it may lead to more state splits that were intended. I.e., the execution path on which the iterator is assigned to a container would be different from the two execution paths on which the comparison was processed.
> 
> You can chain `addTransition()`s to each other, eg.:
> ```lang=c++
> // Return the node produced by the inner addTransition()
> ExplodedNode *N = assignToContainer(...);
> 
> // And then in processComparison(N, ...)
> C.addTransition(N->getState()->assume(*ConditionVal, false), N);
> C.addTransition(N->getState()->assume(*ConditionVal, true), N);
> ```
> 
> It should also be possible to avoid transitions until the final state is computed, if the code is easy enough to refactor this way:
> ```lang=c++
> // No addTransition() calls within, just produce the state
> ProgramStateRef State = assignToContainer(...);
> 
> // And then in processComparison(N, ...)
> C.addTransition(State->assume(*ConditionVal, false), N);
> C.addTransition(State->assume(*ConditionVal, true), N);
> ```
> 
> This sort of stuff can be tested via `clang_analyzer_numTimesReached()` - see if you made exactly as many state splits as you wanted to.
My feel is that a better `.addTransition()` API should capture the user's intent more straightforwardly, so that we could check dynamically that the resulting topology is indeed exactly what the user expects.

I.e., produce multiple narrow-purpose APIs for common patterns: `C.updateState(State)`, `C.splitState(State1, State2, ..., StateN)` - both would fail if there were previous transitions in the same `CheckerContext` or if more transitions are made after them. The `updateState()` variant should probably try to lazily collapse multiple updates into a single node. Maybe instead don't require all branches to be specified simultaneously, i.e. instead do `addBranch(State)` that wouldn't fail in presence of other branches but would still conflict with `updateState()`.

These narrow-purpose APIs are too clumsy to cover the current use-case, but at least they would've caught the problem. Maybe a better design could make it also comfortable to use.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53701/new/

https://reviews.llvm.org/D53701