[PATCH] D80018: [Analyzer][StreamChecker] Added check for "indeterminate file position".

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+  /// This value applies to all error states in ErrorState except FEOF.
+  /// An EOF+indeterminate state is the same as EOF state.
+  bool FilePositionIndeterminate = false;
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > What does this mean? "An EOF+indeterminate state is the same as EOF state." I don't understand the message you want to convey here -- is it that we cannot have an indeterminate file position indicator if we hit EOF, hence we regard a stream that is **known** to be EOF to have its file position indicator determinate?
> > > > 
> > > > A good followup question for the uninitiated would be that "Well why is it ever legal to construct a `StreamState` object that can both have the `FilePositionIndeterminate` set to true and the `ErrorState` indicate that the steam is **known** to be in EOF?" Well, the answer is that we may only realize later that the error state can only be EOF, like in here:
> > > > ```lang=c++
> > > > void f() {
> > > >  FILE *F = fopen(...);
> > > >  if (fseek(F, ...)) {
> > > >     // Could be either EOF, ERROR, and ofc indeterminate
> > > >     if (eof(F)) {
> > > >       // This is where we have a seemingly impossible stream state, but its not a programming error, its a design decision.
> > > >     }
> > > > }
> > > > ```
> > > > This might warrant a bit on explanation either here, or in `ensureNoFilePositionIndeterminate`. Probably the latter.
> > > > 
> > > > With that said, can `SteamState`'s constructor ensure that we do not create a known to be EOF stream that is indeterminate?
> > > Actually, not enforcing this could leave to false positives:
> > > 
> > > ```
> > > void f() {
> > >  FILE *F = fopen(...);
> > >  if (fseek(F, ...)) {
> > >     // Could be either EOF, ERROR, and ofc indeterminate
> > >     if (eof(F)) {
> > >       clearerr(F);
> > >       fseek(F, ...); // false positive warning
> > >     }
> > > }
> > > ```
> > The comment wants to say only that if the **ErrorState** contains the **ErrorFEof** the value of `filePositionIndeterminate` is to be ignored for the EOF case. If the file is in EOF it does not matter what value `filePositionIndeterminate` has. The cause for this handling is that ErrorState handles multiple possible errors together but the indeterminate position does not apply to all. If **ErrorState** contains **ErrorFEof** and **ErrorFError** together and the `filePositionIndeterminate` is set, the position is not indeterminate in the EOF case. For EOF case we should know that the position is at the end of the file, not indeterminate.
> > 
> > Another solution for this problem can be to have a "ErrorFErrorIndeterminate" and "ErrorNoneIndeterminate" error type but this makes things more difficult to handle.
> What do you mean under the term "the EOF case"? When we **know** the stream to only be in the EOF state? The overall modeling seems correct, its just that little corner case that if we **know** that the stream hit EOF, the file position must be determinate.
The "difficult" case is when we do not know if the stream is in error or EOF state. We know only that it is in one of these. And the `FilePositionIndeterminate` is set to true. In this state `FEof` and `FError` are true in `ErrorState`. If the program should know what the error is, it needs to generate 2 new states, one with only `FEof` true and other with `FError`. And how to set the `FilePositionIndeterminate` for the new states? For `FError` it must be set to true because it was true before, for `FEof` it is set to false because by definition it is not applied to `FEof` case. (This last is //the EOF case// above.)

This table shows what (real) stream state is represented (values at right) by what values in `StreamState` (values at left side):
| FEof | FError | FilePositionIndeterminate | stream feof? | stream ferror? | position indeterminate? |
| false | false | true | false | false | true |
| true | false | true | true | false | false |
| false | true | true | false | true | true |
| true | true | true | - | - | - |


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80018