<div dir="ltr">Hi Hans,<div><br></div><div>I'd like to get this into Clang 5, but it's not entirely risk-free. Perhaps we could leave it in the tree for a little while and then merge if it seems OK?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 11 August 2017 at 18:46, Richard Smith via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rsmith<br>
Date: Fri Aug 11 18:46:03 2017<br>
New Revision: 310776<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=310776&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=310776&view=rev</a><br>
Log:<br>
PR34163: Don't cache an incorrect key function for a class if queried between<br>
the class becoming complete and its inline methods being parsed.<br>
<br>
This replaces the hack of using the "late parsed template" flag to track member<br>
functions with bodies we've not parsed yet; instead we now use the "will have<br>
body" flag, which carries the desired implication that the function declaration<br>
*is* a definition, and that we've just not parsed its body yet.<br>
<br>
Added:<br>
cfe/trunk/test/CodeGenCXX/<wbr>pr34163.cpp<br>
Modified:<br>
cfe/trunk/include/clang/AST/<wbr>Decl.h<br>
cfe/trunk/lib/AST/DeclCXX.cpp<br>
cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp<br>
cfe/trunk/lib/Sema/SemaDecl.<wbr>cpp<br>
cfe/trunk/lib/Sema/<wbr>SemaTemplateInstantiateDecl.<wbr>cpp<br>
cfe/trunk/test/SemaCUDA/<a href="http://function-overload.cu" rel="noreferrer" target="_blank">functi<wbr>on-overload.cu</a><br>
cfe/trunk/test/SemaCUDA/<a href="http://no-destructor-overload.cu" rel="noreferrer" target="_blank">no-<wbr>destructor-overload.cu</a><br>
<br>
Modified: cfe/trunk/include/clang/AST/<wbr>Decl.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/AST/Decl.h?rev=310776&<wbr>r1=310775&r2=310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/AST/<wbr>Decl.h (original)<br>
+++ cfe/trunk/include/clang/AST/<wbr>Decl.h Fri Aug 11 18:46:03 2017<br>
@@ -1666,8 +1666,7 @@ private:<br>
unsigned HasSkippedBody : 1;<br>
<br>
/// Indicates if the function declaration will have a body, once we're done<br>
- /// parsing it. (We don't set it to false when we're done parsing, in the<br>
- /// hopes this is simpler.)<br>
+ /// parsing it.<br>
unsigned WillHaveBody : 1;<br>
<br>
/// \brief End part of this FunctionDecl's source range.<br>
<br>
Modified: cfe/trunk/lib/AST/DeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/AST/<wbr>DeclCXX.cpp?rev=310776&r1=<wbr>310775&r2=310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/AST/DeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/AST/DeclCXX.cpp Fri Aug 11 18:46:03 2017<br>
@@ -1837,9 +1837,10 @@ bool CXXMethodDecl::hasInlineBody() cons<br>
const FunctionDecl *CheckFn = getTemplateInstantiationPatter<wbr>n();<br>
if (!CheckFn)<br>
CheckFn = this;<br>
-<br>
+<br>
const FunctionDecl *fn;<br>
- return CheckFn->hasBody(fn) && !fn->isOutOfLine();<br>
+ return CheckFn->isDefined(fn) && !fn->isOutOfLine() &&<br>
+ (fn-><wbr>doesThisDeclarationHaveABody() || fn->willHaveBody());<br>
}<br>
<br>
bool CXXMethodDecl::<wbr>isLambdaStaticInvoker() const {<br>
<br>
Modified: cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp?rev=<wbr>310776&r1=310775&r2=310776&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp (original)<br>
+++ cfe/trunk/lib/Parse/<wbr>ParseCXXInlineMethods.cpp Fri Aug 11 18:46:03 2017<br>
@@ -166,20 +166,11 @@ NamedDecl *Parser::ParseCXXInlineMethodD<br>
}<br>
<br>
if (FnD) {<br>
- // If this is a friend function, mark that it's late-parsed so that<br>
- // it's still known to be a definition even before we attach the<br>
- // parsed body. Sema needs to treat friend function definitions<br>
- // differently during template instantiation, and it's possible for<br>
- // the containing class to be instantiated before all its member<br>
- // function definitions are parsed.<br>
- //<br>
- // If you remove this, you can remove the code that clears the flag<br>
- // after parsing the member.<br>
- if (D.getDeclSpec().<wbr>isFriendSpecified()) {<br>
- FunctionDecl *FD = FnD->getAsFunction();<br>
- Actions.<wbr>CheckForFunctionRedefinition(<wbr>FD);<br>
- FD->setLateTemplateParsed(<wbr>true);<br>
- }<br>
+ FunctionDecl *FD = FnD->getAsFunction();<br>
+ // Track that this function will eventually have a body; Sema needs<br>
+ // to know this.<br>
+ Actions.<wbr>CheckForFunctionRedefinition(<wbr>FD);<br>
+ FD->setWillHaveBody(true);<br>
} else {<br>
// If semantic analysis could not build a function declaration,<br>
// just throw away the late-parsed declaration.<br>
@@ -559,10 +550,6 @@ void Parser::ParseLexedMethodDef(<wbr>LexedMe<br>
<br>
ParseFunctionStatementBody(LM.<wbr>D, FnScope);<br>
<br>
- // Clear the late-template-parsed bit if we set it before.<br>
- if (LM.D)<br>
- LM.D->getAsFunction()-><wbr>setLateTemplateParsed(false);<br>
-<br>
while (Tok.isNot(tok::eof))<br>
ConsumeAnyToken();<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDecl.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaDecl.cpp?rev=310776&r1=<wbr>310775&r2=310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/SemaDecl.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDecl.<wbr>cpp Fri Aug 11 18:46:03 2017<br>
@@ -12090,8 +12090,9 @@ Decl *Sema::<wbr>ActOnStartOfFunctionDef(Scop<br>
FD->setInvalidDecl();<br>
}<br>
<br>
- // See if this is a redefinition.<br>
- if (!FD->isLateTemplateParsed()) {<br>
+ // See if this is a redefinition. If 'will have body' is already set, then<br>
+ // these checks were already performed when it was set.<br>
+ if (!FD->willHaveBody() && !FD->isLateTemplateParsed()) {<br>
CheckForFunctionRedefinition(<wbr>FD, nullptr, SkipBody);<br>
<br>
// If we're skipping the body, we're done. Don't enter the scope.<br>
<br>
Modified: cfe/trunk/lib/Sema/<wbr>SemaTemplateInstantiateDecl.<wbr>cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Sema/<wbr>SemaTemplateInstantiateDecl.<wbr>cpp?rev=310776&r1=310775&r2=<wbr>310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Sema/<wbr>SemaTemplateInstantiateDecl.<wbr>cpp (original)<br>
+++ cfe/trunk/lib/Sema/<wbr>SemaTemplateInstantiateDecl.<wbr>cpp Fri Aug 11 18:46:03 2017<br>
@@ -3771,6 +3771,8 @@ void Sema::<wbr>InstantiateFunctionDefinition<br>
if (PatternDef) {<br>
Pattern = PatternDef->getBody(<wbr>PatternDef);<br>
PatternDecl = PatternDef;<br>
+ if (PatternDef->willHaveBody())<br>
+ PatternDef = nullptr;<br>
}<br>
<br>
// FIXME: We need to track the instantiation stack in order to know which<br>
<br>
Added: cfe/trunk/test/CodeGenCXX/<wbr>pr34163.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/pr34163.cpp?rev=310776&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>CodeGenCXX/pr34163.cpp?rev=<wbr>310776&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/CodeGenCXX/<wbr>pr34163.cpp (added)<br>
+++ cfe/trunk/test/CodeGenCXX/<wbr>pr34163.cpp Fri Aug 11 18:46:03 2017<br>
@@ -0,0 +1,13 @@<br>
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple x86_64-linux-gnu -o - -x c++ %s | FileCheck %s<br>
+<br>
+void f(struct X *) {}<br>
+<br>
+// CHECK: @_ZTV1X =<br>
+struct X {<br>
+ void a() { delete this; }<br>
+ virtual ~X() {}<br>
+ virtual void key_function();<br>
+};<br>
+<br>
+// CHECK: define {{.*}} @_ZN1X12key_functionEv(<br>
+void X::key_function() {}<br>
<br>
Modified: cfe/trunk/test/SemaCUDA/<a href="http://function-overload.cu" rel="noreferrer" target="_blank">functi<wbr>on-overload.cu</a><br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCUDA/function-overload.cu?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>SemaCUDA/function-overload.cu?<wbr>rev=310776&r1=310775&r2=<wbr>310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/SemaCUDA/<a href="http://function-overload.cu" rel="noreferrer" target="_blank">functi<wbr>on-overload.cu</a> (original)<br>
+++ cfe/trunk/test/SemaCUDA/<a href="http://function-overload.cu" rel="noreferrer" target="_blank">functi<wbr>on-overload.cu</a> Fri Aug 11 18:46:03 2017<br>
@@ -222,7 +222,7 @@ GlobalFnPtr fp_g = g;<br>
// Test overloading of destructors<br>
// Can't mix H and unattributed destructors<br>
struct d_h {<br>
- ~d_h() {} // expected-note {{previous declaration is here}}<br>
+ ~d_h() {} // expected-note {{previous definition is here}}<br>
__host__ ~d_h() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
<br>
Modified: cfe/trunk/test/SemaCUDA/<a href="http://no-destructor-overload.cu" rel="noreferrer" target="_blank">no-<wbr>destructor-overload.cu</a><br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCUDA/no-destructor-overload.cu?rev=310776&r1=310775&r2=310776&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/test/<wbr>SemaCUDA/no-destructor-<wbr>overload.cu?rev=310776&r1=<wbr>310775&r2=310776&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/test/SemaCUDA/<a href="http://no-destructor-overload.cu" rel="noreferrer" target="_blank">no-<wbr>destructor-overload.cu</a> (original)<br>
+++ cfe/trunk/test/SemaCUDA/<a href="http://no-destructor-overload.cu" rel="noreferrer" target="_blank">no-<wbr>destructor-overload.cu</a> Fri Aug 11 18:46:03 2017<br>
@@ -7,27 +7,27 @@<br>
// giant change to clang, and the use cases seem quite limited.<br>
<br>
struct A {<br>
- ~A() {} // expected-note {{previous declaration is here}}<br>
+ ~A() {} // expected-note {{previous definition is here}}<br>
__device__ ~A() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
struct B {<br>
- __host__ ~B() {} // expected-note {{previous declaration is here}}<br>
+ __host__ ~B() {} // expected-note {{previous definition is here}}<br>
__host__ __device__ ~B() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
struct C {<br>
- __host__ __device__ ~C() {} // expected-note {{previous declaration is here}}<br>
+ __host__ __device__ ~C() {} // expected-note {{previous definition is here}}<br>
__host__ ~C() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
struct D {<br>
- __device__ ~D() {} // expected-note {{previous declaration is here}}<br>
+ __device__ ~D() {} // expected-note {{previous definition is here}}<br>
__host__ __device__ ~D() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
struct E {<br>
- __host__ __device__ ~E() {} // expected-note {{previous declaration is here}}<br>
+ __host__ __device__ ~E() {} // expected-note {{previous definition is here}}<br>
__device__ ~E() {} // expected-error {{destructor cannot be redeclared}}<br>
};<br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>