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

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

balazske marked 2 inline comments as done.
balazske 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.");
----------------
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?


================
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"));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262