[cfe-dev] fix for Clang PR 8419

[Zhanyong Wan (λx.x x)]

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.

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

> 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