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

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

balazske marked 4 inline comments as done.
balazske added a comment.

An improvement can be made if the check runs only when size of `char` and `int` are equal. And it is possible to avoid the warning if `fgetc` is called after `fgetc` (with no functions in between). (More complicated thing is to test for loop construct and maybe not more efficient.)

The code can be improved somewhat but there are now too many special cases and there is already a default eval function. Probably it is better to have every "check" function take and return `ExplodedNode` instead of state.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:255-261
+  if (EOFv == 0) {
+    if (const llvm::Optional<int> OptInt =
+            tryExpandAsInteger("EOF", C.getPreprocessor()))
+      EOFv = *OptInt;
+    else
+      EOFv = -1;
+  }
----------------
Szelethus wrote:
> I'm 99% sure that the `Preprocessor` is retrievable in the checker registry function (`register*Checker`), and this code should be moved there, and `EOFv ` be made non-mutable. 
I am not sure if this would be good. The `Preprocessor` is not available easily and the "register" function is probably there to register a checker and do nothing else. Probably the source code is not even known when it is called. And how to pass the EOF value to the created checker in the register function (if not a static variable is used)?


================
Comment at: clang/test/Analysis/stream.c:182
+
+void check_fgetc_error() {
+  FILE *fp = fopen("foo.c", "r");
----------------
martong wrote:
> Do you have tests for `getc` as well? Maybe we could have at least one for getc too.
`getc` is not done yet. It can need more change in `StreamChecker` to handle functions on std streams (at least stdin).


================
Comment at: clang/test/Analysis/stream.c:188
+    C = fgetc(fp);
+    fread(0, 0, 0, fp); // expected-warning {{Should call}}
+    fread(0, 0, 0, fp);
----------------
Szelethus wrote:
> Are we sure that this is the error message we want to emit here? Sure, not checking whether the stream is in an error state is a recipe for disaster, but this seems to clash with
> > Should call 'feof' and 'ferror' after failed character read.
> as error here is not even checking the return value. Shouldn't we say
> > Should check the return value of `fgetc` before ...
> instead?
No, it is not enough to check the return value of `fgetc`, exactly this is the reason for the checker. "Should call 'feof' and 'ferror' after failed character read." is more exact: The read was suspected to fail because EOF was returned but should call feof (...) to verify it. A better message can be:

If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.


================
Comment at: clang/test/Analysis/stream.c:246-247
+    if (C == EOF) {
+      feof(fp);
+      ferror(fp);
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045