[PATCH] D69662: [Checkers] Avoid using evalCall in StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.
Herald added a subscriber: martong.

In D69662#1744479 <https://reviews.llvm.org/D69662#1744479>, @NoQ wrote:

> In D69662#1736601 <https://reviews.llvm.org/D69662#1736601>, @balazske wrote:
>
> > Anyway the checks that do not use BindExpr (all except the open functions) could be moved into a PreCall or PostCall callback?
>
>
> Moving from evalCall to PreCall/PostCall has the additional effect of not giving you control over invalidation of the heap (unless you do evalCall in a checker, it ends up being the normal behavior of `conservativeEvalCall()` most of the time). For that reason ideally every library function should be evalCall'ed by a checker.
>
> Also if you're making updates to the program state that other checkers should see immediately (say, writing out-parameter values into the Store or updating a state trait that other checkers will read in the same callback), you should either use evalCall for that, or make sure your dependencies are set up correctly (@Szelethus, our callback invocation order is now affected by checker dependencies, right?).


Sorry for the slack :)

One should never count on the invocation order of callback funcions in between checkers. In fact, I'm not too sure that my patches affect this, but I suspect that it does, as the container of choice for checker objects is `std::vector`.

> `checkArgNullStream()` should definitely be at PreCall.
> 
> `evalFseek()` doesn't have a `BindExpr` but it //should// have it; looks like a bug. If you're evalCall-ing a non-void function you //must// bind a return value (we should add an assertion for this; there's never a reason to bind an `UnknownVal` in evalCall because there generally never is a good reason to bind `UnknownVal` to anything because it shouldn't have been present in our `SVal` hierarchy in the first place because conjuring a value is always strictly better).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69662