r225140 - Remove an assert that's not true on invalid code.

[Nico Weber]

On Sun, Jan 4, 2015 at 4:30 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> Maybe we should fix this when we create the Expr rather than putting parse
> error recovery logic in AST?
>
Is that how this is usually done? I thought usually AST nodes for invalid
code end up with invalid SourceLocs.


> On 4 Jan 2015 20:37, "Nico Weber" <nicolasweber at gmx.de> wrote:
>
>> Author: nico
>> Date: Sun Jan  4 14:32:12 2015
>> New Revision: 225140
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=225140&view=rev
>> Log:
>> Remove an assert that's not true on invalid code.
>>
>> r185773 added an assert that checked that a CXXUnresolvedConstructExpr
>> either
>> has a valid rparen, or exactly one argument.  This doesn't have to be
>> true for
>> invalid inputs.  Convert the assert to an if, and add a test for this
>> case.
>>
>> Found by SLi's afl bot.
>>
>> Added:
>>     cfe/trunk/test/Misc/ast-dump-invalid.cpp
>> Modified:
>>     cfe/trunk/include/clang/AST/ExprCXX.h
>>     cfe/trunk/test/SemaCXX/return.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/ExprCXX.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprCXX.h?rev=225140&r1=225139&r2=225140&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/ExprCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/ExprCXX.h Sun Jan  4 14:32:12 2015
>> @@ -2915,8 +2915,9 @@ public:
>>
>>    SourceLocation getLocStart() const LLVM_READONLY;
>>    SourceLocation getLocEnd() const LLVM_READONLY {
>> -    assert(RParenLoc.isValid() || NumArgs == 1);
>> -    return RParenLoc.isValid() ? RParenLoc : getArg(0)->getLocEnd();
>> +    if (!RParenLoc.isValid() && NumArgs > 0)
>> +      return getArg(NumArgs - 1)->getLocEnd();
>> +    return RParenLoc;
>>    }
>>
>>    static bool classof(const Stmt *T) {
>>
>> Added: cfe/trunk/test/Misc/ast-dump-invalid.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-invalid.cpp?rev=225140&view=auto
>>
>> ==============================================================================
>> --- cfe/trunk/test/Misc/ast-dump-invalid.cpp (added)
>> +++ cfe/trunk/test/Misc/ast-dump-invalid.cpp Sun Jan  4 14:32:12 2015
>> @@ -0,0 +1,20 @@
>> +// RUN: not %clang_cc1 -std=c++11 -triple x86_64-linux-gnu
>> -fms-extensions -ast-dump -ast-dump-filter Test %s | FileCheck
>> -check-prefix CHECK -strict-whitespace %s
>> +
>> +namespace TestInvalidRParenOnCXXUnresolvedConstructExpr {
>> +template <class T>
>> +void f(T i, T j) {
>> +  return T (i, j;
>> +}
>> +}
>> +
>> +// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}}
>> TestInvalidRParenOnCXXUnresolvedConstructExpr
>> +// CHECK-NEXT: `-FunctionTemplateDecl
>> +// CHECK-NEXT:   |-TemplateTypeParmDecl
>> +// CHECK-NEXT:   `-FunctionDecl
>> +// CHECK-NEXT:     |-ParmVarDecl
>> +// CHECK-NEXT:     |-ParmVarDecl
>> +// CHECK-NEXT:     `-CompoundStmt
>> +// CHECK-NEXT:       `-ReturnStmt
>> +// CHECK-NEXT:         `-CXXUnresolvedConstructExpr {{.*}} <col:10,
>> col:16> 'T'
>> +// CHECK-NEXT:           |-DeclRefExpr {{.*}} <col:13> 'T' lvalue
>> ParmVar {{.*}} 'i' 'T'
>> +// CHECK-NEXT:           `-DeclRefExpr {{.*}} <col:16> 'T' lvalue
>> ParmVar {{.*}} 'j' 'T'
>>
>> Modified: cfe/trunk/test/SemaCXX/return.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return.cpp?rev=225140&r1=225139&r2=225140&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/return.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/return.cpp Sun Jan  4 14:32:12 2015
>> @@ -112,3 +112,11 @@ namespace ctor_returns_void {
>>      ~S() { return f(); } // expected-error {{destructor '~S' must not
>> return void expression}}
>>    };
>>  }
>> +
>> +void cxx_unresolved_expr() {
>> +  // The use of an undeclared variable tricks clang into building a
>> +  // CXXUnresolvedConstructExpr, and the missing ')' gives it an invalid
>> source
>> +  // location for its rparen.  Check that emitting a diag on the range
>> of the
>> +  // expr doesn't assert.
>> +  return int(undeclared, 4; // expected-error {{expected ')'}}
>> expected-note{{to match this '('}} expected-error {{void function
>> 'cxx_unresolved_expr' should not return a value}} expected-error {{use of
>> undeclared identifier 'undeclared'}}
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150104/03febdfa/attachment.html>