<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Jan 4, 2015 at 6:00 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Sun, Jan 4, 2015 at 4:30 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Maybe we should fix this when we create the Expr rather than putting parse error recovery logic in AST?</p></blockquote></span><div>Is that how this is usually done? I thought usually AST nodes for invalid code end up with invalid SourceLocs.</div></div></div></div></blockquote><div><br></div><div>Well, invalid expressions usually don't create AST nodes at all. I think these are reasonable guiding principles:</div><div><br></div><div>- error recovery code should be limited in scope to the code that performs the error recovery</div><div>- 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</div><div>- AST nodes (even those created by error recovery) should have valid source locations so that later diagnostics have somewhere to point</div><div><span style="color:rgb(80,0,80)"><br></span></div><div><span style="color:rgb(80,0,80)">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).</span></div><div><span style="color:rgb(80,0,80)"> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
<div class="gmail_quote">On 4 Jan 2015 20:37, "Nico Weber" <<a href="mailto:nicolasweber@gmx.de" target="_blank">nicolasweber@gmx.de</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: nico<br>
Date: Sun Jan 4 14:32:12 2015<br>
New Revision: 225140<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=225140&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=225140&view=rev</a><br>
Log:<br>
Remove an assert that's not true on invalid code.<br>
<br>
r185773 added an assert that checked that a CXXUnresolvedConstructExpr either<br>
has a valid rparen, or exactly one argument. This doesn't have to be true for<br>
invalid inputs. Convert the assert to an if, and add a test for this case.<br>
<br>
Found by SLi's afl bot.<br>
<br>
Added:<br>
cfe/trunk/test/Misc/ast-dump-invalid.cpp<br>
Modified:<br>
cfe/trunk/include/clang/AST/ExprCXX.h<br>
cfe/trunk/test/SemaCXX/return.cpp<br>
<br>
Modified: cfe/trunk/include/clang/AST/ExprCXX.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprCXX.h?rev=225140&r1=225139&r2=225140&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprCXX.h?rev=225140&r1=225139&r2=225140&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/AST/ExprCXX.h (original)<br>
+++ cfe/trunk/include/clang/AST/ExprCXX.h Sun Jan 4 14:32:12 2015<br>
@@ -2915,8 +2915,9 @@ public:<br>
<br>
SourceLocation getLocStart() const LLVM_READONLY;<br>
SourceLocation getLocEnd() const LLVM_READONLY {<br>
- assert(RParenLoc.isValid() || NumArgs == 1);<br>
- return RParenLoc.isValid() ? RParenLoc : getArg(0)->getLocEnd();<br>
+ if (!RParenLoc.isValid() && NumArgs > 0)<br>
+ return getArg(NumArgs - 1)->getLocEnd();<br>
+ return RParenLoc;<br>
}<br>
<br>
static bool classof(const Stmt *T) {<br>
<br>
Added: cfe/trunk/test/Misc/ast-dump-invalid.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-invalid.cpp?rev=225140&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-invalid.cpp?rev=225140&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Misc/ast-dump-invalid.cpp (added)<br>
+++ cfe/trunk/test/Misc/ast-dump-invalid.cpp Sun Jan 4 14:32:12 2015<br>
@@ -0,0 +1,20 @@<br>
+// 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<br>
+<br>
+namespace TestInvalidRParenOnCXXUnresolvedConstructExpr {<br>
+template <class T><br>
+void f(T i, T j) {<br>
+ return T (i, j;<br>
+}<br>
+}<br>
+<br>
+// CHECK: NamespaceDecl {{.*}} <{{.*}}> {{.*}} TestInvalidRParenOnCXXUnresolvedConstructExpr<br>
+// CHECK-NEXT: `-FunctionTemplateDecl<br>
+// CHECK-NEXT: |-TemplateTypeParmDecl<br>
+// CHECK-NEXT: `-FunctionDecl<br>
+// CHECK-NEXT: |-ParmVarDecl<br>
+// CHECK-NEXT: |-ParmVarDecl<br>
+// CHECK-NEXT: `-CompoundStmt<br>
+// CHECK-NEXT: `-ReturnStmt<br>
+// CHECK-NEXT: `-CXXUnresolvedConstructExpr {{.*}} <col:10, col:16> 'T'<br>
+// CHECK-NEXT: |-DeclRefExpr {{.*}} <col:13> 'T' lvalue ParmVar {{.*}} 'i' 'T'<br>
+// CHECK-NEXT: `-DeclRefExpr {{.*}} <col:16> 'T' lvalue ParmVar {{.*}} 'j' 'T'<br>
<br>
Modified: cfe/trunk/test/SemaCXX/return.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return.cpp?rev=225140&r1=225139&r2=225140&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return.cpp?rev=225140&r1=225139&r2=225140&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/return.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/return.cpp Sun Jan 4 14:32:12 2015<br>
@@ -112,3 +112,11 @@ namespace ctor_returns_void {<br>
~S() { return f(); } // expected-error {{destructor '~S' must not return void expression}}<br>
};<br>
}<br>
+<br>
+void cxx_unresolved_expr() {<br>
+ // The use of an undeclared variable tricks clang into building a<br>
+ // CXXUnresolvedConstructExpr, and the missing ')' gives it an invalid source<br>
+ // location for its rparen. Check that emitting a diag on the range of the<br>
+ // expr doesn't assert.<br>
+ 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'}}<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>