<br><br><div class="gmail_quote">On Tue, Oct 21, 2008 at 12:32 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d"><br>
On Oct 20, 2008, at 3:31 AM, Zhongxing Xu wrote:<br>
<br>
</div><div class="Ih2E3d"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Some more comments:<br>
<br>
int a[10];<br>
int (*p)[10];<br>
<br>
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.<br>
</blockquote>
<br></div>
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:<br>
<br>
"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."</blockquote>
<div><br>I think I made a mistake when understanding the last "and is not an lvalue". It should refer to the expression after conversion.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
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.<div class="Ih2E3d"><br>

<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
But this is inconsistent with the usual case in VisitDeclRefExpr and VisitUnaryExpr. But 'a' and '*p' do have lvalue sometimes.<br>
</blockquote>
<br></div>
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.<div class="Ih2E3d"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
And special handling of these non-lvalue cases in the GRExprEngine is ugly.<br>
</blockquote>
<br></div>
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.</blockquote>
<div><br>Agree. <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br>
<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
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.<br>

</blockquote>
<br></div>
Understood.  Three comments.<br>
<br>
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.<br>

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

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

<br>
Although it's more wordy, perhaps we should rename GetSVal() to GetBinding() just to make the meaning of this method more clear.</blockquote><div>Agree. <br></div></div><br>