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

[Ted Kremenek]

Hi Jordy,

I feel that from an API perspective, "Fill" seems like a more natural abstraction for clients.  It has the clear semantics of doing a batch binding of values to a chunk of memory and wiping out what was previously there.  Given such clear semantics, it is up to the StoreManager to efficiently implement it.

In the case of RegionStore, RegionStoreManager can implement "Fill" by setting a default binding and removing existing direct bindings.  This keeps the implementation lightweight, but also makes the semantics correct.  For example, if the the memory region previously had some bindings, some clients will want to know when those bindings go away (e.g., the Objective-C memory leak checker).

BasicStore could also implement "Fill" very naturally for the bindings it supports without adding the notion of a "default" binding (which it doesn't have).

'bindDefault' might be okay if all we want to use it for is setting initial values for a memory region, but that's it.  I can see that API being misused, however, if clients trying using it outside of just the initialization of a chunk of memory.  In this case, I would want to add an assertion checking that that there wasn't an existing default/direct binding when 'bindDefault' is called.

'Fill' also seems amendable to modeling memset and friends.

On May 31, 2010, at 9:16 PM, Jordy Rose wrote:

> 
> 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();
>>>>  }
>>>