<br><br><div class="gmail_quote">2010/11/20 Zhanyong Wan (λx.x x) <span dir="ltr"><<a href="mailto:wan@google.com">wan@google.com</a>></span><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi Ted,<br>
<br>
Thanks for the quick feedback! I'll respond to your comments about<br>
the code comments later, as they are less important.<br>
<div class="im"><br>
On Fri, Nov 19, 2010 at 2:09 PM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br>
> On Nov 19, 2010, at 10:38 AM, Zhanyong Wan (λx.x x) wrote:<br>
><br>
</div><div class="im">> I think the motivation behind this change is right, but not the change<br>
> itself.<br>
> The problem with VisitChildren() is that it has no context of whether<br>
> 'Terminator' (which is misnamed) expects the subexpression to be evaluated<br>
> as an lvalue. Since your logic is specific to CallExpr, it really could<br>
> just be sunk into processing of CallExpr itself. When the caller passses<br>
> down 'AsLValue', it is indicating that the parent expects that the child is<br>
> evaluated as an lvalue, but that's based on the parent's context. There is<br>
> no such information here.<br>
<br>
</div>Thanks for explaining. It's a revelation to me to learn that:<br>
<br>
1. The "AsLValue" bit encodes whether the *consumer* of an expression<br>
uses it as an LValue, not whether the expression itself is an LValue.<br></blockquote><div><br>This is right.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
2. The caller of Visit*() must provide this bit accurately. It's not<br>
OK for the caller to be "conservative" (i.e. say "AsLValue" when<br>
the consumer of the expression only needs an RValue).<br></blockquote><div><br>I don't quite understand this point.<br><br>Just like Ted said, the first step of the fix is: <br><br>when the 'foo()' is added as the block-level expr in CFG construction, its kind should be StatementAsLValue, and this should be confined in the unary operator case.<br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
I suspected #1 was the case, but wasn't certain. I wasn't sure about<br>
#2 at all (hence my patch is the way it is). The fact that all tests<br>
passed with the patch led me to think that it's fine for the caller of<br>
Visit*() to say AsLValue when only an RValue is needed. Now I see<br>
that it's just inadequate test coverage.<br>
<br>
I'll document this in my new patch. Also, how would you suggest to<br>
add a test to catch violation of #2?<br>
<div><div></div><div class="h5"><br>
> Further, I think it's no technically correct. Consider:<br>
> const int &foo();<br>
> int i = foo();<br>
> This is what the AST looks like for the second line:<br>
> (DeclStmt 0x101842c18 <line:3:3, col:16><br>
> 0x101818510 "int i =<br>
> (CallExpr 0x1018185d8 <col:11, col:15> 'const int' lvalue<br>
> (ImplicitCastExpr 0x1018185b8 <col:11> 'const int &(*)(void)'<br>
> <FunctionToPointerDecay><br>
> (DeclRefExpr 0x101818560 <col:11> 'const int &(void)' lvalue<br>
> FunctionDecl='foo' 0x101818450)))"))<br>
> In this example, CallExpr does indeed return an lvalue. The problem is that<br>
> there is an implicit lvalue-to-rvalue conversion. So the semantics really<br>
> are:<br>
> (DeclStmt 0x101842c18 <line:3:3, col:16><br>
> 0x101818510 "int i =<br>
> (RValueConversion <--- I made this up<br>
> (CallExpr 0x1018185d8 <col:11, col:15> 'const int' lvalue<br>
> (ImplicitCastExpr 0x1018185b8 <col:11> 'const int &(*)(void)'<br>
> <FunctionToPointerDecay><br>
> (DeclRefExpr 0x101818560 <col:11> 'const int &(void)' lvalue<br>
> FunctionDecl='foo' 0x101818450)))"))<br>
> Currently we don't represent lvalue-to-rvalue conversions explicitly in the<br>
> AST, although in my conversations with John McCall there is interest in<br>
> doing this. The problem with your patch is that the parent expression may<br>
> actually expect that there is an rvalue, even though the subexpression<br>
> returns an lvalue. Your patch doesn't actually address this problem at all,<br>
> but actually papers over it.<br>
> Codegen goes through some histrionics to infer these semantics from the AST,<br>
> but they analyzer mostly goes off the AST itself for semantics. As you can<br>
> see this is a problem, since we really have no "sequence point" for the<br>
> analyzer to do the lvalue-to-rvalue conversion. We get around this<br>
> limitation in various places when it comes to C-style constructs, but there<br>
> are many corner cases for C++.<br>
<br>
</div></div>This is really unfortunate. :-(<br>
<div class="im"><br>
> In this particular case, I think we need to add special support in the CFG<br>
> builder for UnaryOperator. Specifically, we need to add a<br>
> 'VisitUnaryOperator' to the CFGBuilder class, and have it determine, base on<br>
> the opcode, whether or not the operation requires the subexpression to be an<br>
> lvalue or rvalue. It then passes the appropriate value for AddStmtChoice.<br>
<br>
</div>I'll try that.<br>
<br>
One question: for the purpose of the CFG builder, does an LValue have<br>
to be mutable? For example, given<br>
<br>
void Foo(const Bar& x) { ... }<br>
Bar& GetBar() { ... }<br>
<br>
when the analyzer sees "Foo(GetBar());", should it treat "GetBar()" as<br>
an LValue or RValue? What if the implementation of Foo() actually<br>
cares about the identity of the input, e.g.<br>
<br>
void Foo(const Bar& x) {<br>
if (&x == &someGlobalBarVairalbe)<br>
...;<br>
}<br>
<br>
? Thanks.<br></blockquote><div><br>In this context, we evaluate GetBar() to its l-value, i.e., a MemRegion.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
> Similarly, for CallExprs, any CallExpr that returns a reference needs to be<br>
> treated as returning an lvalue. The problem here is that we don't have a<br>
> sequence point for doing arbitrary lvalue-to-rvalue conversion in the AST,<br>
> but this is something we can possibly model in GRExprEngine given enough<br>
> breadcrumbs in the CFG.<br>
<br>
</div><div><div></div><div class="h5">--<br>
Zhanyong<br>
<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</div></div></blockquote></div><br>