[PATCH] D80015: [Analyzer][StreamChecker] Added support for 'fread' and 'fwrite'.

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

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

The difference of fread and fwrite comes from the fact that `fwrite` can always succeed, `fread` does not succeed in EOF state.
The plan is to add the file functions sequentially. From the currently implemented functions only `fread` needs the warning in EOF state. When any later function is added that fails in EOF state, the warning should be added for that function at that time.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:56
+  bool operator!=(const StreamErrorState &ES) const {
+    return NoError != ES.NoError || FEof != ES.FEof || FError != ES.FError;
+  }
----------------
Szelethus wrote:
> How about `return !(*this == ES)`?
Probably better (does not look better but is more bug-safe).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:456-463
+  SymbolRef Sym = StreamVal.getAsSymbol();
+  if (Sym && State->get<StreamMap>(Sym)) {
+    const StreamState *SS = State->get<StreamMap>(Sym);
+    if (SS->ErrorState & ErrorFEof)
+      reportFEofWarning(C, State);
+  } else {
+    C.addTransition(State);
----------------
Szelethus wrote:
> Do we not want this for `fwrite` as well?
It should be no problem to write into a file at the end of the file, the file is extended.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:495
+    return;
+  Optional<NonLoc> CountVal = Call.getArgSVal(2).getAs<NonLoc>();
+  if (!CountVal)
----------------
Szelethus wrote:
> `// The standard refers to this parameter as "nmemb", but almost everywhere else its called "count".`
I looked up the functions in [[ https://en.cppreference.com/w/c/io/fread | cppreference.com ]], there it is called `count`. But probably it is better to use the standard name here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:523
+  // FEOF).
+  if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+    ProgramStateRef StateNotFailed =
----------------
Szelethus wrote:
> Aaaah okay it took me a while. I think `SS->ErrorState != ErrorFEof` isn't as talkative as `isConstrained.*`, could you add just a line like "if we **know** the state to be EOF..."?
> 
> On another note, why is fwrite so special in this context?
If it is an `fwrite`, the function call can always succeed (even after a previously EOF or error).
If it is an `fread`, it can succeed but not after an exactly-EOF state.

A success transition is not needed in an `fread` with exactly EOF condition. (If we have fread with non-exactly EOF, for example `ErrorFEof` and `ErrorFError`, the success is needed because the read in `ErrorFError` can succeed.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80015