[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

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

balazske marked an inline comment as done.
balazske added a comment.

So, I think a different approach can be to store the stream state for the stream instead of `ShouldCallX` variables. (The state can be no-error, eof-error, other-error.) Optionally a warning can be made if any (modeled) stream function is called and the stream is in error state (and handle feof and ferror specially). (This is optional because it is not necessary to check the error always, it should not cause crash or undefined behavior. If a function is called in already failed stream state, it should simply fail again, or even not. Retrying a read for example can be correct. To detect if there is any error checking in the code the "ErrorReturnChecker" in https://reviews.llvm.org/D72705 is the better solution.) A special case is if the last called function was `getc` and the stream is in error state, here we know that it is necessary to call `ferror` or `feof`.



================
Comment at: clang/test/Analysis/stream.c:246-247
+    if (C == EOF) {
+      feof(fp);
+      ferror(fp);
+    }
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > I bet this isn't how we envision how these function to be used. Maybe `ReturnValueChecker` could be deployed here? In any case, that would be a topic of a different revision.
> > If `fgetc` returns non-EOF we can be sure that there is no error, so no need to call `ferror` and `feof`. If it returns EOF we must "double-check" that it was a real error or an EOF-looking character that was read.
> Yea, but shouldn't we check the return values of `ferror` and `feof`? Otherwise whats the point?
Yes but this is job of an other checker (return value not used). This here is to ensure that the functions are called at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045