[PATCH] D27918: [analyzer] OStreamChecker

[Anna Zaks via Phabricator via cfe-commits]

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