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

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

balazske marked 6 inline comments as done.
balazske 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 &)>;
----------------
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`?





================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:366
+
+  State = State->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(State);
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:379-381
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return;
----------------
NoQ wrote:
> Maybe we should add a helper function for this, `CallEvent::getOriginCallExpr()`. That's an otherwise ugly function given that `CallEvent ` is a class hierarchy and the superclass doesn't need to know about subclasses, but most users do in fact only care about call-expressions and concise checker API is important. Same with `dyn_cast_or_null<FunctionDecl>(Call.getDecl())` that could be written as `Call.getFunctionDecl()`.
Can be done in another revision together with changing every use of `dyn_cast_or_null<CallExpr>(Call.getOriginExpr())` to the new function.


================
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}}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682