zaks.anna added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+ CheckerContext &C) const {
----------------
gamesh411 wrote:
> NoQ wrote:
> > One of the practical differences between `checkPostCall` and `evalCall` is
> > that in the latter you have full control over the function execution,
> > including invalidations that the call causes. Your code not only sets the
> > return value, but also removes invalidations that normally happen. Like,
> > normally when an unknown function is called, it is either inlined and
> > therefore modeled directly, or destroys all information about any global
> > variables or heap memory that it might have touched. By implementing
> > `evalCall`, you are saying that the only effect of calling `operator<<()`
> > on a `basic_ostream` is returning the first argument lvalue, and nothing
> > else happens; inlining is suppressed, invalidation is suppressed.
> >
> > I'm not sure if it's a good thing. On one hand, this is not entirely true,
> > because the operator changes the internal state of the stream and maybe of
> > some global stuff inside the standard library. On the other hand, it is
> > unlikely to matter in practice, as far as i can see.
> >
> > Would it undermine any of your efforts here if you add a manual
> > invalidation of the stream object and of the `GlobalSystemSpaceRegion`
> > memory space (you could construct a temporary `CallEvent` and call one of
> > its methods if it turns out to be easier)?
> >
> > I'm not exactly in favor of turning this into `checkPostCall()` because
> > binding expressions in that callback may cause hard-to-notice conflicts
> > across multiple checkers. Some checkers may even take the old value before
> > it's replaced. For `evalCall()` we at least have an assert.
> >
> > If you intend to keep the return value as the only effect, there's option
> > of making a synthesized body in our body farm, which is even better at
> > avoiding inter-checker conflicts. Body farms were created for that specific
> > purpose, even though they also have their drawbacks (sometimes `evalCall`
> > is more flexible than anything we could synthesize, eg. D20811).
> >
> > If you have considered all the alternative options mentioned above and
> > rejected them, could you add a comment explaining why? :)
> I am not familiar with the BodyFarm approach, however I tried something
> along the lines of:
> auto CEvt =
> ResultEqualsFirstParam->getStateManager().getCallEventManager().getSimpleCall(CE,
> S, C.getLocationContext());
> ProgramStateRef StreamStateInvalidated =
> CEvt->invalidateRegions(C.blockCount());
>
> It however broke test2 (where the state is set to hex formatting, then, back
> to dec). Any tips why resetting regions could cause problems?
>
I agree that we should not use evalCall(), especially, in an opt-in checker, as
it can introduce subtle/hard to debug issues. As was mentioned, if a checker
implements evalCall(), the usual evaluation by the analyzer core does not
occur, which could lead to various unpredictable results.
https://reviews.llvm.org/D27918
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
