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

[Krzysztof Wiśniewski via cfe-commits]

k-wisniewski added a comment.

Hi again!

Thanks for review! I'll upload updated version of this patch today in the evening/tomorrow. As Anna suggested I'll split it into two parts - one regarding extraction of argument SVals for StackFrameCtx and another one adding LocationContext to check::RegionChanges interface. I have commented inline  on why I think this change is needed.

Cheers!



================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+                      const CallEvent *Call,
+                      const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
----------------
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?
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?


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+                                       ProgramStateRef state,
+                                       const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
----------------
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.
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?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:231
 
   ProgramStateRef bindLoc(Loc location,
                           SVal V,
----------------
NoQ wrote:
> Because this API becomes more complicated, i think we should add some docstrings here, explaining the meaning of the location context parameter, in particular.
Will do!


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:741
+    // because the call wasn't modeled in the first place.
+    const VarDecl *ArgDecl = FunctionDecl->parameters()[ArgIdx];
+    const Loc ArgLoc = getLValue(ArgDecl, SFC);
----------------
NoQ wrote:
> a.sidorin wrote:
> > Unfortunately, this code does not consider the fact that argument values may be overwritten. If we want to get initial values, we should find another way.
> Uhm, yeah, this is in fact a problem!
> 
> The purpose of this code is, in fact, to construct `SymbolRegionValue` for the parameter, so it is equivalent to calling `SValBuilder::getRegionValueSymbolVal()` over the parameter region, as long as the current `StoreManager` implementation remains unquestioned.
> 
> We could also ask `StoreManager` to provide a binding for this region from an empty store, maybe extend its API to allow such queries, this would remove the layering violation.
> 
> Because this topic getting is rather complicated, it might have been better to make a separate review for this change, originally (no problem now that most of the interested people have already had a look).
Yeah, as Anna suggested I will split this change and the addition of LocationContext in check::RegionChanges and we should probably continue this discussion there


https://reviews.llvm.org/D26588