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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
