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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
