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

Zhongxing Xu xuzhongxing at gmail.com
Mon May 31 21:53:55 PDT 2010 

On Tue, Jun 1, 2010 at 12:43 PM, Zhongxing Xu <xuzhongxing at gmail.com> wrote:

>
>
> On Tue, Jun 1, 2010 at 12:30 PM, Ted Kremenek <kremenek at apple.com> wrote:
>
>> 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.
>>
>
> This makes sense.
>
> Another approach is to reuse InvalidateRegions(), and change it to
> InvalidateRegionsWithVal(). By default it can set default binding to
> symbolic. But we can also pass other values to it. That way we can remove
> bindDefault().
>

But this could be too heavy. What the client need is a way to initialize a
region with a default value.


>
>
>>
>> '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();
>> >>>>  }
>> >>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100601/9c039b95/attachment.html>

More information about the cfe-commits mailing list