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

[Zhongxing Xu]

On Fri, Nov 7, 2008 at 10:40 AM, Ted Kremenek <kremenek at apple.com> wrote:

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


I like this approach. It is similar to my original thought: an Extent is
just a NonLoc. BTW, what is ElementSizeBits for? Does it describe the size
of element of array or the number of elements in an array?


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


I cannot see obvious benefits for the time being.


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

The extent for a variable is obvious while the extent for a malloced region
is not. We should make it clear whether we want all the extent data be
maintained by the GDM. If so, whether we do the lazy mapping or eager
mapping. Calling setExtent() after we create a new region everytime is the
eager approach. Setting the extent after the first time we query for it is
the lazy approach.

But there is a problem with the lazy approach. Note that for a getExtent()
we not only return the extent but also need to return a new GRState, because
we might add a new GDM mapping for the extent. How can we do this? With a
reference parameter? This is the reason that I use the eager approach.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081107/fb32cb62/attachment.html>