[cfe-commits] [PATCH] RegionStore

[Zhongxing Xu]

On Tue, Oct 21, 2008 at 12:32 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Oct 20, 2008, at 3:31 AM, Zhongxing Xu wrote:
>
>  Some more comments:
>>
>> int a[10];
>> int (*p)[10];
>>
>> Per C99 6.3.2p3, 'a' and '*p' is not lvalue when they are placed in a[1]
>> or (*p)[1]. So in fact we cannot do lvalue evaluation of them.
>>
>
> I'm looking at the section, and I'm not seeing the actual clause that your
> are referencing.  Is there a particular sentence or paragraph you are
> referring to?  I think this is the phrase:
>
> "Except when it is the operand of the sizeof operator or the unary &
> operator, or is a  string literal used to initialize an array, an expression
> that has type ''array of type'' is  converted to an expression with type
> ''pointer to type'' that points to the initial element of the array object
> and is not an lvalue."


I think I made a mistake when understanding the last "and is not an lvalue".
It should refer to the expression after conversion.


>
>
> This conversion is handled by the ImplicitCastExpr.  It seems fairly
> straightforward to me to have VisitCast() call VisitLValue() on its child
> expression when doing an array to pointer conversion.
>
>  But this is inconsistent with the usual case in VisitDeclRefExpr and
>> VisitUnaryExpr. But 'a' and '*p' do have lvalue sometimes.
>>
>
> Perhaps, but arrays really are a special case, both in the language syntax
> and the C type system.  It's not surprising to me that we need to handle
> them specifically.
>
>  And special handling of these non-lvalue cases in the GRExprEngine is
>> ugly.
>>
>
> I agree it isn't pleasant, but the ugliness is highly localized.  I believe
> it's much better to put this ugliness in one place (the core analysis
> engine) rather than require all implementations of Store to get it right.
>  GRExprEngine already takes care of lots of details like this.


Agree.


>
>  On the other hand, a simple trick in the Store will do us the favor. If we
>> are passed loc::MemRegionVal(some array's region) in the store, we do not go
>> through the binding, but simply return loc::MemRegionVal(some array's
>> region) as the loaded value. Then we can do the usual thing for all cases in
>> GRExprEngine's Visitors: get the lvalue of the Decl, ask Store what its
>> rvalue is.
>>
>
> Understood.  Three comments.
>
> First, having the Store do this means that *every* implementation of Store
> will have to specially implement this corner case.  By doing it outside of
> Store (i.e., in GRExprEngine), we might need to work hard to get it right,
> but once we do it will be right for all implementations of Store.  It's not
> an obvious thing for a Store to do, which means people will likely get it
> wrong.
>
> Second, after our long thread a week ago, my feeling is that the Store has
> no notion of lvalues or rvalues, only locations and the values that are
> binded to those locations.  In this regards, I believe that GetSVal() should
> only return the value that is bound to a region.  The implementation of
> GetSVal() should not have to guess of whether it is called to return an
> lvalue or an rvalue given the type of a region.  That just doesn't seem very
> clean to me, and violates the separation of concerns between handling all
> the nastiness of C's syntax in GRExprEngine and handling the semantics of
> value bindings in Store.
>
> Third, what regions a Store returns to callers (via getLValueXXX) and the
> meaning of those regions is entirely up to the Store.  To me it makes no
> sense to call GetSval() on a VarRegion for an array variable because the
> operation of "loading from an array" doesn't make sense, i.e., there is no C
> syntax that allows one to "load array" (or "get binding"); we can load an
> element of an array, but not the array itself. The only way for GetSVal() to
> make sense in this context is for it to return an lvalue, but that really
> isn't the binding.  Perhaps you have a different interpretation of
> GetSVal(), but to me just keeping it to mean "fetch the value bound to this
> location" keeps things clean and simple.
>
> Although it's more wordy, perhaps we should rename GetSVal() to
> GetBinding() just to make the meaning of this method more clear.

Agree.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081021/2419f56f/attachment.html>