On Sun, Jan 4, 2015 at 6:00 PM, Nico Weber <[email protected]> wrote:
> On Sun, Jan 4, 2015 at 4:30 PM, Richard Smith <[email protected]> > 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" <[email protected]> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
