[PATCH] D106262: [clang][analyzer] Use generic note tag in alpha.unix.Stream .

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

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:696
   // Add transition for the failed state.
   Optional<NonLoc> RetVal = makeRetVal(C, CE).castAs<NonLoc>();
   assert(RetVal && "Value should be NonLoc.");
----------------
balazske wrote:
> Szelethus wrote:
> > Lets leave a TODO here, before we forget it:
> > [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.1.2]], Description of `fread`:
> > > If a partial element is read, its value is indeterminate.
> This means that the (content of the) buffer passed to `fread` should become in a "uninitialized" (undefined) state?
On the return value:
> The fread function returns the number of elements successfully read, which may be less than nmemb if a read error or end-of-file is encountered.
So I guess only the (return value + 1)th element of the array is indeterminate.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:726-729
+                        "Stream reaches end-of-file or operation fails here"));
   else
-    C.addTransition(StateFailed);
+    C.addTransition(StateFailed, constructFailureNoteTag(
+                                     C, StreamSym, "Operation fails here"));
----------------
balazske wrote:
> Szelethus wrote:
> > We can be more specific here. While the standard doesn't explicitly specify that a read failure could result in ferror being set, it does state that the file position indicator will be indeterminate:
> > 
> > [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.1.2]], Description of `fread`:
> > > If an error occurs, the resulting value of the file position indicator for the stream is indeterminate.
> > 
> > [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.2.2]], Description of `fwrite`:
> > > If an error occurs, the resulting value of the file position indicator for the stream is indeterminate.
> > 
> > Since this is the event to highlight, I'd like to see it mentioned. How about:
> > > Stream either reaches end-of-file, or fails and has its file position indicator left indeterminate, or the error flag set.
> > > After this operation fails, the stream either has its file position indicator left indeterminate, or the error flag set.
> > 
> > Same for any other case where indeterminate file positions could occur.
> For the `fread` and `fwrite` cases, I think that the error flag **and** the indeterminate position is always set if error occurs. It looks more natural to tell the user that "the operation fails" than "file position becomes indeterminate". And the user could see that the operation fails and file position is "indeterminate" from the error reports, the failure causes the indeterminate (or "undefined"?) position.
> 
> Only the `fseek` is where indeterminate position can appear without setting the ferror flag (but the failure is discoverable by checking the return value of `fseek`). Still the cases "operation fails" (set ferror flag and/or leave file position indeterminate, return nonzero) and "stream reaches end-of-file" are the ones that are possible. The checker documentation can contain more exactly why the checker works this way.
Well, to me, seeing both the error flag and the file position indicator being mentioned here sounds nice, since we are already in the possession of that information. How about

>Stream either reaches end-of-file, or fails and has its file position indicator left indeterminate and the error flag set.
>After this operation fails, the stream either has its file position indicator left indeterminate and the error flag set.
?

> The checker documentation can contain more exactly why the checker works this way.
I think adding this bit about the file position indicator shouldn't be in the docs only, though explaining the schrödinger-like behaviour in there would be nice.

>I think that the error flag and the indeterminate position is always set if error occurs.

This means that we need to make our `ferror` in the future smarter. Can you leave a TODO about that `ferror` needs to check what was the last stream operation that may have failed? In the case where it was an `fread`/`fwrite`, on its false branch, we need to clear both ferror and the file position indocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262