[cfe-dev] fix for Clang PR 8419

[Zhongxing Xu]

2010/11/20 Zhanyong Wan (λx.x x) <wan at google.com>

> Hi Ted,
>
> Thanks for the quick feedback!  I'll respond to your comments about
> the code comments later, as they are less important.
>
> On Fri, Nov 19, 2010 at 2:09 PM, Ted Kremenek <kremenek at apple.com> wrote:
> > On Nov 19, 2010, at 10:38 AM, Zhanyong Wan (λx.x x) wrote:
> >
> > I think the motivation behind this change is right, but not the change
> > itself.
> > The problem with VisitChildren() is that it has no context of whether
> > 'Terminator' (which is misnamed) expects the subexpression to be
> evaluated
> > as an lvalue.  Since your logic is specific to CallExpr, it really could
> > just be sunk into processing of CallExpr itself.  When the caller passses
> > down 'AsLValue', it is indicating that the parent expects that the child
> is
> > evaluated as an lvalue, but that's based on the parent's context.  There
> is
> > no such information here.
>
> Thanks for explaining.  It's a revelation to me to learn that:
>
> 1. The "AsLValue" bit encodes whether the *consumer* of an expression
>   uses it as an LValue, not whether the expression itself is an LValue.
>

This is right.


>
> 2. The caller of Visit*() must provide this bit accurately.  It's not
>   OK for the caller to be "conservative" (i.e. say "AsLValue" when
>   the consumer of the expression only needs an RValue).
>

I don't quite understand this point.

Just like Ted said, the first step of the fix is:

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.


>
> I suspected #1 was the case, but wasn't certain.  I wasn't sure about
> #2 at all (hence my patch is the way it is).  The fact that all tests
> passed with the patch led me to think that it's fine for the caller of
> Visit*() to say AsLValue when only an RValue is needed.  Now I see
> that it's just inadequate test coverage.
>
> I'll document this in my new patch.  Also, how would you suggest to
> add a test to catch violation of #2?
>
> > Further, I think it's no technically correct.  Consider:
> > const int &foo();
> > int i = foo();
> > This is what the AST looks like for the second line:
> >   (DeclStmt 0x101842c18 <line:3:3, col:16>
> >     0x101818510 "int i =
> >       (CallExpr 0x1018185d8 <col:11, col:15> 'const int' lvalue
> >         (ImplicitCastExpr 0x1018185b8 <col:11> 'const int &(*)(void)'
> > <FunctionToPointerDecay>
> >           (DeclRefExpr 0x101818560 <col:11> 'const int &(void)' lvalue
> > FunctionDecl='foo' 0x101818450)))"))
> > In this example, CallExpr does indeed return an lvalue.  The problem is
> that
> > there is an implicit lvalue-to-rvalue conversion.  So the semantics
> really
> > are:
> >   (DeclStmt 0x101842c18 <line:3:3, col:16>
> >     0x101818510 "int i =
> >       (RValueConversion <--- I made this up
> >         (CallExpr 0x1018185d8 <col:11, col:15> 'const int' lvalue
> >           (ImplicitCastExpr 0x1018185b8 <col:11> 'const int &(*)(void)'
> > <FunctionToPointerDecay>
> >             (DeclRefExpr 0x101818560 <col:11> 'const int &(void)' lvalue
> > FunctionDecl='foo' 0x101818450)))"))
> > Currently we don't represent lvalue-to-rvalue conversions explicitly in
> the
> > AST, although in my conversations with John McCall there is interest in
> > doing this.  The problem with your patch is that the parent expression
> may
> > actually expect that there is an rvalue, even though the subexpression
> > returns an lvalue.  Your patch doesn't actually address this problem at
> all,
> > but actually papers over it.
> > Codegen goes through some histrionics to infer these semantics from the
> AST,
> > but they analyzer mostly goes off the AST itself for semantics.  As you
> can
> > see this is a problem, since we really have no "sequence point" for the
> > analyzer to do the lvalue-to-rvalue conversion.  We get around this
> > limitation in various places when it comes to C-style constructs, but
> there
> > are many corner cases for C++.
>
> This is really unfortunate. :-(
>
> > In this particular case, I think we need to add special support in the
> CFG
> > builder for UnaryOperator.  Specifically, we need to add a
> > 'VisitUnaryOperator' to the CFGBuilder class, and have it determine, base
> on
> > the opcode, whether or not the operation requires the subexpression to be
> an
> > lvalue or rvalue.  It then passes the appropriate value for
> AddStmtChoice.
>
> I'll try that.
>
> One question: for the purpose of the CFG builder, does an LValue have
> to be mutable?  For example, given
>
>  void Foo(const Bar& x) { ... }
>  Bar& GetBar() { ... }
>
> when the analyzer sees "Foo(GetBar());", should it treat "GetBar()" as
> an LValue or RValue?  What if the implementation of Foo() actually
> cares about the identity of the input, e.g.
>
>  void Foo(const Bar& x) {
>    if (&x == &someGlobalBarVairalbe)
>      ...;
>  }
>
> ?  Thanks.
>

In this context, we evaluate GetBar() to its l-value, i.e., a MemRegion.


>
> > Similarly, for CallExprs, any CallExpr that returns a reference needs to
> be
> > treated as returning an lvalue.  The problem here is that we don't have a
> > sequence point for doing arbitrary lvalue-to-rvalue conversion in the
> AST,
> > but this is something we can possibly model in GRExprEngine given enough
> > breadcrumbs in the CFG.
>
> --
> Zhanyong
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101120/c354b64b/attachment.html>