[cfe-commits] [PATCH] A tentative implementation of RegionStoreManager

[Ted Kremenek]

On Oct 6, 2008, at 11:49 PM, Zhongxing Xu wrote:

> These are all good points to consider. My suggestion is that we add  
> complexity step by step. In this first basic region store model (see  
> the attached new patch), we assume simple cases: we assume no  
> aliasing for parameters, we do 1-limiting analysis and assume  
> nothing about the heap shape. So the only big thing compared to  
> BasicStore is the field sensitivity.

I completely agree.  What you propose is an excellent first step.

Incidentally, please include what you just said in the header for  
RegionStoreManager.[h,cpp].  It succinctly describes the scope of the  
store implementation.

> Then we can design more complex ones: heap store model, alias store  
> model, ...

Absolutely!

> Another cure that I'm looking forward to is the inter-procedural  
> analysis. Most complexities above are due to lack of environment  
> information. But when we do top-down inter-procedural analysis, we  
> will have (at least part of) function entree information. This can  
> relieve us from assuming the worst case.

That's very true.

>> It seems to me that RegionStoreManager doesn't need to use just one  
>> kind of region and one ImmutableMap.  For example, a map from  
>> VarDecl* -> VarRegion could be used for mappings from variables to  
>> regions, and then other maps could be used for other bindings.  For  
>> example, a (MemRegion*, FieldDecl*) -> FieldRegion could be used  
>> for field bindings.  A second set of mappings could then be used  
>> for region -> value bindings.  In your prototype implementation you  
>> have AnonTypedRegion* -> RVal, but one could also just have  
>> MemRegion* -> RVal.
>>
>> My thought is that we don't need any mappings from Decl* to  
>> MemRegion*. We only need one mapping from MemRegion* to its stored  
>> value RVal. Because once we have the mapping MemRegion* -> RVal, we  
>> can calculate the location (MemRegion*) of any name on the fly. So  
>> we don't need to store them.
>
> Unfortunately I don't believe that's true (which counteracts  
> something I said in an earlier private email).
>
> Consider:
>
> int *p = 0;
>
> for (int i = 0; i < 10; i++) {
>   if (p) *p++;
>   int j = i + 1;
>   p = &j;
> }
>
> On each iteration of the loop the VarDecl for 'j' will conceptually  
> bind to a different region (although by coincidence it may bind to  
> the same physical memory, logically it binds to a different  
> "object").  While memory bindings for globals and parameters stay  
> fixed during the execution of a function (with globals staying  
> always fixed), bindings for local variables do not.  If we want to  
> catch cases like the *p++ being a use of invalid memory, we actually  
> have to consider the case where the same VarDecl can bind to  
> different regions.  Note that we do not capture enough scope  
> information in the CFGs to do this analysis right now, but that is  
> something we probably we will add to the CFGs in the future  
> (especially if we wish to model implicit calls to destructors for C+ 
> + objects).
>
> Why in this example 'j' will bind to a different region on each  
> iteration? In actual (physical) execution of this program, 'j' just  
> holds the same stack memory. It's the same as the one declared  
> outside of the loop, except its scope is limited inside the loop.  
> Instead, if we bind it to different region on each iteration,  we  
> will get wrong results, because it's inconsistent with the semantics  
> of C.

After reading your comments, I realized this wasn't the example that I  
intended.  I'm going to pull this topic out into a separate thread.

> <regionstore.patch>

I have two more questions, but I think your patch looks good to apply  
right now (we can continue to iterate on details).

First, is AnonPointeeRegion used now?  Scanning the patch I see no  
uses of it.  Do you plan on adding this later?

Second, the "unknown" region appears to be the same for any place it  
will be used (simply based on how getUnknownRegion() is implemented).  
Is this intentional?