[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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

Szelethus added a comment.

In D78374#2033826 <https://reviews.llvm.org/D78374#2033826>, @balazske wrote:

> I think this is a expected way of how it works, failure of a stream operation does not necessarily depend on result of a previous operation. So any operation can fail or not, independent of the previous error state (the "ferror" may happen because a temporary disk error and it is worth to retry the read, at least a non-`fread` function where the "indeterminate file position" problem does not happen). This would make things more simple. Only the EOF is probably exception, in EOF state any read operation fails with EOF.


Right. But, speaking about generality, would it be reasonably to assume that all (or almost all) operations may also result in an indeterminate file indicator? Because if so, we could, and totally should strip those changes from the fread/fwrite ones and make a new revision.

That is by the way true for a lot of other changes, such as the delayed state split or the new error state struct. I mean not to burden you by micromanaging a lot of small patches, but it makes reviewing far faster for me.

> (But I do not know if it is possible that a stream in EOF state becomes non-EOF because new data appears at the end, if other program can write into it or it is some form of "pipe" or special file. If this is true even EOF needs no special handling.)

That is a great question, but I strongly believe that it cannot without `freopen`. I mean, if I were to create an io library, I would set a pointer to the beginning and the end of the file, and know we hit eof is the current pointer hits the end point. Besides, how could anyone change the `FILE *` object in a non-analyzable way?

In any case, its a reasonable to assume it won't change.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:937-939
+        new BuiltinBug(this, "Stream is in error state",
+                       "Function called when stream has already error state "
+                       "flag (EOF or error) set."));
----------------
Since this isn't a bug but rather a code smell, a note would be a nice compliment.
"Use clearerr() to clear the error flag"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374