[clang][analyzer] Simplify code of StreamChecker (NFC).

[balazske]

Common parts of some "eval" functions are moved into one function. The non-common parts of the "eval" functions are passed through lambda parameters to the new function.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Kéri (balazske)

Changes

Common parts of some "eval" functions are moved into one function. The non-common parts of the "eval" functions are passed through lambda parameters to the new function.

---

Full diff: https://github.com/llvm/llvm-project/pull/79312.diff

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+230-208)

diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..95e7e5d9f0912cf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -400,6 +400,51 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
   void evalFflush(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
+  // Contains function parameters to 'evalRWCommon'.
+  struct EvalRWCommonFn {
+    using InitFn = std::function<ProgramStateRef(SymbolRef, const CallExpr *,
+                                                 ASTContext &, SValBuilder &)>;
+    using GetSuccessStateFn = std::function<ProgramStateRef(
+        SymbolRef, const CallExpr *, ASTContext &, SValBuilder &)>;
+    using GetFailureStateFn =
+        std::function<std::pair<ProgramStateRef, StreamErrorState>(
+            SymbolRef, const CallExpr *, ASTContext &, SValBuilder &)>;
+
+    EvalRWCommonFn(InitFn I, GetSuccessStateFn GSS, GetFailureStateFn GFS)
+        : Init(I), GetSuccessState(GSS), GetFailureState(GFS) {}
+    EvalRWCommonFn(GetSuccessStateFn GSS, GetFailureStateFn GFS)
+        : Init([](SymbolRef, const CallExpr *, ASTContext &, SValBuilder &) {
+            return nullptr;
+          }),
+          GetSuccessState(GSS), GetFailureState(GFS) {}
+
+    // Called at start of the evaluation.
+    // Should return 'nullptr' to continue evaluation, or a program state that
+    // is added and evaluation stops here.
+    // This is used to handle special cases when no success or failure state
+    // is added.
+    InitFn Init;
+    // Create the state for the "success" case of the function and return it.
+    // Can return 'nullptr' to not add a success state.
+    GetSuccessStateFn GetSuccessState;
+    // Create the state for the "failure" case of the function.
+    // Return the state and the new stream error state (combination of possible
+    // errors).
+    GetFailureStateFn GetFailureState;
+  };
+
+  /// Common part of many eval functions in the checker.
+  /// This can be used for simple stream read and write functions.
+  /// This function handles full modeling of such functions, by passing a
+  /// 'GetSuccessState' and 'GetFailureState' function. These functions should
+  /// create the program state for the success and failure cases. 'IsRead'
+  /// indicates if the modeled function is a stream read or write operation.
+  /// If the evaluated function is a read operation and the stream is already
+  /// in EOF, the new error will always be EOF and no success state is added.
+  /// This case is handled here and 'GetFailureState' should not care about it.
+  void evalRWCommon(const FnDescription *Desc, const CallEvent &Call,
+                    CheckerContext &C, bool IsRead, EvalRWCommonFn Fn) const;
+
   /// Check that the stream (in StreamVal) is not NULL.
   /// If it can only be NULL a fatal error is emitted and nullptr returned.
   /// Otherwise the return value is a new state where the stream is constrained
@@ -726,225 +771,140 @@ void StreamChecker::preReadWrite(const FnDescription *Desc,
 void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
                                     const CallEvent &Call, CheckerContext &C,
                                     bool IsFread) const {
-  ProgramStateRef State = C.getState();
-  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
-  if (!StreamSym)
-    return;
-
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return;
-
-  std::optional<NonLoc> SizeVal = Call.getArgSVal(1).getAs<NonLoc>();
-  if (!SizeVal)
-    return;
-  std::optional<NonLoc> NMembVal = Call.getArgSVal(2).getAs<NonLoc>();
-  if (!NMembVal)
-    return;
-
-  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
-  if (!OldSS)
-    return;
-
-  assertStreamStateOpened(OldSS);
-
-  // C'99 standard, §7.19.8.1.3, the return value of fread:
-  // The fread function returns the number of elements successfully read, which
-  // may be less than nmemb if a read error or end-of-file is encountered. If
-  // size or nmemb is zero, fread returns zero and the contents of the array and
-  // the state of the stream remain unchanged.
-
-  if (State->isNull(*SizeVal).isConstrainedTrue() ||
-      State->isNull(*NMembVal).isConstrainedTrue()) {
-    // This is the "size or nmemb is zero" case.
-    // Just return 0, do nothing more (not clear the error flags).
-    State = bindInt(0, State, C, CE);
-    C.addTransition(State);
-    return;
-  }
-
-  // Generate a transition for the success state.
-  // If we know the state to be FEOF at fread, do not add a success state.
-  if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
-    ProgramStateRef StateNotFailed =
-        State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-    StateNotFailed =
-        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
-    C.addTransition(StateNotFailed);
-  }
-
-  // Add transition for the failed state.
-  NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
-  ProgramStateRef StateFailed =
-      State->BindExpr(CE, C.getLocationContext(), RetVal);
-  SValBuilder &SVB = C.getSValBuilder();
-  auto Cond =
-      SVB.evalBinOpNN(State, BO_LT, RetVal, *NMembVal, SVB.getConditionType())
-          .getAs<DefinedOrUnknownSVal>();
-  if (!Cond)
-    return;
-  StateFailed = StateFailed->assume(*Cond, true);
-  if (!StateFailed)
-    return;
-
-  StreamErrorState NewES;
-  if (IsFread)
-    NewES =
-        (OldSS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
-  else
-    NewES = ErrorFError;
-  // If a (non-EOF) error occurs, the resulting value of the file position
-  // indicator for the stream is indeterminate.
-  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
-  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
-  if (IsFread && OldSS->ErrorState != ErrorFEof)
-    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
-  else
-    C.addTransition(StateFailed);
+  std::optional<NonLoc> SizeVal;
+  std::optional<NonLoc> NMembVal;
+
+  EvalRWCommonFn Fn{
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) -> ProgramStateRef {
+        SizeVal = Call.getArgSVal(1).getAs<NonLoc>();
+        NMembVal = Call.getArgSVal(2).getAs<NonLoc>();
+        if (!SizeVal || !NMembVal)
+          return C.getState();
+
+        // C'99 standard, §7.19.8.1.3, the return value of fread:
+        // The fread function returns the number of elements successfully read,
+        // which may be less than nmemb if a read error or end-of-file is
+        // encountered. If size or nmemb is zero, fread returns zero and the
+        // contents of the array and the state of the stream remain unchanged.
+        if (C.getState()->isNull(*SizeVal).isConstrainedTrue() ||
+            C.getState()->isNull(*NMembVal).isConstrainedTrue())
+          // This is the "size or nmemb is zero" case.
+          // Just return 0, do nothing more (not clear the error flags).
+          return bindInt(0, C.getState(), C, CE);
+        return nullptr;
+      },
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) {
+        return C.getState()->BindExpr(CE, C.getLocationContext(), *NMembVal);
+      },
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) -> std::pair<ProgramStateRef, StreamErrorState> {
+        NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+        ProgramStateRef State =
+            C.getState()->BindExpr(CE, C.getLocationContext(), RetVal);
+        auto Cond = SVB.evalBinOpNN(State, BO_LT, RetVal, *NMembVal,
+                                    SVB.getConditionType())
+                        .getAs<DefinedOrUnknownSVal>();
+        if (!Cond)
+          return {nullptr, ErrorNone};
+        State = State->assume(*Cond, true);
+        StreamErrorState NewES =
+            (IsFread ? (ErrorFEof | ErrorFError) : ErrorFError);
+        return {State, NewES};
+      }};
+
+  evalRWCommon(Desc, Call, C, IsFread, Fn);
 }
 
 void StreamChecker::evalFgetx(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C, bool SingleChar) const {
-  ProgramStateRef State = C.getState();
-  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
-  if (!StreamSym)
-    return;
-
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return;
-
-  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
-  if (!OldSS)
-    return;
-
-  assertStreamStateOpened(OldSS);
-
   // `fgetc` returns the read character on success, otherwise returns EOF.
   // `fgets` returns the read buffer address on success, otherwise returns NULL.
-
-  if (OldSS->ErrorState != ErrorFEof) {
-    if (SingleChar) {
-      // Generate a transition for the success state of `fgetc`.
-      NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
-      ProgramStateRef StateNotFailed =
-          State->BindExpr(CE, C.getLocationContext(), RetVal);
-      SValBuilder &SVB = C.getSValBuilder();
-      ASTContext &ASTC = C.getASTContext();
-      // The returned 'unsigned char' of `fgetc` is converted to 'int',
-      // so we need to check if it is in range [0, 255].
-      auto CondLow =
-          SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
-                        SVB.getConditionType())
-              .getAs<DefinedOrUnknownSVal>();
-      auto CondHigh =
-          SVB.evalBinOp(State, BO_LE, RetVal,
-                        SVB.makeIntVal(SVB.getBasicValueFactory()
-                                           .getMaxValue(ASTC.UnsignedCharTy)
-                                           .getLimitedValue(),
-                                       ASTC.IntTy),
-                        SVB.getConditionType())
-              .getAs<DefinedOrUnknownSVal>();
-      if (!CondLow || !CondHigh)
-        return;
-      StateNotFailed = StateNotFailed->assume(*CondLow, true);
-      if (!StateNotFailed)
-        return;
-      StateNotFailed = StateNotFailed->assume(*CondHigh, true);
-      if (!StateNotFailed)
-        return;
-      C.addTransition(StateNotFailed);
-    } else {
-      // Generate a transition for the success state of `fgets`.
-      std::optional<DefinedSVal> GetBuf =
-          Call.getArgSVal(0).getAs<DefinedSVal>();
-      if (!GetBuf)
-        return;
-      ProgramStateRef StateNotFailed =
-          State->BindExpr(CE, C.getLocationContext(), *GetBuf);
-      StateNotFailed = StateNotFailed->set<StreamMap>(
-          StreamSym, StreamState::getOpened(Desc));
-      C.addTransition(StateNotFailed);
-    }
-  }
-
-  // Add transition for the failed state.
-  ProgramStateRef StateFailed;
-  if (SingleChar)
-    StateFailed = bindInt(*EofVal, State, C, CE);
-  else
-    StateFailed =
-        State->BindExpr(CE, C.getLocationContext(),
-                        C.getSValBuilder().makeNullWithType(CE->getType()));
-
-  // If a (non-EOF) error occurs, the resulting value of the file position
-  // indicator for the stream is indeterminate.
-  StreamErrorState NewES =
-      OldSS->ErrorState == ErrorFEof ? ErrorFEof : ErrorFEof | ErrorFError;
-  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
-  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
-  if (OldSS->ErrorState != ErrorFEof)
-    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
-  else
-    C.addTransition(StateFailed);
+  EvalRWCommonFn Fn{
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) -> ProgramStateRef {
+        if (SingleChar) {
+          NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+          ProgramStateRef State =
+              C.getState()->BindExpr(CE, C.getLocationContext(), RetVal);
+          // The returned 'unsigned char' of `fgetc` is converted to 'int',
+          // so we need to check if it is in range [0, 255].
+          auto CondLow =
+              SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ACtx.IntTy),
+                            SVB.getConditionType())
+                  .getAs<DefinedOrUnknownSVal>();
+          auto CondHigh =
+              SVB.evalBinOp(State, BO_LE, RetVal,
+                            SVB.makeIntVal(SVB.getBasicValueFactory()
+                                               .getMaxValue(ACtx.UnsignedCharTy)
+                                               .getLimitedValue(),
+                                           ACtx.IntTy),
+                            SVB.getConditionType())
+                  .getAs<DefinedOrUnknownSVal>();
+          if (!CondLow || !CondHigh)
+            return nullptr;
+          State = State->assume(*CondLow, true);
+          if (!State)
+            return nullptr;
+          return State->assume(*CondHigh, true);
+        } else {
+          // Generate a transition for the success state of `fgets`.
+          std::optional<DefinedSVal> GetBuf =
+              Call.getArgSVal(0).getAs<DefinedSVal>();
+          if (!GetBuf)
+            return nullptr;
+          return C.getState()->BindExpr(CE, C.getLocationContext(), *GetBuf);
+        }
+      },
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) -> std::pair<ProgramStateRef, StreamErrorState> {
+        StreamErrorState NewES = ErrorFEof | ErrorFError;
+        if (SingleChar)
+          return std::make_pair(bindInt(*EofVal, C.getState(), C, CE), NewES);
+        else
+          return std::make_pair(
+              C.getState()->BindExpr(
+                  CE, C.getLocationContext(),
+                  C.getSValBuilder().makeNullWithType(CE->getType())),
+              NewES);
+      }};
+
+  evalRWCommon(Desc, Call, C, /*IsRead=*/true, Fn);
 }
 
 void StreamChecker::evalFputx(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C, bool IsSingleChar) const {
-  ProgramStateRef State = C.getState();
-  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
-  if (!StreamSym)
-    return;
-
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return;
-
-  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
-  if (!OldSS)
-    return;
-
-  assertStreamStateOpened(OldSS);
-
   // `fputc` returns the written character on success, otherwise returns EOF.
-  // `fputs` returns a non negative value on sucecess, otherwise returns EOF.
-
-  if (IsSingleChar) {
-    // Generate a transition for the success state of `fputc`.
-    std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
-    if (!PutVal)
-      return;
-    ProgramStateRef StateNotFailed =
-        State->BindExpr(CE, C.getLocationContext(), *PutVal);
-    StateNotFailed =
-        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
-    C.addTransition(StateNotFailed);
-  } else {
-    // Generate a transition for the success state of `fputs`.
-    NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
-    ProgramStateRef StateNotFailed =
-        State->BindExpr(CE, C.getLocationContext(), RetVal);
-    SValBuilder &SVB = C.getSValBuilder();
-    auto &ASTC = C.getASTContext();
-    auto Cond = SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ASTC.IntTy),
-                              SVB.getConditionType())
-                    .getAs<DefinedOrUnknownSVal>();
-    if (!Cond)
-      return;
-    StateNotFailed = StateNotFailed->assume(*Cond, true);
-    if (!StateNotFailed)
-      return;
-    StateNotFailed =
-        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
-    C.addTransition(StateNotFailed);
-  }
-
-  // Add transition for the failed state. The resulting value of the file
-  // position indicator for the stream is indeterminate.
-  ProgramStateRef StateFailed = bindInt(*EofVal, State, C, CE);
-  StreamState NewSS = StreamState::getOpened(Desc, ErrorFError, true);
-  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
-  C.addTransition(StateFailed);
+  // `fputs` returns a nonnegative value on success, otherwise returns EOF.
+  EvalRWCommonFn Fn{
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) -> ProgramStateRef {
+        if (IsSingleChar) {
+          std::optional<NonLoc> PutVal = Call.getArgSVal(0).getAs<NonLoc>();
+          if (!PutVal)
+            return nullptr;
+          return C.getState()->BindExpr(CE, C.getLocationContext(), *PutVal);
+        } else {
+          NonLoc RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+          ProgramStateRef State =
+              C.getState()->BindExpr(CE, C.getLocationContext(), RetVal);
+          auto Cond =
+              SVB.evalBinOp(State, BO_GE, RetVal, SVB.makeZeroVal(ACtx.IntTy),
+                            SVB.getConditionType())
+                  .getAs<DefinedOrUnknownSVal>();
+          if (!Cond)
+            return nullptr;
+          return State->assume(*Cond, true);
+        }
+      },
+      [&](SymbolRef StreamSym, const CallExpr *CE, ASTContext &ACtx,
+          SValBuilder &SVB) {
+        return std::make_pair(bindInt(*EofVal, C.getState(), C, CE),
+                              ErrorFError);
+      }};
+
+  evalRWCommon(Desc, Call, C, /*IsRead=*/false, Fn);
 }
 
 void StreamChecker::evalFprintf(const FnDescription *Desc,
@@ -1510,6 +1470,68 @@ void StreamChecker::evalFflush(const FnDescription *Desc, const CallEvent &Call,
   C.addTransition(StateFailed);
 }
 
+void StreamChecker::evalRWCommon(const FnDescription *Desc,
+                                 const CallEvent &Call, CheckerContext &C,
+                                 bool IsRead, EvalRWCommonFn Fn) const {
+  ProgramStateRef State = C.getState();
+  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!StreamSym)
+    return;
+
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
+
+  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
+  if (!OldSS)
+    return;
+
+  assertStreamStateOpened(OldSS);
+
+  ASTContext &ACtx = C.getASTContext();
+  SValBuilder &SVB = C.getSValBuilder();
+
+  if (ProgramStateRef State = Fn.Init(StreamSym, CE, ACtx, SVB)) {
+    C.addTransition(State);
+    return;
+  }
+
+  // A read operation will aways fail with EOF if the stream is already in EOF
+  // state, no success case is added.
+  if (!IsRead || OldSS->ErrorState != ErrorFEof) {
+    ProgramStateRef StateSuccess = Fn.GetSuccessState(StreamSym, CE, ACtx, SVB);
+    if (StateSuccess) {
+      StateSuccess =
+          StateSuccess->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+      C.addTransition(StateSuccess);
+    }
+  }
+
+  ProgramStateRef StateFailed;
+  StreamErrorState NewES;
+  std::tie(StateFailed, NewES) = Fn.GetFailureState(StreamSym, CE, ACtx, SVB);
+  if (!StateFailed)
+    return;
+  if (OldSS->ErrorState == ErrorFEof && NewES.FEof) {
+    assert(IsRead && "Stream write function must not produce EOF error.");
+    // If state was EOF before the call and it is possible to get EOF from this
+    // call, create always EOF error.
+    // (Read from an EOF stream results in EOF error, no other error.)
+    StateFailed = StateFailed->set<StreamMap>(
+        StreamSym, StreamState::getOpened(Desc, ErrorFEof, false));
+    C.addTransition(StateFailed);
+  } else {
+    // Assume that at any non-EOF read or write error the file position
+    // indicator for the stream becomes indeterminate.
+    StateFailed = StateFailed->set<StreamMap>(
+        StreamSym, StreamState::getOpened(Desc, NewES, NewES.FError));
+    if (OldSS->ErrorState != ErrorFEof && NewES.FEof)
+      C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+    else
+      C.addTransition(StateFailed);
+  }
+}
+
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, const Expr *StreamE,
                                    CheckerContext &C,

