[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

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

balazske marked 22 inline comments as done and an inline comment as not done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:90-106
+  FnCheck identifyCall(const CallEvent &Call, CheckerContext &C,
+                       const CallExpr *CE) const;
+
+  void evalFopen(CheckerContext &C, const CallExpr *CE) const;
+  void evalTmpfile(CheckerContext &C, const CallExpr *CE) const;
+  void evalFclose(CheckerContext &C, const CallExpr *CE) const;
+  void evalFread(CheckerContext &C, const CallExpr *CE) const;
----------------
NoQ wrote:
> balazske wrote:
> > NoQ wrote:
> > > For most purposes it's more convenient to pass `CallEvent` around. The only moment when you actually need the `CallExpr` is when you're doing the binding for the return value, which happens once, so it's fine to call `.getOriginExpr()` directly at this point.
> > The CallExpr is used at many places to get arguments and other data. There is a `CallEvent::getArgSVal` that can be used instead but at some places CallExpr is used in other ways. I do not see much benefit of change the passed `CallExpr` to `CallEvent`.
> > at some places CallExpr is used in other ways
> 
> Can we add more convenient getters to `CallEvent` to help with those? 'Cause `CallEvent` has strictly more information than `CallExpr` (combining syntactic information with path-sensitive information), it's all about convenience.
> 
> I also see that `CallExpr` is stored in `StreamState` (which is something you can't do with a `CallEvent` because it's short-lived) but i suspect that it's actually never read from there and i doubt that it was the right solution anyway. I.e., no other checker needs that and i don't have a reason to believe that `StreamChecker` is special.
In the new code `CallEvent` is passed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:61
+  using FnCheck = bool (StreamChecker::*)(const CallEvent &,
+                                          CheckerContext &) const;
+
----------------
Szelethus wrote:
> Charusso wrote:
> > I prefer `std::function`, because it is modern.
> > ```
> >   using StreamCheck = 
> >       std::function<void(const StreamChecker *, const CallEvent &,
> >                          CheckerContext &)>;
> > ```
> > I think it is fine with pointers, but at some point we need to modernize this.
> But its also a lot more expensive. https://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/
> 
> `std::function` is able to wrap lambdas with different captures and all sorts of things like that, which comes at a cost.
Now `std::function` and `std::bind` is used. Probably more expensive but it is called once in a `evalCall`, that should be no problem?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:99
+  Optional<ProgramStateRef> CheckNullStream(SVal SV, CheckerContext &C) const;
+  Optional<ProgramStateRef> CheckDoubleClose(const CallEvent &Call,
+                                             CheckerContext &C) const;
----------------
Charusso wrote:
> This `Optional` is not necessary. When the state is changed, you can rely on `CheckerContext::isDifferent()` to see whether the modeling was succeeded. Therefore you could revert the `bool` return values as well.
`Optional` is not used now. The functions only return if the passed state was modified (if applicable). To detect if error was generated the `isDifferent` is used.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:137
+  return *Callback;
 }
 
----------------
Charusso wrote:
> I would move this tiny `identifyCall()` into `evalCall()` as the purpose of `evalCall()` is the validation of the `Callback` in this case.
`identifyCall` is removed now.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:196
 
   if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) {
     if (!BT_illegalwhence)
----------------
NoQ wrote:
> This is getting pretty messy, given that you've already made a transition in this function. If you do `addTransition` and then `generateNonFatalErrorNode`, then you'll get two parallel successor nodes, but you only need one. You should either delay the transition that happens immediately after `CheckNullStream`, or chain the two transitions together sequentially (as opposed to in parallel).
`evalFseek` has now a transition to new (non-null stream) state or transition to (non-fatal) error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67706