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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88
 struct FnDescription;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
----------------
balazske wrote:
> NoQ wrote:
> > `llvm::function_ref`?
> `function_ref`'s documentation says:
> > This class does not own the callable, so it is not in general safe to store a function_ref.
> The `FnDescription` stores these functions so it is not safe to use `llvm::function_ref`?
> 
> 
> 
I think you're using it only for global function pointers, no?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:366
+
+  State = State->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(State);
----------------
balazske wrote:
> NoQ wrote:
> > So what exactly happens when you're calling `clearerr` on a closed stream? Does it actually become opened? Also what happens when you're calling it on an open-failed stream?
> The stream gets always in opened and error-free state. The `preDefault` checks that the stream is opened (not closed and not open-failed) and creates error node if not. If this create fails that is a problem case, the stream will be opened after `clearerr`. Should check in the eval functions for closed stream even if the pre-functions (in `preCall` callback) have normally checked this?
Aha, ok, got it.
Let's assert it then?


================
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}}
----------------
balazske wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > 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 :)
> > Yeah, that's gonna be fun.
> If there is a `fputc` call (for example) that is recognized (has recognized stream and no functions "fail") there is already an error and non-error branch created for it. On the error branch the return value of `fputc` would be **EOF** and the stream state is set to error. (The `fputc` should be later modeled by this checker, not done yet.)
> 
> If the `ch == EOF` is found then to figure out if this `ch` comes from an `fputc` that is called on a stream that is not recognized (it may be function parameter) and then make a "tracked" stream with error state for it, this is another thing.
Aha, fair enough, that's probably a well-justified state split.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682