[cfe-commits] r59232 - /cfe/trunk/lib/Analysis/GRExprEngine.cpp

[Zhongxing Xu]

On Sat, Nov 15, 2008 at 1:49 AM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Nov 14, 2008, at 3:46 AM, Zhongxing Xu wrote:
>
>  Hi Ted,
>>
>> I rewrite the code with an AnonTypedRegion created to attach type
>> information to. Please review it.
>>
>>
> This looks pretty good so far!  First I'll mentioned a couple thoughts,
> then a few specific comments on the code.
>
> Thoughts:
>
> (1) We might want GRExprEngine::VisitCast to do the delegation of pointer
> -> pointer casts to StoreManager.  I'm not certain if this is the right
> approach, but it would give more flexibility to the particular
> implementation of StoreManager of how to handle such things.  Basically:
>
> if (isa<loc::MemRegionVal>(V)) {
>  assert (Loc::isLocType(CastE->getType());
>  assert (Loc::isLocType(Ex->getType());
>  // Delegate to store manager.
>  SVal VNew =
> StateManager.getStoreManager().CastRegion(CastE->getType()->getCanonicalType(),
> ..., V);
>  // update the state, etc.
> }
>
> Here "CastRegion" would do the actual region conversion, creating
> AnonTypedRegionm etc.  The advantage of this is that it allows different
> StoreManagers to handle arbitrary region to region casts differently and
> removes these details from GRExprEngine.
>
> (2) The conversion from AllocaRegion to an AnonTypedRegion probably applies
> to all regions that are not typed, not just AllocaRegion.
>
> (3) The interesting issues really come into play in how we query things in
> the store.  For example, how do we handle games like:
>
>  void* p = alloca(...);
>  char* q = (char*) p;
>  p = (void*) q;
>
> On the last line we should probably get back the original region returned
> from calling "alloca".  This is important if you consider a slightly
> different example:
>
>  void* p = alloca(...);
>  char *q = (char*) p;
>  void* r = (void*) q;
>  assert (r == p && "r and q should be the same!");
>
> Basically, my original concept of AnonTypedRegion was to layer some context
> on to an existing region, and then strip it away when it no longer applies.
>  This is really the design point we need to iterte on.  Is this the right
> approach?  etc.
>
> Also consider:
>
>  void* p = alloca(...);
>  ...
>  char *q = (char*) p;
>  *q = 'c';
>  ...
>  double *d = (double*) p;
>  *d = 1.0;
>  ...
>  char ch = *q;  // we should be able to flag this as an error, since that
> chunk of memory now binds to 'd'
>
> Another horrible case:
>
>  void* p = alloca(...);
>    ...
>  char *q = (char*) p;
>  *q = 'c';
>  ...
>  double *d = ((double*) p) + 1;
>  *d = 1.0;
>  ...
>  char ch = *q;  // whether or not this is an error is subjective.  It also
> gets down the point of what is the "extent" of AnonTypedRegion.
>
> I don't think we need to solve these problems all at once, but I think it's
> important to keep these design points in mind.  I'd like to converge to a
> clean and flexible design that allows us to model such things if we should
> so choose (as opposed to not allowing us to model such things at all).
>

These quite complex cases are introduced by the C language property that
allows us to have different views on the same chunk of raw memory. We intend
to tackle these cases by adding another layer on top of a raw memory region,
i.e. the AnonTypedRegion. But it is not enough to know what the view of a
chunk of raw memory is in the current state. To flag the above error, we
also need to know besides the current view of the memory, what other views
of the memory exist? But from an AnonTypedRegion we can only get its super
region, but no other AnonTypedRegion derived from that super region.

To get this knowledge, I propose to add a RegionView and a RegionViewMap
information. A RegionView is currently designed to be a ImmutableList of
MemRegions, which collects the current view on a memory region. The
RegionViewMap maps a region to its view. In code:

typedef llvm::ImmutableList<MemRegion*> RegionViewTy;
typedef llvm::ImmutableMap<MemRegion*, RegionViewTy> RegionViewMapTy;

Later if necessary we can extend the RegionView into a real class to hold
other information. With RegionViewMap, we can get all of the views of a
memory region.

One side problem is that I cannot make the MemRegion* in the ImmutableList
const. If I did that, I would get compile error, related to FoldingSet
Profile() overloading.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081115/e20b7547/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: regionview.patch
Type: application/octet-stream
Size: 10762 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081115/e20b7547/attachment.obj>