[Analyzer][StreamChecker] Changed representation of stream error state - NFC.

[balazske]

Summary

State of error flags for a stream is handled by having separate flags
that allow combination of multiple error states to be described with one
error state object.
After a failed function the error state is set in the stream state
and must not be determined later based on the last failed function
like before this change. The error state can not always be determined
from the last failed function and it was not the best design.

[Szelethus]

LGTM! Amazing job! Sorry for this dragging out for so long, but I'm very happy with how this patch and the overall direction turned out. I've left an annoying nit, and as always, feel free address it and commit without another round of review.

The error state can not always be determined from the last failed function and it was not the best design.

It was a great idea, still!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

40–45

I see what you mean, but in the context of the analyzer, I'd prefer to not use "there is an execution path with .*". Would this imply an a new branch in the exploded graph? Is this execution path modeled or does it only exist conceptually? I think we should avoid this ambiguity by saying something else, like "The steam may or may not have .* flag set".

[balazske]

There was a problem showing up in the buildbots, it should be fixed now.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include <functional>
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 namespace {
 
 struct FnDescription;
 
+/// State of the stream error flags.
+/// Sometimes it is not known to the checker what error flags are set.
+/// This is indicated by setting more than one flag to true.
+/// This is an optimization to avoid state splits.
+/// A stream can either be in FEOF or FERROR but not both at the same time.
+/// Multiple flags are set to handle the corresponding states together.
+struct StreamErrorState {
+  /// The stream can be in state where none of the error flags set.
+  bool NoError = true;
+  /// The stream can be in state where the EOF indicator is set.
+  bool FEof = false;
+  /// The stream can be in state where the error indicator is set.
+  bool FError = false;
+
+  bool isNoError() const { return NoError && !FEof && !FError; }
+  bool isFEof() const { return !NoError && FEof && !FError; }
+  bool isFError() const { return !NoError && !FEof && FError; }
+
+  bool operator==(const StreamErrorState &ES) const {
+    return NoError == ES.NoError && FEof == ES.FEof && FError == ES.FError;
+  }
+
+  StreamErrorState operator|(const StreamErrorState &E) const {
+    return {NoError || E.NoError, FEof || E.FEof, FError || E.FError};
+  }
+
+  StreamErrorState operator&(const StreamErrorState &E) const {
+    return {NoError && E.NoError, FEof && E.FEof, FError && E.FError};
+  }
+
+  StreamErrorState operator~() const { return {!NoError, !FEof, !FError}; }
+
+  /// Returns if the StreamErrorState is a valid object.
+  operator bool() const { return NoError || FEof || FError; }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddBoolean(NoError);
+    ID.AddBoolean(FEof);
+    ID.AddBoolean(FError);
+  }
+};
+
+const StreamErrorState ErrorNone{true, false, false};
+const StreamErrorState ErrorFEof{false, true, false};
+const StreamErrorState ErrorFError{false, false, true};
+
 /// Full state information about a stream pointer.
 struct StreamState {
   /// The last file operation called in the stream.
   const FnDescription *LastOperation;
 
   /// State of a stream symbol.
   /// FIXME: We need maybe an "escaped" state later.
   enum KindTy {
     Opened, /// Stream is opened.
     Closed, /// Closed stream (an invalid stream pointer after it was closed).
     OpenFailed /// The last open operation has failed.
   } State;
 
-  /// The error state of a stream.
-  /// Valid only if the stream is opened.
-  /// It is assumed that feof and ferror flags are never true at the same time.
-  enum ErrorKindTy {
-    /// No error flag is set (or stream is not open).
-    NoError,
-    /// EOF condition (`feof` is true).
-    FEof,
-    /// Other generic (non-EOF) error (`ferror` is true).
-    FError,
-    /// Unknown error flag is set (or none), the meaning depends on the last
-    /// operation.
-    Unknown
-  } ErrorState = NoError;
+  /// State of the error flags.
+  /// Ignored in non-opened stream state but must be NoError.
+  StreamErrorState ErrorState;
 
   bool isOpened() const { return State == Opened; }
   bool isClosed() const { return State == Closed; }
   bool isOpenFailed() const { return State == OpenFailed; }
 
-  bool isNoError() const {
-    assert(State == Opened && "Error undefined for closed stream.");
-    return ErrorState == NoError;
-  }
-  bool isFEof() const {
-    assert(State == Opened && "Error undefined for closed stream.");
-    return ErrorState == FEof;
-  }
-  bool isFError() const {
-    assert(State == Opened && "Error undefined for closed stream.");
-    return ErrorState == FError;
-  }
-  bool isUnknown() const {
-    assert(State == Opened && "Error undefined for closed stream.");
-    return ErrorState == Unknown;
-  }
-
   bool operator==(const StreamState &X) const {
-    // In not opened state error should always NoError.
+    // In not opened state error state should always NoError, so comparison
+    // here is no problem.
     return LastOperation == X.LastOperation && State == X.State &&
            ErrorState == X.ErrorState;
   }
 
-  static StreamState getOpened(const FnDescription *L) {
-    return StreamState{L, Opened};
-  }
-  static StreamState getOpened(const FnDescription *L, ErrorKindTy E) {
-    return StreamState{L, Opened, E};
+  static StreamState getOpened(const FnDescription *L,
+                               const StreamErrorState &ES = {}) {
+    return StreamState{L, Opened, ES};
   }
   static StreamState getClosed(const FnDescription *L) {
     return StreamState{L, Closed};
   }
   static StreamState getOpenFailed(const FnDescription *L) {
     return StreamState{L, OpenFailed};
   }
 
-  /// Return if the specified error kind is possible on the stream in the
-  /// current state.
-  /// This depends on the stored `LastOperation` value.
-  /// If the error is not possible returns empty value.
-  /// If the error is possible returns the remaining possible error type
-  /// (after taking out `ErrorKind`). If a single error is possible it will
-  /// return that value, otherwise unknown error.
-  Optional<ErrorKindTy> getRemainingPossibleError(ErrorKindTy ErrorKind) const;
-
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(LastOperation);
     ID.AddInteger(State);
     ID.AddInteger(ErrorState);
   }
 };
 
 class StreamChecker;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
 
 using ArgNoTy = unsigned int;
 static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
 
 struct FnDescription {
   FnCheck PreFn;
   FnCheck EvalFn;
   ArgNoTy StreamArgNo;
-  // What errors are possible after this operation.
-  // Used only if this operation resulted in Unknown state
-  // (otherwise there is a known single error).
-  // Must contain 2 or 3 elements, or zero.
-  llvm::SmallVector<StreamState::ErrorKindTy, 3> PossibleErrors = {};
 };
 
