[cfe-commits] [PATCH] Fix for PR7218, and analyzer support for calloc()

[Ted Kremenek]

Hi Zhongxing, Jordy,

Do you think that 'bindDefault' is the right API, or at least the right name?  The notion of 'default' and 'direct' bindings is something internal to the implementation of RegionStore.  Is this something we want to expose as part of the Store API that must be serviced by all StoreManagers?  If so, what does a 'default' value mean to external clients?  What are the semantics if an external client calls 'bindDefault' after doing a direct binding?

Ted

On May 31, 2010, at 8:04 PM, Zhongxing Xu wrote:

> Hi Jordy,
> 
> I applied most of your patch, except that I adopted 'bindDefault' approach to set the default value, since it does not touch GRExprEngine. Thanks for working on this!
> 
> On Sun, May 30, 2010 at 3:46 PM, Jordy Rose <jediknil at belkadan.com> wrote:
> 
> Binding a symbolic region whose type is a reference shows up when the
> reference is an argument, like so:
> 
> char t3 (char& r) {
>  r = 'c';
>  if (r) return r;
>  return '0';
> }
> 
> The reason for the SymbolicRegion section in canHaveDirectBinding(),
> though, was originally more about having a way to set default values by
> taking advantages of a fact about SymbolicRegions (if you're accessing them
> directly, it's either *p or a reference, or an explicit call to Bind()),
> not enforcing a rule.
> 
> For looking up super regions' direct bindings, I tried commenting out that
> entire section, but it makes the case you mentioned fail
> (no-outofbounds.c). Looking at each of the inside IFs:
> 1. SVN blame says this fixed a crash in misc-ps-region-store-x86_64.m.
> 2. Same as your example, but with x not yet defined:
>  int x;
>  char *p = &x;
>  return p[0]; // expected-warning {{Undefined}}
> 3. LazyCompoundVal is LazyCompoundVal...i.e. I don't fully understand what
> circumstances they're used in. On the other hand, if I understand
> correctly, things with compound values should never have direct bindings
> /other/ than LazyCompoundVal.
> 
> It seems like if there's any chance of the feature being useful (even
> UnknownVal vs UndefinedVal) this section has to stay. Between
> no-outofbounds.c and the commit message about misc-ps-region-store-x86_64.m
> crashing; I didn't pry much deeper.
> 
> 
> As for the calloc() part of the patch, it depends on how default values
> should be set from outside RegionStore.cpp, which calloc() needs to be
> properly modeled. (And malloc(), for that matter.) I came up with three
> options:
> 1. Add a new BindDefault() method to Store, make it a no-op in BasicStore
> and implement it for real in RegionStore.
> 2. Add a new parameter to SetExtent(). After all, anything with an
> explicit extent doesn't get the usual BindArray initialization, and so it's
> going to /need/ some kind of initialization. Downside: the rest of the
> Extent code isn't related (though the only extents right now are malloc
> regions and alloca regions, which could both use this).
> 3. Define binding to a symbolic region as setting a default value. Because
> my fix for PR7218 already changed how lookups worked for arrays, this
> didn't seem too far afield, but that's not really a great reason. This
> involved the part about changing *p to use ElementRegions ahead of time, so
> that the only SymbolicRegions that would make it to Bind() would be the
> kind that could have default values (...and references).
> 
> I chose #3, but either of the other two would also work. #2 is actually my
> favorite, if we don't mind linking regions and default values conceptually.
> 
> Guess I should have submitted this in two pieces. It's because I was
> working on calloc() first that I didn't think of that ahead of time. Let's
> get PR7218 nailed down first, then decide how calloc() can set a default
> value from outside RegionStore.cpp.
> 
> Sorry...? And thanks for the review, and pointing me in the right
> direction.
> Jordy
> (IRC: jediknil)
> 
> 
> On Sat, 29 May 2010 14:55:08 +0800, Zhongxing Xu <xuzhongxing at gmail.com>
> wrote:
> > I'm thinking about the whole logic below. Does it make sense to try to
> get
> > the direct binding of the super region of an element region?
> >
> > I can only think of one case:
> >
> > int x = 1;
> > char *y = &x;
> > y[2];
> >
> > But this case only triggers 'return UnknownVal();' in the last. What
> cases
> > does the 3 'if' above deal with?
> >
> > RegionStore.cpp:1177
> >
> >   // Check if the immediate super region has a direct binding.
> >   if (const Optional<SVal> &V = getDirectBinding(B, superR)) {
> >     if (SymbolRef parentSym = V->getAsSymbol()) {
> >       return ValMgr.getDerivedRegionValueSymbolVal(parentSym, R);
> >     }
> >
> >     if (V->isUnknownOrUndef())
> >       return *V;
> >
> >     // Handle LazyCompoundVals for the immediate super region.  Other
> cases
> >     // are handled in 'RetrieveFieldOrElementCommon'.
> >     if (const nonloc::LazyCompoundVal *LCV =
> >         dyn_cast<nonloc::LazyCompoundVal>(V)) {
> >       R = MRMgr.getElementRegionWithSuper(R, LCV->getRegion());
> >       return RetrieveElement(LCV->getStore(), R);
> >     }
> >
> >     // Other cases: give up.
> >     return UnknownVal();
> >   }
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100531/81fffc95/attachment.html>