[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

Szelethus added a comment.

Thanks for sticking this out! It just takes me a while to get to the mental space regarding this patch you are already in, sometimes through saying incorrect statements or stupid suggestions! :) If others could chip in in this discussion, it would be most appreciated, because I fear that otherwise this patch is a bit too exposed to my own wrongs.

In D75682#1918839 <https://reviews.llvm.org/D75682#1918839>, @balazske wrote:

> In D75682#1917257 <https://reviews.llvm.org/D75682#1917257>, @Szelethus wrote:
>
> > Could you please add the warning we discussed? That would be a great demonstration of this patch's capabilities, and wouldn't deserve its own patch.
>
>
> Was it this warning?
>
>   if (feof(F)) { // note: Assuming the condition is true
>                  // note: Stream 'F' has the EOF flag set
>     fgets(F); // warning: Illegal stream operation on a stream that has EOF set
>   }
>
>
> The warning above should be get even if the `feof` call is missing because it is still possible that the previous operation resulted in EOF state.


What I meant to demonstrate (but failed to describe) is that if the condition of the `if` statement is `!feof(F)`, we **shouldn't** get the warning.

> The checker does not work by "assuming feof is true" (this can be a later feature, if the `F` is not tracked and a feof call is encountered). Value of `feof` (more precisely: if any error happened) is fixed when the file operation is called that causes the error. [...] And still that warning would not be testable because `FEof` error state is never set in this patch.

I suspect that at this point you are 2 steps ahead of me, but this is how I would imagine such a patch to look like: we //evaluate// `feof()` by splitting the state (this should be worth the cost, because why would we ever call `feof` if the state couldn't be  EOF or non-EOF?), associate the state where its return value is true with the argument being marked as EOF, and the other as non-EOF.

This obviously wouldn't be sufficient -- values retrieved from stream reading operations may be EOF themselves, which should make us set the appropriate state for the originating stream, but that should indeed be the topic of a followup patch.

What is the goal here if not this? I have a suspicion I'm just not seeing the obvious here, but I don't get it just yet.

> By the way, a file read is not invalid if any error is there, it simply fails again.

Oh, yea, silly me. You're totally right :)

> Probably it would be better to make a full working prototype first, because the design decisions made now can turn out to be wrong later when the new functionality would be added and we do "not see the full picture" now.

If you try to implement a warning just to see how this would turn out long term, that would indeed be a good idea. Feel free to upload them, if you prefer, as a WIP patch!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:393-394
+
+  // Make the return value accordingly to the error.
+  State = State->assume(RetVal, (SS->*IsOfError)());
+  assert(State && "Return value should not be constrained already.");
----------------
Is this correct? We set the `feof()` return value whether we know the state of the stream to be EOF, but in this case we would incorrectly mark it as non-EOF: 
```lang=bash
$ cat a.txt # contains a single newline

$ cat test.cpp 
```
```lang=cpp
#include <cstdio>

void tryRead(FILE *F) {
  char C = fgetc(F);

  if (feof(F))
    printf("The stream is EOF!\n");
  else
    printf("The stream is good! Read '%c'\n", C);
}

int main() {
  FILE *F = fopen("a.txt", "r");
  tryRead(F);
  tryRead(F); // Incorrectly mark F to be non-EOF
}
```
```lang=bash
$ clang test.cpp -fsanitize=address && ./a.out 
The stream is good! Read '
'
The stream is EOF!
```

Wouldn't it be safer to assume it to be `true` if we **know** its EOF and do nothing otherwise? How about a state split?

(I know this functions also handler FError, but you see what my point is.)


================
Comment at: clang/test/Analysis/stream-error.c:33-34
+  int ch = fputc('a', F);
+  if (ch == EOF) {
+    // FIXME: fputc not handled by checker yet, should expect TRUE
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
----------------
Sure, we don't, but the bigger issue here is that we wouldn't mark `F` to be in EOF even if we did handle it, we would also need to understand that `ch == EOF` is the telling sign. But that also ins't in the scope of this patch :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682