[cfe-dev] fix for Clang PR 8419

[Ted Kremenek]

On Nov 19, 2010, at 10:38 AM, Zhanyong Wan (λx.x x) wrote:

> 2. The meat of the fix is in CFG.cpp.  I'm still not sure if this is a
> principled fix, but all tests pass, so I hope it's at least an
> improvement.


Hi Zhanyong,

More comments on this part of your patch:

> +// Returns true if a CFGBuilder should treat 'S' as an L-value when
> +// visiting it.
> +static bool ShouldTreatAsLValue(Stmt* S) {
> +  // A call to a reference-returning function is an L-value.
> +  if (CallExpr* CallEx = dyn_cast<CallExpr>(S))
> +    return CallEx->getCallReturnType()->isReferenceType();
> +
> +  // Otherwise, we assume it's not an L-value.
> +  // FIXME: document why this assumption is OK, or fix this in a
> +  // principled way if it's not.
> +  return false;
> +}
> +
>  /// VisitChildren - Visit the children of a Stmt.
>  CFGBlock *CFGBuilder::VisitChildren(Stmt* Terminator) {
>    CFGBlock *B = Block;
>    for (Stmt::child_iterator I = Terminator->child_begin(),
>           E = Terminator->child_end(); I != E; ++I) {
> -    if (*I) B = Visit(*I);
> +    if (*I) {
> +      B = Visit(*I, ShouldTreatAsLValue(*I) ?
> +                AddStmtChoice::AsLValueNotAlwaysAdd :
> +                AddStmtChoice::NotAlwaysAdd);
> +    }
>    }
>    return B;
>  }

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.

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

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20101119/7c2bf0d5/attachment.html>