[PATCH] D27918: [analyzer] OStreamChecker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 6 inline comments as done.
gamesh411 added a comment.

Update diff.



================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:263-282
+  mutable IdentifierInfo *II_BasicOstream, *II_Flags, *II_Setf, *II_Unsetf,
+      *II_Setiosflags, *II_Resetiosflags, *II_Precision, *II_SetPrecision,
+      *II_BaseField, *II_Hex, *II_Dec, *II_Oct, *II_AdjustField, *II_Left,
+      *II_Right, *II_Internal, *II_BoolAlpha, *II_NoBoolAlpha, *II_ShowPos,
+      *II_NoShowPos, *II_ShowBase, *II_NoShowBase, *II_UpperCase,
+      *II_NoUpperCase, *II_ShowPoint, *II_NoShowPoint, *II_FloatField,
+      *II_Fixed, *II_Scientific;
----------------
NoQ wrote:
> If you ever want to extend the `CallDescription` class to cover your use case, please let us know :)
> 
> While most of these aren't functions, the approach used in this class could be used here as well - making initialization code shorter.
I will look at the CallDescription class, and how the checker can benefit. Is it still a problem, that we can not initialize IdentifierInfos during checker construction? If so, how will would a CallDescription implementation solve that?


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:387
+
+static Optional<int> tryEvaluateAsInt(const Expr *E, ProgramStateRef S,
+                                      CheckerContext C) {
----------------
NoQ wrote:
> I think you should use `SimpleSValBuilder::getKnownValue()`. The `EvaluateAsInt` part doesn't add much because the analyzer's engine should already be more powerful than that. On the other hand, the `ConstraintManager::getSymVal()` thing, which is already done in `SimpleSValBuilder::getKnownValue()`, may be useful.
I have tried an implementation of getKnownValue(), and it more terse, and did not handle the cases it should have had to anyway.


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+                                    CheckerContext &C) const {
----------------
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?



https://reviews.llvm.org/D27918