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

[Jordy Rose]

My revised version (from earlier today, never mailed) used a method called
Fill(), and actually /was/ a fill -- as in, it wiped out any subregion
bindings and could be used to model memset(). It's heavier, but could be
used in more situations -- the problem I see with that is that it wouldn't
do /enough/ (wouldn't handle memset_pattern*(), for example, at least not
easily in RegionStore).

In terms of performance, bindDefault() is certainly lighter, and if the
contract is that it's only used for "default" filler values for regions,
then it's not exposing an internal RegionStore detail, just re-using the
name. (That is, FlatStore should also support such "default" filler
values.) It would then be incorrect behavior to call bindDefault() if there
are any direct bindings. (Properly, it would probably be incorrect if there
are any bindings to the region or any subregions, but that's a relatively
expensive check just for an assertion.)

So I could see bindDefault() being okay, as long as it's a semantic
"default value" and not a RegionStore term. Alternate terms: fill(er),
init(ial), defaultContents.


On Mon, 31 May 2010 21:05:26 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> 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();
>> >   }
>>