[balazske]

Later more functions can be simplified with evalRWCommon, probably in a next patch to make the review more simple.

[steakhal]

I'm yet to review the PR, but I would express my opinion on the ergonomics of the StreamChecker, as I've spent the last couple of days around it.

I find code duplication less harmful than unnatural generalization over small set of functions (I know, it's a hot take :D).
std::bind is harmful, and specific handlers should be preferred. As a rule of thumb, if we need a flag on a top-level API, then that's likely a code-smell.

E.g.: There presence of IsRead or IsSingleChar (that we need to bind), signals to me that there should be a specific API for the read, write, singleChar and buffer cases. Internally, those could dispatch to a generic call, which has this flag - but at least on the use-sites, we don't need to use std::bind, but naturally just taking the address of the member function without any tricks.

I'd also advise against using more callables bundled with CallDescriptions. They make debugging code more difficult, as the control-flow would become also data-dependent. I'd suggest other ways to generalize.
If I have some spare time in the next days, I'm going to also look deeper and come up with actual recommendations.

[NagyDonat]

I'm seconding the suggestions of @steakhal, and in particular I agree with

I'd also advise against using more callables bundled with CallDescriptions. They make debugging code more difficult, as the control-flow would become also data-dependent. I'd suggest other ways to generalize.

Instead of creating this shared framework that calls different callbacks depending on the checked function, consider keeping separate top-level evaluator functions that rely on a shared set of smaller tool-like helper functions.

[steakhal]

I'm seconding the suggestions of @steakhal, and in particular I agree with

I'd also advise against using more callables bundled with CallDescriptions. They make debugging code more difficult, as the control-flow would become also data-dependent. I'd suggest other ways to generalize.

Instead of creating this shared framework that calls different callbacks depending on the checked function, consider keeping separate top-level evaluator functions that rely on a shared set of smaller tool-like helper functions.

One more not about composability.
In general, addTransition calls don't compose, thus limits reusability.
For example, preReadWrite should be reusable from other precondition handlers, such that the new one can just call preReadWrite and then do some other more specialized checks. However, once a handler calls addTransition we are done. We no longer can simply wrap it, aka. compose with other functionalities.
Consequently, I'd suggest to have some common pattern, such as return state pointers, or NULL if an error was reported. And leave the addTransition to the caller - if they chose to call it.

eval* handlers are more difficult to compose, given that they also need to bind the return value and apply their sideffects, but I believe we could achieve similar objectives there too (thus, have refined, composable handlers, combined in useful ways).

[balazske]

My concern was the addition of many small functions that are used only from few other eval* calls and are relatively special. And all of these need a common big set of arguments like StreamSym, CE, Call. The build of states for success and failure cases in the thing that is really special for the calls.
But I can make another solution with "helper" functions.

[NagyDonat]

Yes, the "common big set of arguments" is an annoying problem. Perhaps you could consider the solution that I used in core.BitwiseShift where I introduced a "validator" class that holds all the common arguments as data members, so instead of helper functions you have helper methods that can all access the common arguments. (The checker callback constructs a validator, and calls its "main" method, which will call the others as needed)

[balazske]

For clarification, the evalRWCommon new function is not planned to be used in the same way as other eval* calls, the name may be misleading. This function contains a "generic algorithm" that is called from the other eval* calls, but not used directly in CallDescriptionMap. The old eval* calls and the evalRWCommon are not meant to be called from each other, except evalRWCommon from other eval* calls. I agree that using lambdas makes debugging more difficult and lambdas are not ideal for big parts of code and code formatting is not nice either.

[benshi001]

I do not like too long lambda either. In my opinion lambda should be short/compact.

And I do not think the redundancy is serious, except the common part

  ProgramStateRef State = C.getState();
  SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
  if (!StreamSym)
    return;

  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
  if (!CE)
    return;

  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
  if (!OldSS)
    return;

  assertStreamStateOpened(OldSS);

which appears in the beginning of most evalXX functions, and they are identical. Can we replace this piece with a macro ?

[benshi001]

For the part in the end of most evalXX functions,

StateFailed = ...
StateNotFailed = ...

They are quite similar but not identical, so we can generalize them with helper functions.

[NagyDonat]

LGTM, this is a nice little cleanup patch.