[cfe-commits] [Patch]LocationContext Support for CheckedReturn

[Ted Kremenek]

Hi Lei,

I think there are multiple possible approaches here.

First, I think it's reasonable for us to add a "Generic Data Map" mechanism, similar to what we have in GRState, for keeping checker-specific data in LocationContexts.  It needs to be a general mechanism, however, so that the LocationContext API doesn't have anything relating to a specific checker; just a generic way to hold data.  That data also needs to be deallocated once the LocationContext object is freed.

That said, a simpler mechanism may just be to keep a DenseMap in the statistical checker from LocationContexts -> statistics, and then notify a checker when the LocationContext object gets destroyed.  Actually, we can possibly implement the Generic Data Map mechanism in this way.

Concerning "inlining", what you call "textual" analysis versus WPA mode reduces down to us having a "policy" object that dictates when we inline and when we don't.  With a policy object, we could change when and how IPA is done, and reuse the same analysis engine.  Different checkers could then articulate that they require a specific IPA policy in place (in the case of this checker, no IPA).  This idea is only half-formed, but I think there are multiple places in the analyzer engine where various "policies" could be used to help guide how analysis is done (e.g., how are loops handled, etc.).

If your checker truly won't work in IPA, then it is probably okay to just use a DenseMap in the Checker itself, and just invalidate that DenseMap when we start analyzing a function.  That seems like the least complicated solution, although it prohibits a single instance of the Checker from being used simultaneously by multiple ExprEngines.  After thinking about this some, I think this is a reasonable approach, just as long as you understand the restrictions here and that the statistics really will only work when analyzing a single function at a time.

On Apr 11, 2011, at 11:10 PM, 章磊 wrote:

> Hi Ted, 
>  
> I think statistical checker is some different from the checkers we have. 
> 
> IMO, when doing statistical check, the static analyzer should not work in the "whole-program-analysis" mode, but in the "textual" statistic mode. 
> 
> In this mode, we treat every top level functiondecl (also the inline function) has a body as a cell. Then we do symbolic execution in every cell to check CheckedReturn state for calls (i.e, a call to foo()), and textually count a function decl's (i.e, foo()) CheckedReturn state from all the occurrences (calls). 
> 
> So we need a new action to activate this "textual" mode, and in this mode the inline IPA should be deactivated (i think the function summary IPA also works). Then every cell will have only one LocationContext, right? 
>  
> Here is a example to make it clearer. 
>  
> int f1(); 
>  
> int main() { 
>   f2(); 
>   f3(); 
> } 
>  
> void f2() { 
>   f1(); 
> } 
>  
> void f3() { 
>   f2(); 
>   if (f1()) 
>     return; 
> } 
>  
> There are two "textual" calls to f1(), the one in f3() is checked and the one in f2() is unchecked. But if we do the check in WPA Mode(IMO, as viewed from "execute"), there are 3 calls to f1(), one of them is checked and the other two are not. 
>  
> We should do it in the "textual" way, right?
> 
> What you say about this?
> 
> 2011/4/12 Ted Kremenek <kremenek at apple.com>
> Lei,
> 
> Just to make this a little clearer, suppose we had:
> 
> unsigned factorial(unsigned n) {
>  if (n == 0)
>   return 1;
>  return n * factorial(n - 1);
> }
> 
> Currently when we analyze this function, we create only one LocationContext.  However, one way to do IPA is to due inlining of function calls.  In this case, we might create a second LocationContext during the first recursive call to factorial.  If we wanted to further inline, we might create a second LocationContext, etc., each one capturing the "context" of the recursive call and simulating an abstract stack frame (obviously bounding the amount of LocationContexts we create).
> 
> On Apr 11, 2011, at 10:53 AM, Ted Kremenek wrote:
> 
> > I thought about this some more, and using the LocationContext might be a reasonable place to put checker-specific data that represents "global" information (i.e not specific to a GRState) but also limited in scope (i.e. the data is limited to the scope of analyzing a given *call* to a function).  Once we support analysis inlining, it will be possible for multiple LocationContext objects to be around at the once for the same function.  Are the statistics you are interested in specific to a given LocationContext?
> >
> > On Apr 11, 2011, at 10:19 AM, Ted Kremenek <kremenek at apple.com> wrote:
> >
> >> Hi Lei,
> >>
> >> LocationContext should not contain any checker-specific data.  It is only intended to model context-sensitivity (i.e., it simulates an abstract stack frame, or "location" in an abstract call chain).
> >>
> >> What are you trying to do?
> >>
> >> Ted
> >>
> >> On Apr 10, 2011, at 8:26 PM, 章磊 wrote:
> >>
> >>> Hi Clang,
> >>>
> >>> This patch add a DenseMap to record the CheckedReturn count for a functiondecl(fielddecl, vardecl as function pointer) in LocationContext.
> >>>
> >>> ImmutableMap seems not alright here, is DenseMap ok?
> >>>
> >>> And how to make it checker-specific?
> >>>
> >>> This patch is preparation for statistical UncheckedRenturn checker.
> >>> I'll appreciate it if there are any advice about this patch.
> >>>
> >>> --
> >>> Best regards!
> >>>
> >>> Lei Zhang
> >>> <CRResultMap.patch>_______________________________________________
> >>> cfe-commits mailing list
> >>> cfe-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> 
> 
> 
> -- 
> Best regards!
> 
> Lei Zhang

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110413/5ff410eb/attachment.html>