-Optional<StreamState::ErrorKindTy>
-StreamState::getRemainingPossibleError(ErrorKindTy ErrorKind) const {
-  assert(ErrorState == Unknown &&
-         "Function to be used only if error is unknown.");
-  llvm::SmallVector<StreamState::ErrorKindTy, 3> NewPossibleErrors;
-  for (StreamState::ErrorKindTy E : LastOperation->PossibleErrors)
-    if (E != ErrorKind)
-      NewPossibleErrors.push_back(E);
-  if (NewPossibleErrors.size() == LastOperation->PossibleErrors.size())
-    return {};
-  if (NewPossibleErrors.size() == 1)
-    return NewPossibleErrors.front();
-  return Unknown;
-}
-
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
   assert(Desc && Desc->StreamArgNo != ArgNone &&
          "Try to get a non-existing stream argument.");
   return Call.getArgSVal(Desc->StreamArgNo);
 }
 
public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 
   /// If true, evaluate special testing stream functions.
   bool TestMode = false;
 
 private:
   CallDescriptionMap<FnDescription> FnDescriptions = {
-      {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone, {}}},
+      {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"freopen", 3},
-       {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2, {}}},
-      {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone, {}}},
+       {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
+      {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"fclose", 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0, {}}},
-      {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3, {}}},
-      {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3, {}}},
-      {{"fseek", 3},
-       {&StreamChecker::preFseek,
-        &StreamChecker::evalFseek,
-        0,
-        {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-      {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}},
-      {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}},
-      {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0, {}}},
-      {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0, {}}},
+       {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
+      {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}},
+      {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}},
+      {{"fseek", 3}, {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
+      {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0}},
+      {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0}},
+      {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}},
+      {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"clearerr", 1},
-       {&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0, {}}},
-      // Note: feof can result in Unknown if at the call there is a
-      // PossibleErrors with all 3 error states (including NoError).
-      // Then if feof is false the remaining error could be FError or NoError.
+       {&StreamChecker::preDefault, &StreamChecker::evalClearerr, 0}},
       {{"feof", 1},
        {&StreamChecker::preDefault,
-        &StreamChecker::evalFeofFerror<StreamState::FEof>,
-        0,
-        {StreamState::FError, StreamState::NoError}}},
-      // Note: ferror can result in Unknown if at the call there is a
-      // PossibleErrors with all 3 error states (including NoError).
-      // Then if ferror is false the remaining error could be FEof or NoError.
+        std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof),
+        0}},
       {{"ferror", 1},
        {&StreamChecker::preDefault,
-        &StreamChecker::evalFeofFerror<StreamState::FError>,
-        0,
-        {StreamState::FEof, StreamState::NoError}}},
-      {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}},
+        std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFError),
+        0}},
+      {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}},
   };
 
   CallDescriptionMap<FnDescription> FnTestDescriptions = {
       {{"StreamTesterChecker_make_feof_stream", 1},
-       {nullptr, &StreamChecker::evalSetFeofFerror<StreamState::FEof>, 0}},
+       {nullptr,
+        std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof),
+        0}},
       {{"StreamTesterChecker_make_ferror_stream", 1},
-       {nullptr, &StreamChecker::evalSetFeofFerror<StreamState::FError>, 0}},
+       {nullptr,
+        std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
+                  ErrorFError),
+        0}},
   };
 
   void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
   void preFreopen(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
   void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
                    CheckerContext &C) const;
 
   void evalFclose(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
   void preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
   void evalFseek(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
   void preDefault(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
   void evalClearerr(const FnDescription *Desc, const CallEvent &Call,
                     CheckerContext &C) const;
 
-  template <StreamState::ErrorKindTy ErrorKind>
   void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call,
-                      CheckerContext &C) const;
+                      CheckerContext &C,
+                      const StreamErrorState &ErrorKind) const;
 
-  template <StreamState::ErrorKindTy EK>
   void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call,
-                         CheckerContext &C) const;
+                         CheckerContext &C,
+                         const StreamErrorState &ErrorKind) 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
   /// to be non-null.
   ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                       ProgramStateRef State) const;
 
