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

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

Szelethus marked an inline comment as done.
Szelethus 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;
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:306
+  ProgramStateRef
+  ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+                                    ProgramStateRef State) const;
----------------
balazske wrote:
> Szelethus wrote:
> > Ooooor `ensureFilePositionDeterminate`? :D
> It is better to call "Invalid" or "Unknown" position, "indeterminate" is taken from the text of the C standard. I think "indeterminate" is a special name here that is better to have in this form always.
Sure, I'm sold.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80018