[cfe-commits] [PATCH] RegionStore

[Ted Kremenek]

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."

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.

> 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.