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

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

balazske marked 6 inline comments as done.
balazske added a comment.

The problem here with tests is that there is no function to call that sets the error flags `FEof` or `FError`. So it is only possible to check if these are not set after opening the file.



================
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.
----------------
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.


================
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.
----------------
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`.


================
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;
----------------
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).


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:355
+
+  if (SS->isNoError())
+    return;
----------------
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.


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682