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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to