<br><br><div class="gmail_quote">On Fri, Nov 7, 2008 at 10:40 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c"><br>
On Nov 5, 2008, at 10:00 PM, Zhongxing Xu wrote:<br>
<br>
</div></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="Wj3C7c">
Second try.<br>
- All region extents are managed by MemRegionManager.<br>
- Region extents are set explicitly by  GRExprEngine calling StoreManager::setExtent() after BindDecl(). (better approach?)<br></div></div>
<extent.patch><br>
</blockquote>
<br>
Hi Zhongxing,<br>
<br>
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.<br>

<br>
Consider an alternate, much simpler, representation of extents:<br>
<br>
class RegionExtent {<br>
  const NonLoc  SizeBits;<br>
  const llvm::APSInt* ElementSizeBits;<br>
public:<br>
  RegionExtent(NonLoc sizebits)<br>
   : SizeBits(sizebits), ElementSizeBits(0) {}<br>
<br>
  RegionExtent(NonLoc sizebits, const llvm::APSInt* esizebits)<br>
    : SizeBits(sizebits), ElementSizeBits(esizebits) {}<br>
<br>
  NonLoc getSizeBits() const {<br>
    return SizeBits;<br>
  }<br>
<br>
  NonLoc getSizeBytes() const;<br>
  NonLoc getSizeElements();<br>
};<br>
<br>
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.<br>

<br>
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.</blockquote><div><br>
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?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
Alternatively, having a specific variant for extents that is uniqued through a folding set also has its benefits.</blockquote><div><br>I cannot see obvious benefits for the time being.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
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.<br>

<br>
Thoughts?  I definitely think we're going in the right direction.<br>
</blockquote></div><br>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.<br>
<br>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.<br>