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

[Ted Kremenek]

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).

That said, I only have a couple additional comments on your patch:

+    if (isa<loc::MemRegionVal>(V)) {
+      if (const AllocaRegion* AR =
+           
dyn_cast<AllocaRegion>(cast<loc::MemRegionVal>(V).getRegion())) {
+
+        MemRegionManager& RM = getStateManager().getRegionManager();
+
+        // Create a new region to attach type information to it.
+        const AnonTypedRegion* TR = RM.getAnonTypedRegion(T, AR);
+
+        // Get the pointer to the first element.
+        // FIXME: get the pointer width from the target information.
+        llvm::APSInt Zero(llvm::APInt::getNullValue(32), false);
+        SVal  
ZeroIdx(nonloc::ConcreteInt(getBasicVals().getValue(Zero)));

Just use BasicValueFactory::getZeroWithPtrWidth().  i.e.:

   SVal ZeroIdx(getBasicVals().getZeroWithPtrWidth());

+        const ElementRegion* ER = RM.getElementRegion(ZeroIdx, TR);

Move this (and probably the majority of this chunk of code) into the  
StoreManager classes.  This use of ElementRegion seems pretty tied to  
how RegionStore reasons about things.  BasicStore can probably just  
return the original region.

+
+        MakeNode(Dst, CastE, N, BindExpr(St, CastE,  
loc::MemRegionVal(ER)));
+        continue;
+      }
+    }
+