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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 9 inline comments as done.
balazske 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.
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > balazske wrote:
> > > xazax.hun wrote:
> > > > baloghadamsoftware wrote:
> > > > > 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.
> > > > In a more expressive language each enum value could have parameters and we could have
> > > > ```
> > > > Opened(ErrorKind),
> > > > Closed,
> > > > OpenFailed
> > > > ```
> > > > 
> > > > While we do not have such an expressive language, we can simulate this using the current constructs such as a variant. The question is, does this worth the effort? At this point this is more like the matter of taste as long as it is properly documented. 
> > > Have a `bool isOpened` and an error kind is better?
> > That sounds wonderful, @balazske! :)
> Yes, we do not need the `OpenFailed` state either. A stream is either opened or not. This is the //state// of the stream. If there is any error, there are the //error codes// for.
The "error kind" concept is needed separately from the stream state, see D75851. This is a reason for not merging them.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+
+  if (SS->isAnyError()) {
+    // We do not know yet what kind of error is set.
----------------
baloghadamsoftware wrote:
> 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.
This is not applicable any more, `AnyError` is removed (from here).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+        StreamSym, StreamState::getOpenedWithOtherError());
+    C.addTransition(StateEof);
+    C.addTransition(StateOther);
----------------
Szelethus wrote:
> balazske wrote:
> > baloghadamsoftware wrote:
> > > 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.
> > `feof` and `ferror` should return the same value if called multiple times (and no other file operations in between). If the exact error is not saved in the state, how can we ensure that the calls return the same value?
> This is a very interesting question indeed, never crossed my mind before. I think it would be better to move `AnyError` to the next revision and discuss it there.
The state fork problem is solved with `UnknownError` is the later revision, here it is no problem any more. A single state split is always needed because the return values differ in the failed and success case, and I want to make correlation between return values and stream error state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682