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

[Zhongxing Xu]

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/20100601/8d70a126/attachment.html>