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

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

balazske marked 8 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37-40
+  // NoError: No error flag is set or stream is not open.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > These too. Also, I'm not yet sure whether we need `OtherError` and `AnyError`, as stated in a later inline.
> > I plan to use `AnyError` for failing functions that can have **EOF** and other error as result. At a later `ferror` or `feof` call a new state split follows to differentiate the error (instead of having to add 3 new states after the function, for success, EOF error and other error). If other operation is called we know anyway that some error happened.
> I think it would still be better to introduce them as we find uses for them :) Also, to stay in the scope of this patch, shouldn't we only introduce `FseekError`? If we did, we could make warning messages a lot clearer.
This change is generally about introduction of the error handling, not specifically about `fseek`. Probably not `fseek` was the best choice, it could be any other function. Probably I can add another, or multiple ones, but then again the too-big-patch problem comes up. (If now the generic error states exist the diffs for adding more stream operations could be more simple by only adding new functions and not changing the `StreamState` at all.) (How is this related to warning messages?)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:351-354
+  // Set state to any error.
+  // Assume that EOF and other error is possible after the call.
+  StateFailed =
+      StateFailed->set<StreamMap>(StreamSym, StreamState::getOpenedWithError());
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > But why? The standard suggests that the error state isn't changed upon failure. I think we should leave it as-is.
> > The fseek can fail and set the error flag, see the example code here:
> > https://en.cppreference.com/w/c/io/fseek
> > Probably it can not set the **EOF** flag, according to that page it is possible to seek after the end of the file. So the function can succeed or fail with `OtherError`.
> Yup, right, you won :) I tried some examples out on my system, and it could preserve or change the error state of the stream. To me it seems like that not checking the state of the stream after a failed `fseek` is surely unadvised.
> 
> This still should be `AnyError` (or `FseekError`), as according to the `OtherError`'s description `OtherError` may not refer to `EOF`, yet after a failed `fseek` call the stream can be in `EOF`:
> 
> ```lang=bash
> $ cat test.cpp
> ```
> ```lang=cpp
> #include <cstdio>
> 
> int main() {
>   FILE *F = fopen("test.cpp", "r");
> 
>   while (EOF != fgetc(F)) {}
> 
>   if (feof(F))
>     printf("The file is closed!\n");
> 
>   // Return value is non-zero on failure.
>   if (fseek(F, -100000, SEEK_END)) {
>     if (feof(F))
>       printf("The file is still closed!\n");
>     else
>       printf("The file is no longer closed!\n");
>   }
> }
> ```
> ```lang=bash
> $ clang test.cpp && ./a.out
> The file is closed!
> The file is still closed!
> ```
After some experimenting I think it is best to have every possibility of errors after `fseek`. This  means, either **EOF**, or other-error, or non-**EOF** and not other-error (but failed fseek call according to return value). So we need 1 success and 2 error return value states, one error return with `AnyError` and one with `NoError` (strange but happens according to the shown output). Probably there is relation between the previous state and the produced result of `fseek` but I do not want to figure out, it may be different in other systems and the documentations say nothing.

This comes from this program:
```
#include <stdio.h>

void print_result(FILE *F, int rc, const char *errtxt) {
  printf("--------\n%s", errtxt);
  if (rc) {
    printf("failed...\n");
    if (feof(F)) {
      printf("FEOF\n");
    }
    if (ferror(F)) {
      printf("FERROR\n");
      perror("error");
    }
  } else {
    printf("success\n");
  }
}

int main() {
  FILE *F = fopen("fseek.c", "r");

  while (EOF != fgetc(F)) {}
  print_result(F, 1, "read done\n");

  // Return value is non-zero on failure.
  int rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");

  rc = fseek(F, 2, SEEK_END);
  print_result(F, rc, "seek valid\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");
  
  fputs("str", F);
  print_result(F, 1, "failed operation\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");

  rc = fseek(F, -1, SEEK_END);
  print_result(F, rc, "seek valid\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");
}
```
And the result is:
```
--------
read done
failed...
FEOF
--------
seek invalid
failed...
FEOF
--------
seek valid
success
--------
seek invalid
failed...
--------
failed operation
failed...
FERROR
error: Bad file descriptor
--------
seek invalid
failed...
FERROR
error: Invalid argument
--------
seek valid
success
--------
seek invalid
failed...
FERROR
error: Invalid argument
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356