[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  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.
----------------
xazax.hun wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Hmm, now that I think of it, could we just merge these 2 enums? Also, I fear that indexers would accidentally assign the comment to the enum after the comma:
> > > > 
> > > > ```lang=cpp
> > > >     Opened, /// Stream is opened.
> > > >     Closed, /// Closed stream (an invalid stream pointer after it was closed).
> > > >     OpenFailed /// The last open operation has failed.
> > > > ```
> > > > ` /// Stream is opened` might be assigned to `Closed`. How about this:
> > > > ```lang=cpp
> > > >     /// Stream is opened.
> > > >     Opened,
> > > >     /// Closed stream (an invalid stream pointer after it was closed).
> > > >     Closed,
> > > >     /// The last open operation has failed.
> > > >     OpenFailed
> > > > ```
> > > Probably these can be merged, it is used for a stream that is in an indeterminate state after failed `freopen`, but practically it is handled the same way as a closed stream. But this change would be done in another revision.
> > I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and the fields associated with them (`State` and `ErrorState`). We could however merge `Closed` and `OpenFailed`, granted that we put a `NoteTag` to `evalFreopen`. But I agree, that should be another revision's topic :)
> Since you mentioned that ErrorState is only applicable to Open streams, I am also +1 on merging the enums. These two states are not orthogonal, no reason for them to be separate.
Not orthogonal, but rather hiearchical. That is a reason for not merging. I am completely against it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:43
+    EofError,   /// EOF condition (`feof` is true).
+    OtherError, /// Other (non-EOF) error (`ferror` is true).
+    AnyError    /// EofError or OtherError, used if exact error is unknown.
----------------
balazske wrote:
> Szelethus wrote:
> > Shouldn't we call this `FError` instead, if this is set precisely when `ferror()` is true? Despite the comments, I still managed to get myself confused with it :)
> Plan is to rename to `NoError`, `FEofError`, `FErrorError`, `FEofOrFErrorError`.
If would suggest  `NoError`, `FEoF`, `FError`, `FEoFOrFError`. Too many `Error` suffixes reduce the readability.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+  bool isNoError() const { return ErrorState == NoError; }
+  bool isEofError() const { return ErrorState == EofError; }
----------------
balazske wrote:
> Szelethus wrote:
> > If a stream's opening failed, its still an error, yet this getter would return true for it. I think this is yet another reason to just merge the 2 enums :)
> But as the comment says, the error state is applicable only in the opened state. If not opened, we may not call the "error" functions at all (assertion can be added). Anyway it is possible to merge the enums, then the `isOpened` will be a bit difficult (true if state is one of `OpenedNoError`, `OpenedFErrorError`, `OpenedFEofError`, `OpenedFEofOrFErrorError`). Logically it is better to see that the state is an aggregation of two states, an "openedness" and an error state. Probably this does not matter much here because it is unlikely that new states will appear that make the code more complex.
I would not merge the two enums: this way they are hierarchical. When only checking the openness we do not need to check for 4 different states. I completely agree with @balazske here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+  bool isOtherError() const { return ErrorState == OtherError; }
+  bool isAnyError() const { return ErrorState == AnyError; }
+
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This is confusing -- I would expect a method called `isAnyError()` to return true when `isNoError()` is false.
> > The error state kinds are not mutually exclusive. The "any error" means we know that there is some error but we do not know what the error is (it is not relevant because nothing was done that depends on it). If a function is called that needs to know if "ferror" or "feof" is the error, the state will be split to `FErrorError` and `FEofError`.
> How about we change the name of it to `isUnknownError`? My main issue is that to me, the name `isAnyError()` answeres the question whether the stream is in any form of erroneous state.
Yes, "Any" usually means //any// of the possible kinds. But instead of `UnknownError` use `FEoFOrFerror`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+
+  if (SS->isAnyError()) {
+    // We do not know yet what kind of error is set.
----------------
Szelethus wrote:
> Does this ever run? We never actually set the stream state to `AnyError`.
I suggest to move codes that are only executed after a new functionality is added in a subsequent patch to that patch.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+        StreamSym, StreamState::getOpenedWithOtherError());
+    C.addTransition(StateEof);
+    C.addTransition(StateOther);
----------------
xazax.hun wrote:
> As you probably know we are splitting the execution paths here. This will potentially result in doubling the analysis tome for a function for each `feof` call. Since state splits can be really bad for performance we need good justification for each of them.
> So the questions are:
> * Is it really important to differentiate between eof and other error or is it possible to just have an any error state?
> * Do we find more bugs in real world code that justifies these state splits?
I agree here. Do not introduce such forks without specific reason.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:404-405
+
+void StreamChecker::evalFerror(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
----------------
balazske wrote:
> Szelethus wrote:
> > This function is practically the same as `evalFeof`, can we merge them?
> It is not the same code ("EOF" and "other error" are interchanged), but can be merged somehow.
Maybe using a template with a functor parameter and pass a lambda in the two instantiations? (See `DebugContainerModeling` and `DebugIteratorModeling`) and also some example is `Iterator.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682