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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

You've sold me on `AnyError`, we should have something like that with the addition of `NoteTags`. With that said, I feel uneasy about adding it in this patch and not testing it properly. I can also sympathize with your anxiety against bloating the `fseek` patch further by simply moving the code there, but I'm just not sure there is a good way to avoid it. I have a suggestion:

1. Remove the code that adds and handles `AnyError`. Merge the two enums in the `StreamState`, and make `ensureSteamOpened` emit a warning if the stream is either `feof()` or `ferror()`. Add `NoteTags` saying something along the lines of:

  if (feof(F)) { // note: Assuming the condition is true
                 // note: Stream 'F' has the EOF flag set
    fgets(F); // warning: Illegal stream operation on a stream that has EOF set
  }

I think that would make this a very neat, self contained patch.

2. Add the removed code to the fseek patch (preferably with the name `UnknownError`). Make `ensureSteamOpened` emit a warning on this enum value as well. I don't think it would bloat the patch too much -- we just simply need that much to make sense of it.

What do you think?



================
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:
> 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 :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:44
+    OtherError, /// Other (non-EOF) error (`ferror` is true).
+    AnyError    /// EofError or OtherError, used if exact error is unknown.
+  } ErrorState = NoError;
----------------
balazske wrote:
> Szelethus wrote:
> > When do we know that the stream is in an error state, but not precisely know what that error is within the context of this patch?  `fseek` would indeed introduce such a state, as described in D75356#inline-689287, but that should introduce its own error `ErrorKind`. I still don't really get why we would ever need `AnyError`.
> This is the problem is the change is too small: The next part of it introduces functions where the new error flags are used. In this patch the AnyError feature is not used (the feof and ferror read it but nothing sets it).
Okay, after your comment in the other revision, I see how such a kind should be left, and the bug report should rather be enriched by `NoteTags`. With that said, `AnyError` is still a bit confusing as a name -- how abour `UnknownError`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+  bool isNoError() const { return ErrorState == NoError; }
+  bool isEofError() const { return ErrorState == EofError; }
----------------
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 :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+  bool isOtherError() const { return ErrorState == OtherError; }
+  bool isAnyError() const { return ErrorState == AnyError; }
+
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:355
+
+  if (SS->isNoError())
+    return;
----------------
balazske wrote:
> Szelethus wrote:
> > What if we call `clearerr()` on a stream that is in an `feof()` state? Shouln't we return if the stream is `!isOtherError()` (or `!isFError()`, if we were to rename it)?
> `clearerr` does not return any value. It only clears the error flags (sets to false). In my interpretation the stream has one error value associated with it, this value may be **EOF** or "other" error, or no error. There are not two error flags, only one kind of error that may be EOF or other.
> The `clearerr()` function clears the end-of file and error indicators for the given stream.

Yup, you're totally right on this one. My bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682