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

[Richard Smith]

On Sun, Jan 4, 2015 at 6:00 PM, Nico Weber <thakis at chromium.org> wrote:

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

Well, invalid expressions usually don't create AST nodes at all. I think
these are reasonable guiding principles:

- error recovery code should be limited in scope to the code that performs
the error recovery
- after error recovery, we should form "approximately" valid AST so that
consumers of the AST don't need to worry about whether they deal correctly
with broken AST fragments
- AST nodes (even those created by error recovery) should have valid source
locations so that later diagnostics have somewhere to point

I would expect there are places where we don't follow these principles (and
I'd expect many of those places result in bugs like the one you were fixing
here).


> 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/20150105/878f8ee7/attachment.html>