[cfe-commits] r63347 - in /cfe/trunk: include/clang/Analysis/PathSensitive/BasicValueFactory.h include/clang/Analysis/PathSensitive/MemRegion.h include/clang/Analysis/PathSensitive/SVals.h lib/Analysis/MemRegion.cpp lib/Analysis/RegionStore.cpp l

[Zhongxing Xu]

On Tue, Feb 3, 2009 at 5:21 AM, Ted Kremenek <kremenek at apple.com> wrote:

> Zhongxing,
>
> I'm going to respond to your comments out-of-order.
>
> On Jan 30, 2009, at 1:36 AM, Zhongxing Xu wrote:
>
> <SNIP>
>
>  It saves much trouble later when p is used as array base.
>>
>> If we do not make that ElementRegion when doing cast, it is hard to
>> distinguish the case for code:
>>
>> struct s* p = (struct s*) alloc(sizeof struct s);
>> struct s v = p[1];
>>
>> For this case the region for p should not be used as the base region for
>> element region.
>>
>> My point is that we make an ElementRegion in the first case and an
>> AnonTypedRegion for the second case, and we can only do the right thing when
>> doing cast, but not later when we getLValueElement for p[1].
>>
>> Then when we getLValueElement, we abandon (emit warning or retun unknown)
>> when the base region is not an ElementRegion.
>>
>
> I honestly don't think we can have the invariant that getLValueElement is
> always invoked on a MemRegion that is an ElementRegion.  Consider the
> following examples:
>
> Example 1:
>
> int foo(int *x) {
>  return x[0];
> }
>
> In Example 1, 'x' binds to a loc::SymbolVal.  The region associated with
> that symbol is a SymbolicRegion (let's call it SymR).  How do we ensure the
> an ElementRegion whose base is SymR and index 0 is passed to
> getLValueElement?  Honestly, what does this buy us?  If we pass a
> non-ElementRegion to getLvalueElement, it's a synonym for
> ElementRegion(SymR, 0).
>
> Example 2:
>
> int bar() {
>  char x;
>  char *p = &x;
>  return p[0];
> }
>
> In this case we have a VarRegion for x (let's denote that VarRegion(x)).
>  After the second line 'p' binds to loc::MemRegionVal(VarRegion(x)).  This
> value is then passed to getLValueElement.  Essentially this is the same as
> passing ElementRegion(VarRegion(x), 0).
>
> I'm sure we can come up with many more cases like this.  What does creating
> an ElementRegion (with base 0) before calling getLValueElement buy us?  This
> logic could certainly be done in GRStateManager, just before it calls
> StoreManager::getLValueElement.  Maybe that would simplify some things.
>
> Creating an ElementRegion when we do the cast from the return value of
> alloca() to another type doesn't make much sense to me at all.  When we see
> code like the following, 'p' binds to the *entire* AnonTypedRegion, not just
> a specific element:
>
>  char *p = (char*) alloca(1000);
>
> I just semantically adds an unnecessary veneer to have 'p' bind to
> ElementRegion(AnonTypedRegion(AllocaRegion(...)), 0) instead of just
> AnonTypedRegion(...).  I argue that this veneer buys us nothing since we
> have to handle all the other cases I mentioned above anyway in some uniform
> fashion.
>
>
>> As the rabbit hole goes arbitrary deep here, I suggest we abandon early
>> when we getLValueElement for wonky[1]. For this example, I think the region
>> of _wonky_gesticulate_cheese should be the brother but not the parent of the
>> region for wonky[1].
>>
>
> I'm not certain what you mean by the "brother."  We don't have that notion
> (yet) in MemRegion.  Can you elucidate?  What would this buy us?  What would
> be the parent of ElementRegion(_wonky_gesticulate_cheese, 1) (i.e.,
> "wonky[1])?
>
>  But when we getLValueElement for wonky[1], we actually use the region of
>> _wonky_gesticulate_cheese as the super region for the element region.
>>
>
> I'm not certain what this dual view buys us.  The variable 'wonky' has the
> region 'VarRegion(wonky)'.  That region binds to the location of
> _wonky_gesticulate_cheese.  The lvalue of 'wonky[1]' evaluates to
> ElementRegion(_wonky_gesticulate_cheese, 1).  Wouldn't the parent of the
> later just be _wonky_gesticulate_cheese in all cases?
>
>  Then it goes to my original argument for guaranteeing all base regions
>> passed to getLValueElement() to be ElementRegions, since we do not have
>> enough information in getLValueElement() for us to make the right decision.
>>
>
> I feel like I'm missing something here.  What information are we missing to
> make the right decision?
>
> Put another way, what problem are you trying to solve by imposing the
> precondition to getLValueElement that it is always passed an ElementRegion?
>  As I said earlier, we can enforce this precondition by adding some wrapper
> logic in GRStateManager, but all that would be is code refactoring.  From my
> understanding of things now, all of the information necessary to make the
> "right" decision can be done in getLValueElement.
>
>  For code
>>
>> char * p = (char*) alloc(4);
>> char c = p[1];
>>
>> we make p points to an element region when we do the cast.
>>
>
> I honestly feel that special casing this example buys us nothing (given the
> examples I provided above), and only adds complexity and extra space
> overhead.
>
> Don't get me wrong, I think we need some nothing of an "extended" type
> system when doing analysis.  Some casts could be deemed "okay" and others
> not so much.  Unions represent a good example where memory can be reused for
> different data types, so we're just going to have to make some choices at
> some point of how to handle pieces of memory that are used in a conflated
> manner.


Hi Ted,

I agree with you now. Some "extended" type system should be the right
direction.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090203/2142d36b/attachment.html>