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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yes, indeed, `evalCall` can only be performed by one checker. But if any, it is this checker that's fully responsible for stream functions. So i recommend doing evalCall in this particular checker and falling back to `PreCall`/`PostCall` in other checkers that model other aspects of streams.

Yes, indeed, `evalCall` prevents inlining from happening. This is fine, however, as long as you manually model all the effects of the call on the Environment (namely, set the return value) and on the Store (invalidate regions that may be touched by the function - in most cases it's none).

Generally, inlining shouldn't be thought of as an ideal solution because it has a downside of being extremely expensive. Instead, it should be thought of as a poor man's solution when it comes to any sort of API functions that have well-documented behavior. In particular, `evalCall` is more precise when it comes to state splits (which is why `StdCLibraryFunctionsChecker` uses `evalCall`, see D20811 <https://reviews.llvm.org/D20811> for details) (also "inlined defensive checks").



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:161
           .castAs<DefinedSVal>();
   state = state->BindExpr(CE, C.getLocationContext(), RetVal);
 
----------------
balazske wrote:
> NoQ wrote:
> > You're not allowed to do this in `checkPostCall` because other post-call checkers may have already read the return value.
> Is it possible to do in `check::PreCall`? The value of the call expression is not used, only a state split is done.
Nope, because `ExprEngine` will overwrite the value when modeling the function call, regardless of whether it will be inlined or not. The only valid reason to use `BindExpr` in a checker is to bind the return value in `evalCall`.

Generally, values in the Environment are not meant to be overwritten. Any active expression can only have one value.

The only way to obtain an environment with a different value for the same expression is to wait until the expression dies and reach the same expression later in the same analysis. In this case the value is first removed from the Environment during dead symbols collection, and then added back later.

For quite some time i wanted to prevent these mistakes by adding an assertion "Environment values are never mutated". Unfortunately, there are multiple existing violations of this rule, which are bugs in `ExprEngine`, and i didn't have time to fix them all. I highly recommend this little project as an exercise :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69662