void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   ProgramStateRef StateNotFailed, StateFailed;
   std::tie(StateFailed, StateNotFailed) =
       C.getConstraintManager().assumeDual(State, RetVal);
 
   // Reset the state to opened with no error.
   StateNotFailed =
       StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
   // We get error.
+  // It is possible that fseek fails but sets none of the error flags.
   StateFailed = StateFailed->set<StreamMap>(
-      StreamSym, StreamState::getOpened(Desc, StreamState::Unknown));
+      StreamSym,
+      StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError));
 
   C.addTransition(StateNotFailed);
   C.addTransition(StateFailed);
 }
 
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
     return;
 
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   if (!SS)
     return;
 
   assertStreamStateOpened(SS);
 
   State = State->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
   C.addTransition(State);
 }
 
-template <StreamState::ErrorKindTy ErrorKind>
 void StreamChecker::evalFeofFerror(const FnDescription *Desc,
-                                   const CallEvent &Call,
-                                   CheckerContext &C) const {
+                                   const CallEvent &Call, CheckerContext &C,
+                                   const StreamErrorState &ErrorKind) 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;
 
-  auto AddTransition = [&C, Desc, StreamSym](ProgramStateRef State,
-                                             StreamState::ErrorKindTy SSError) {
-    StreamState SSNew = StreamState::getOpened(Desc, SSError);
-    State = State->set<StreamMap>(StreamSym, SSNew);
-    C.addTransition(State);
-  };
-
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   if (!SS)
     return;
 
   assertStreamStateOpened(SS);
 
-  if (!SS->isUnknown()) {
-    // The stream is in exactly known state (error or not).
-    // Check if it is in error of kind `ErrorKind`.
-    if (SS->ErrorState == ErrorKind)
-      State = bindAndAssumeTrue(State, C, CE);
-    else
-      State = bindInt(0, State, C, CE);
-    // Error state is not changed in the new state.
-    AddTransition(State, SS->ErrorState);
-  } else {
-    // Stream is in unknown state, check if error `ErrorKind` is possible.
-    Optional<StreamState::ErrorKindTy> NewError =
-        SS->getRemainingPossibleError(ErrorKind);
-    if (!NewError) {
-      // This kind of error is not possible, function returns zero.
-      // Error state remains unknown.
-      AddTransition(bindInt(0, State, C, CE), StreamState::Unknown);
-    } else {
-      // Add state with true returned.
-      AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind);
-      // Add state with false returned and the new remaining error type.
-      AddTransition(bindInt(0, State, C, CE), *NewError);
-    }
+  if (SS->ErrorState & ErrorKind) {
+    // Execution path with error of ErrorKind.
+    // Function returns true.
+    // From now on it is the only one error state.
+    ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE);
+    C.addTransition(TrueState->set<StreamMap>(
+        StreamSym, StreamState::getOpened(Desc, ErrorKind)));
+  }
+  if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) {
+    // Execution path(s) with ErrorKind not set.
+    // Function returns false.
+    // New error state is everything before minus ErrorKind.
+    ProgramStateRef FalseState = bindInt(0, State, C, CE);
+    C.addTransition(FalseState->set<StreamMap>(
+        StreamSym, StreamState::getOpened(Desc, NewES)));
   }
 }
 
 void StreamChecker::preDefault(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SVal StreamVal = getStreamArg(Desc, Call);
   State = ensureStreamNonNull(StreamVal, C, State);
   if (!State)
     return;
   State = ensureStreamOpened(StreamVal, C, State);
   if (!State)
     return;
 
   C.addTransition(State);
 }
 
-template <StreamState::ErrorKindTy EK>
 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
-                                      const CallEvent &Call,
-                                      CheckerContext &C) const {
+                                      const CallEvent &Call, CheckerContext &C,
+                                      const StreamErrorState &ErrorKind) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   assert(SS && "Stream should be tracked by the checker.");
-  State = State->set<StreamMap>(StreamSym,
-                                StreamState::getOpened(SS->LastOperation, EK));
+  State = State->set<StreamMap>(
+      StreamSym, StreamState::getOpened(SS->LastOperation, ErrorKind));
   C.addTransition(State);
 }
 
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                    ProgramStateRef State) const {
   auto Stream = StreamVal.getAs<DefinedSVal>();
   if (!Stream)