[cfe-commits] [PATCH] Set region size in GRRegionVals transfer function

[Ted Kremenek]

On Nov 5, 2008, at 10:00 PM, Zhongxing Xu wrote:

> Second try.
> - All region extents are managed by MemRegionManager.
> - Region extents are set explicitly by  GRExprEngine calling  
> StoreManager::setExtent() after BindDecl(). (better approach?)
> <extent.patch>

Hi Zhongxing,

Let's step back a second what we want out of RegionExtent, and whether  
or not we actually need to unique them using a folding set.  There is  
a cost to using a FoldingSet, including the allocations, the extra  
hash table, etc., so we should think about whether or not it is  
needed.  I know I implemented much of the original extent code (which  
was removed until we needed it), but I thought I'd toss out some  
points to consider.

Consider an alternate, much simpler, representation of extents:

class RegionExtent {
   const NonLoc  SizeBits;
   const llvm::APSInt* ElementSizeBits;
public:
   RegionExtent(NonLoc sizebits)
    : SizeBits(sizebits), ElementSizeBits(0) {}

   RegionExtent(NonLoc sizebits, const llvm::APSInt* esizebits)
     : SizeBits(sizebits), ElementSizeBits(esizebits) {}

   NonLoc getSizeBits() const {
     return SizeBits;
   }

   NonLoc getSizeBytes() const;
   NonLoc getSizeElements();
};

Here the APSInt would be uniqued using BasicValueManager, and the  
NonLoc object references data that is also uniqued.  The total size of  
an extent is two pointers and an unsigned integer (plus padding).   
This is fine for a temporary object that we just pass around.

The nice thing about this approach is that it piggybacks on NonLoc.   
This means it can represent extents whose size is an integer constant,  
a symbol, an expression (e.g., $1 + 2), unknown extents, etc.

Alternatively, having a specific variant for extents that is uniqued  
through a folding set also has its benefits.

Also, I'm not certain we need use setExtent() for regions associated  
with variables (unless they are arrays).  It seems like the extent for  
a variable is pretty obvious, and can just be returned (created?)  
lazily when we call StoreManager::getExtent().  setExtent() seems  
useful when processing regions created by malloc(), new, alloca(), or  
anything else where we cannot reconstruct the extent just from the  
information stored in the region itself.

Thoughts?  I definitely think we're going in the right direction.