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