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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

It looks like for now there's no way to put the stream into an error state, right?



================
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.
----------------
balazske wrote:
> 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.
> 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.

Nope, there's also the state in which the stream was before it was opened, and it's different from being opened and closed. Most of the time we simply don't track the stream in such state, but when an open fails, we suddenly start tracking it, so we need a separate enum. Like, we'll have to emit different diagnostic messages depending on whether the stream is in `Closed` state or in an `OpenFailed` state and this information should be there at the bug report site, so it's part of the program state.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88
 struct FnDescription;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
----------------
`llvm::function_ref`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:112
+
+  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+  return C.getSValBuilder()
----------------
Simply `C.getLocationContext()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:366
+
+  State = State->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(State);
----------------
So what exactly happens when you're calling `clearerr` on a closed stream? Does it actually become opened? Also what happens when you're calling it on an open-failed stream?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:379-381
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
----------------
Maybe we should add a helper function for this, `CallEvent::getOriginCallExpr()`. That's an otherwise ugly function given that `CallEvent ` is a class hierarchy and the superclass doesn't need to know about subclasses, but most users do in fact only care about call-expressions and concise checker API is important. Same with `dyn_cast_or_null<FunctionDecl>(Call.getDecl())` that could be written as `Call.getFunctionDecl()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:388
+
+  DefinedSVal RetVal = makeRetVal(C, CE);
+
----------------
I recommend not introducing a new symbol when you're going to return 0 anyway. A plain 0 is easier on the constraint solver than a symbol constrained to 0.


================
Comment at: clang/test/Analysis/stream-error.c:33-34
+  int ch = fputc('a', F);
+  if (ch == EOF) {
+    // FIXME: fputc not handled by checker yet, should expect TRUE
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
----------------
Szelethus wrote:
> Sure, we don't, but the bigger issue here is that we wouldn't mark `F` to be in EOF even if we did handle it, we would also need to understand that `ch == EOF` is the telling sign. But that also ins't in the scope of this patch :)
Yeah, that's gonna be fun.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682