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

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

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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:333-335
+  // Ignore the call if the stream is is not tracked.
+  if (!State->get<StreamMap>(StreamSym))
+    return;
----------------
Szelethus wrote:
> If we check in `preCall` whether the stream is opened why don't we conservatively assume it to be open?
If we do that we get a resource leak error for example in the test function `pr8081` (there is only a call to `fseek`). We can assume that if the function gets the file pointer as argument it does not "own" it so the close is not needed. Otherwise the false positive resource leaks come always in functions that take a file argument and use file operations. Or this case (the file pointer is passed as input argument, the file is not opened by the function that is analyzed) can be handled specially, we can assume that there is no error initially and closing the file is not needed.


================
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:
> 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`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:438-439
   const StreamState *SS = State->get<StreamMap>(Sym);
   if (!SS)
     return State;
 
----------------
Szelethus wrote:
> Right here, should we just assume a stream to be opened when we can't prove otherwise? `ensureStreamOpened` is only called when we are about to evaluate a function that assumes the stream to be opened anyways, so I don't expect many false positive lack of `fclose` reports.
The warning is created only if we know that the stream is not opened. This function makes no difference if the stream is "tracked" (found in StreamMap) or not. In the not-tracked case it is the same as if it were opened. Probably the function can be changed to take a `StreamState` instead of `StreamVal` and the not-tracked case can be handled separately. Or this function can add the new `StreamState` in (opened state).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356