[PATCH] D26588: Add LocationContext to members of check::RegionChanges

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+                      const CallEvent *Call,
+                      const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
----------------
k-wisniewski wrote:
> NoQ wrote:
> > zaks.anna wrote:
> > > LocationContext can be obtained by calling CallEvent::getLocationContext(). I do not think that adding another parameter here buys us much. Am I missing something?
> > CallEvent* is optional here - it's there only for invalidations through calls.
> How about the situation when this callback is triggered by something other than a call, like variable assignment? I've tried using that location context and had lots of segfaults as in such cases it appeared to be null. Do you have some info that it should be non-null in such a case?
Ok, makes sense. Have you looked into providing a checker context there? How much more difficult would that be? If that is too difficult, adding LocationContext is good as well.

If Call is optional and LocationContext is not, should the Call parameter be last.


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+                                       ProgramStateRef state,
+                                       const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
----------------
k-wisniewski wrote:
> NoQ wrote:
> > zaks.anna wrote:
> > > What is the scenario in which you need the context here? The idea is that the checker would be interested in region changes if it is already tracking information in the state. For that check, the information in the state is sufficient. What is your scenario?
> > > 
> > > Also, For any checker API change, we need to update the CheckerDocumentation.cpp.
> > Environment, which is a significant portion of the state, is essentially unaccessible without a location context.
> > 
> > Call stack is also an important bit of information to any checker that does something special to support/exploit the inlining IPA - there are many checkers that scan the call stack, but so far none of them needed to subscribe to checkRegionChanges; their possible use case may look like "we want an update only if a certain condition was met within the current stack frame".
> > 
> > For the recursion checker, i think, the point is "if current frame is spoiled, we no longer want updates" (which should probably be fixed in the checker patch: even though the callback is broken, it'd take one less action to fix it if we decide to).
> Well I've added it to be consistent with checkRegionChanges, but AFAIK this callback is no longer necessary and there was a discussion about getting rid of it altogether. Maybe it's a good moment ot purge it?
I don't recall where that discussion has ended. If the callback is not used, I'd be fine with removing it. (That would need to be a separate patch.)


https://reviews.llvm.org/D26588