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:
> > 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 | - | - | - |
Right. How about we make this field private and create a an
`isFilePositionIndeterminate()` method that ensures that for the second row of
that table we always return false? That would take some responsibility off of
this interface's users.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:841-842
+ // FEof is ignored).
+ if (SS->ErrorState.isFEof())
+ return State->set<StreamMap>(Sym, NewState);
+
----------------
This branch could be removed in its entirety.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:854
+ }
+ return State; // FIXME: nullptr?
+ }
----------------
Szelethus wrote:
> Good question. It would make sense... @NoQ, @xazax.hun?
Well, looking at other checkers, they usually early return and don't modify the
state if the creation of the error node failed, so a nullptr return would be
more appropriate.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:857-868
+ // FEof and other states are possible.
+ // The path with FEof is the one that can continue.
+ // For this reason a non-fatal error is generated to continue the analysis
+ // with only FEof state set.
+ ExplodedNode *N = C.generateNonFatalErrorNode(State);
+ if (N) {
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
Szelethus wrote:
> Ugh, tough one. I think this just isn't really *the* error to highlight here,
> but rather that its bad practice to not check on the stream after an
> operation. But I'm willing to accept I might be wrong here.
Actually, I take this back. If we had `NoteTag`s to explain that "this stream
operation failed and left the stream file position indication in an
indeterminate state", this would be great.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80018/new/
https://reviews.llvm.org/D80018
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
