Re: r310776 - PR34163: Don't cache an incorrect key function for a class if queried between

[Hans Wennborg via cfe-commits]

Merged in r311105.

On Mon, Aug 14, 2017 at 10:07 AM, Hans Wennborg  wrote:
> Sounds good to me. Let's give it another day and see how it goes.
>
> On Fri, Aug 11, 2017 at 6:48 PM, Richard Smith  wrote:
>> Hi Hans,
>>
>> 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?
>>
>> On 11 August 2017 at 18:46, Richard Smith via cfe-commits
>>  wrote:
>>>
>>> Author: rsmith
>>> Date: Fri Aug 11 18:46:03 2017
>>> New Revision: 310776
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=310776=rev
>>> Log:
>>> PR34163: Don't cache an incorrect key function for a class if queried
>>> between
>>> the class becoming complete and its inline methods being parsed.
>>>
>>> This replaces the hack of using the "late parsed template" flag to track
>>> member
>>> functions with bodies we've not parsed yet; instead we now use the "will
>>> have
>>> body" flag, which carries the desired implication that the function
>>> declaration
>>> *is* a definition, and that we've just not parsed its body yet.
>>>
>>> Added:
>>> cfe/trunk/test/CodeGenCXX/pr34163.cpp
>>> Modified:
>>> cfe/trunk/include/clang/AST/Decl.h
>>> cfe/trunk/lib/AST/DeclCXX.cpp
>>> cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
>>> cfe/trunk/lib/Sema/SemaDecl.cpp
>>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>>> cfe/trunk/test/SemaCUDA/function-overload.cu
>>> cfe/trunk/test/SemaCUDA/no-destructor-overload.cu
>>>
>>> Modified: cfe/trunk/include/clang/AST/Decl.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=310776=310775=310776=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/AST/Decl.h (original)
>>> +++ cfe/trunk/include/clang/AST/Decl.h Fri Aug 11 18:46:03 2017
>>> @@ -1666,8 +1666,7 @@ private:
>>>unsigned HasSkippedBody : 1;
>>>
>>>/// Indicates if the function declaration will have a body, once we're
>>> done
>>> -  /// parsing it.  (We don't set it to false when we're done parsing, in
>>> the
>>> -  /// hopes this is simpler.)
>>> +  /// parsing it.
>>>unsigned WillHaveBody : 1;
>>>
>>>/// \brief End part of this FunctionDecl's source range.
>>>
>>> Modified: cfe/trunk/lib/AST/DeclCXX.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=310776=310775=310776=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/AST/DeclCXX.cpp (original)
>>> +++ cfe/trunk/lib/AST/DeclCXX.cpp Fri Aug 11 18:46:03 2017
>>> @@ -1837,9 +1837,10 @@ bool CXXMethodDecl::hasInlineBody() cons
>>>const FunctionDecl *CheckFn = getTemplateInstantiationPattern();
>>>if (!CheckFn)
>>>  CheckFn = this;
>>> -
>>> +
>>>const FunctionDecl *fn;
>>> -  return CheckFn->hasBody(fn) && !fn->isOutOfLine();
>>> +  return CheckFn->isDefined(fn) && !fn->isOutOfLine() &&
>>> + (fn->doesThisDeclarationHaveABody() || fn->willHaveBody());
>>>  }
>>>
>>>  bool CXXMethodDecl::isLambdaStaticInvoker() const {
>>>
>>> Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=310776=310775=310776=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Fri Aug 11 18:46:03 2017
>>> @@ -166,20 +166,11 @@ NamedDecl *Parser::ParseCXXInlineMethodD
>>>}
>>>
>>>if (FnD) {
>>> -// If this is a friend function, mark that it's late-parsed so that
>>> -// it's still known to be a definition even before we attach the
>>> -// parsed body.  Sema needs to treat friend function definitions
>>> -// differently during template instantiation, and it's possible for
>>> -// the containing class to be instantiated before all its member
>>> -// function definitions are parsed.
>>> -//
>>> -// If you remove this, you can remove the code that clears the flag
>>> -// after parsing the member.
>>> -if (D.getDeclSpec().isFriendSpecified()) {
>>> -  FunctionDecl *FD = FnD->getAsFunction();
>>> -  Actions.CheckForFunctionRedefinition(FD);
>>> -  FD->setLateTemplateParsed(true);
>>> -}
>>> +FunctionDecl *FD = FnD->getAsFunction();
>>> +// Track that this function will eventually have a body; Sema needs
>>> +// to know this.
>>> +Actions.CheckForFunctionRedefinition(FD);
>>> +FD->setWillHaveBody(true);
>>>} else {
>>>  // If semantic analysis could not build a function declaration,
>>>  // just throw away the late-parsed declaration.
>>> @@ -559,10 +550,6 @@ void Parser::ParseLexedMethodDef(LexedMe
>>>
>>>ParseFunctionStatementBody(LM.D, 

Re: r310776 - PR34163: Don't cache an incorrect key function for a class if queried between

[Hans Wennborg via cfe-commits]

Sounds good to me. Let's give it another day and see how it goes.

On Fri, Aug 11, 2017 at 6:48 PM, Richard Smith  wrote:
> Hi Hans,
>
> 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?
>
> On 11 August 2017 at 18:46, Richard Smith via cfe-commits
>  wrote:
>>
>> Author: rsmith
>> Date: Fri Aug 11 18:46:03 2017
>> New Revision: 310776
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310776=rev
>> Log:
>> PR34163: Don't cache an incorrect key function for a class if queried
>> between
>> the class becoming complete and its inline methods being parsed.
>>
>> This replaces the hack of using the "late parsed template" flag to track
>> member
>> functions with bodies we've not parsed yet; instead we now use the "will
>> have
>> body" flag, which carries the desired implication that the function
>> declaration
>> *is* a definition, and that we've just not parsed its body yet.
>>
>> Added:
>> cfe/trunk/test/CodeGenCXX/pr34163.cpp
>> Modified:
>> cfe/trunk/include/clang/AST/Decl.h
>> cfe/trunk/lib/AST/DeclCXX.cpp
>> cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
>> cfe/trunk/lib/Sema/SemaDecl.cpp
>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>> cfe/trunk/test/SemaCUDA/function-overload.cu
>> cfe/trunk/test/SemaCUDA/no-destructor-overload.cu
>>
>> Modified: cfe/trunk/include/clang/AST/Decl.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=310776=310775=310776=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/Decl.h (original)
>> +++ cfe/trunk/include/clang/AST/Decl.h Fri Aug 11 18:46:03 2017
>> @@ -1666,8 +1666,7 @@ private:
>>unsigned HasSkippedBody : 1;
>>
>>/// Indicates if the function declaration will have a body, once we're
>> done
>> -  /// parsing it.  (We don't set it to false when we're done parsing, in
>> the
>> -  /// hopes this is simpler.)
>> +  /// parsing it.
>>unsigned WillHaveBody : 1;
>>
>>/// \brief End part of this FunctionDecl's source range.
>>
>> Modified: cfe/trunk/lib/AST/DeclCXX.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=310776=310775=310776=diff
>>
>> ==
>> --- cfe/trunk/lib/AST/DeclCXX.cpp (original)
>> +++ cfe/trunk/lib/AST/DeclCXX.cpp Fri Aug 11 18:46:03 2017
>> @@ -1837,9 +1837,10 @@ bool CXXMethodDecl::hasInlineBody() cons
>>const FunctionDecl *CheckFn = getTemplateInstantiationPattern();
>>if (!CheckFn)
>>  CheckFn = this;
>> -
>> +
>>const FunctionDecl *fn;
>> -  return CheckFn->hasBody(fn) && !fn->isOutOfLine();
>> +  return CheckFn->isDefined(fn) && !fn->isOutOfLine() &&
>> + (fn->doesThisDeclarationHaveABody() || fn->willHaveBody());
>>  }
>>
>>  bool CXXMethodDecl::isLambdaStaticInvoker() const {
>>
>> Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=310776=310775=310776=diff
>>
>> ==
>> --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Fri Aug 11 18:46:03 2017
>> @@ -166,20 +166,11 @@ NamedDecl *Parser::ParseCXXInlineMethodD
>>}
>>
>>if (FnD) {
>> -// If this is a friend function, mark that it's late-parsed so that
>> -// it's still known to be a definition even before we attach the
>> -// parsed body.  Sema needs to treat friend function definitions
>> -// differently during template instantiation, and it's possible for
>> -// the containing class to be instantiated before all its member
>> -// function definitions are parsed.
>> -//
>> -// If you remove this, you can remove the code that clears the flag
>> -// after parsing the member.
>> -if (D.getDeclSpec().isFriendSpecified()) {
>> -  FunctionDecl *FD = FnD->getAsFunction();
>> -  Actions.CheckForFunctionRedefinition(FD);
>> -  FD->setLateTemplateParsed(true);
>> -}
>> +FunctionDecl *FD = FnD->getAsFunction();
>> +// Track that this function will eventually have a body; Sema needs
>> +// to know this.
>> +Actions.CheckForFunctionRedefinition(FD);
>> +FD->setWillHaveBody(true);
>>} else {
>>  // If semantic analysis could not build a function declaration,
>>  // just throw away the late-parsed declaration.
>> @@ -559,10 +550,6 @@ void Parser::ParseLexedMethodDef(LexedMe
>>
>>ParseFunctionStatementBody(LM.D, FnScope);
>>
>> -  // Clear the late-template-parsed bit if we set it before.
>> -  if (LM.D)
>> -LM.D->getAsFunction()->setLateTemplateParsed(false);
>> -
>>while (Tok.isNot(tok::eof))
>>  

Re: r310776 - PR34163: Don't cache an incorrect key function for a class if queried between

[Richard Smith via cfe-commits]

Hi Hans,

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?

On 11 August 2017 at 18:46, Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rsmith
> Date: Fri Aug 11 18:46:03 2017
> New Revision: 310776
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310776=rev
> Log:
> PR34163: Don't cache an incorrect key function for a class if queried
> between
> the class becoming complete and its inline methods being parsed.
>
> This replaces the hack of using the "late parsed template" flag to track
> member
> functions with bodies we've not parsed yet; instead we now use the "will
> have
> body" flag, which carries the desired implication that the function
> declaration
> *is* a definition, and that we've just not parsed its body yet.
>
> Added:
> cfe/trunk/test/CodeGenCXX/pr34163.cpp
> Modified:
> cfe/trunk/include/clang/AST/Decl.h
> cfe/trunk/lib/AST/DeclCXX.cpp
> cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> cfe/trunk/test/SemaCUDA/function-overload.cu
> cfe/trunk/test/SemaCUDA/no-destructor-overload.cu
>
> Modified: cfe/trunk/include/clang/AST/Decl.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
> clang/AST/Decl.h?rev=310776=310775=310776=diff
> 
> ==
> --- cfe/trunk/include/clang/AST/Decl.h (original)
> +++ cfe/trunk/include/clang/AST/Decl.h Fri Aug 11 18:46:03 2017
> @@ -1666,8 +1666,7 @@ private:
>unsigned HasSkippedBody : 1;
>
>/// Indicates if the function declaration will have a body, once we're
> done
> -  /// parsing it.  (We don't set it to false when we're done parsing, in
> the
> -  /// hopes this is simpler.)
> +  /// parsing it.
>unsigned WillHaveBody : 1;
>
>/// \brief End part of this FunctionDecl's source range.
>
> Modified: cfe/trunk/lib/AST/DeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/
> DeclCXX.cpp?rev=310776=310775=310776=diff
> 
> ==
> --- cfe/trunk/lib/AST/DeclCXX.cpp (original)
> +++ cfe/trunk/lib/AST/DeclCXX.cpp Fri Aug 11 18:46:03 2017
> @@ -1837,9 +1837,10 @@ bool CXXMethodDecl::hasInlineBody() cons
>const FunctionDecl *CheckFn = getTemplateInstantiationPattern();
>if (!CheckFn)
>  CheckFn = this;
> -
> +
>const FunctionDecl *fn;
> -  return CheckFn->hasBody(fn) && !fn->isOutOfLine();
> +  return CheckFn->isDefined(fn) && !fn->isOutOfLine() &&
> + (fn->doesThisDeclarationHaveABody() || fn->willHaveBody());
>  }
>
>  bool CXXMethodDecl::isLambdaStaticInvoker() const {
>
> Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/
> ParseCXXInlineMethods.cpp?rev=310776=310775=310776=diff
> 
> ==
> --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Fri Aug 11 18:46:03 2017
> @@ -166,20 +166,11 @@ NamedDecl *Parser::ParseCXXInlineMethodD
>}
>
>if (FnD) {
> -// If this is a friend function, mark that it's late-parsed so that
> -// it's still known to be a definition even before we attach the
> -// parsed body.  Sema needs to treat friend function definitions
> -// differently during template instantiation, and it's possible for
> -// the containing class to be instantiated before all its member
> -// function definitions are parsed.
> -//
> -// If you remove this, you can remove the code that clears the flag
> -// after parsing the member.
> -if (D.getDeclSpec().isFriendSpecified()) {
> -  FunctionDecl *FD = FnD->getAsFunction();
> -  Actions.CheckForFunctionRedefinition(FD);
> -  FD->setLateTemplateParsed(true);
> -}
> +FunctionDecl *FD = FnD->getAsFunction();
> +// Track that this function will eventually have a body; Sema needs
> +// to know this.
> +Actions.CheckForFunctionRedefinition(FD);
> +FD->setWillHaveBody(true);
>} else {
>  // If semantic analysis could not build a function declaration,
>  // just throw away the late-parsed declaration.
> @@ -559,10 +550,6 @@ void Parser::ParseLexedMethodDef(LexedMe
>
>ParseFunctionStatementBody(LM.D, FnScope);
>
> -  // Clear the late-template-parsed bit if we set it before.
> -  if (LM.D)
> -LM.D->getAsFunction()->setLateTemplateParsed(false);
> -
>while (Tok.isNot(tok::eof))
>  ConsumeAnyToken();
>
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
> SemaDecl.cpp?rev=310776=310775=310776=diff
> 
> ==
> --- 

r310776 - PR34163: Don't cache an incorrect key function for a class if queried between

[Richard Smith via cfe-commits]

Author: rsmith
Date: Fri Aug 11 18:46:03 2017
New Revision: 310776

URL: http://llvm.org/viewvc/llvm-project?rev=310776=rev
Log:
PR34163: Don't cache an incorrect key function for a class if queried between
the class becoming complete and its inline methods being parsed.

This replaces the hack of using the "late parsed template" flag to track member
functions with bodies we've not parsed yet; instead we now use the "will have
body" flag, which carries the desired implication that the function declaration
*is* a definition, and that we've just not parsed its body yet.

Added:
cfe/trunk/test/CodeGenCXX/pr34163.cpp
Modified:
cfe/trunk/include/clang/AST/Decl.h
cfe/trunk/lib/AST/DeclCXX.cpp
cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/test/SemaCUDA/function-overload.cu
cfe/trunk/test/SemaCUDA/no-destructor-overload.cu

Modified: cfe/trunk/include/clang/AST/Decl.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=310776=310775=310776=diff
==
--- cfe/trunk/include/clang/AST/Decl.h (original)
+++ cfe/trunk/include/clang/AST/Decl.h Fri Aug 11 18:46:03 2017
@@ -1666,8 +1666,7 @@ private:
   unsigned HasSkippedBody : 1;
 
   /// Indicates if the function declaration will have a body, once we're done
-  /// parsing it.  (We don't set it to false when we're done parsing, in the
-  /// hopes this is simpler.)
+  /// parsing it.
   unsigned WillHaveBody : 1;
 
   /// \brief End part of this FunctionDecl's source range.

Modified: cfe/trunk/lib/AST/DeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=310776=310775=310776=diff
==
--- cfe/trunk/lib/AST/DeclCXX.cpp (original)
+++ cfe/trunk/lib/AST/DeclCXX.cpp Fri Aug 11 18:46:03 2017
@@ -1837,9 +1837,10 @@ bool CXXMethodDecl::hasInlineBody() cons
   const FunctionDecl *CheckFn = getTemplateInstantiationPattern();
   if (!CheckFn)
 CheckFn = this;
-  
+
   const FunctionDecl *fn;
-  return CheckFn->hasBody(fn) && !fn->isOutOfLine();
+  return CheckFn->isDefined(fn) && !fn->isOutOfLine() &&
+ (fn->doesThisDeclarationHaveABody() || fn->willHaveBody());
 }
 
 bool CXXMethodDecl::isLambdaStaticInvoker() const {

Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=310776=310775=310776=diff
==
--- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original)
+++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Fri Aug 11 18:46:03 2017
@@ -166,20 +166,11 @@ NamedDecl *Parser::ParseCXXInlineMethodD
   }
 
   if (FnD) {
-// If this is a friend function, mark that it's late-parsed so that
-// it's still known to be a definition even before we attach the
-// parsed body.  Sema needs to treat friend function definitions
-// differently during template instantiation, and it's possible for
-// the containing class to be instantiated before all its member
-// function definitions are parsed.
-//
-// If you remove this, you can remove the code that clears the flag
-// after parsing the member.
-if (D.getDeclSpec().isFriendSpecified()) {
-  FunctionDecl *FD = FnD->getAsFunction();
-  Actions.CheckForFunctionRedefinition(FD);
-  FD->setLateTemplateParsed(true);
-}
+FunctionDecl *FD = FnD->getAsFunction();
+// Track that this function will eventually have a body; Sema needs
+// to know this.
+Actions.CheckForFunctionRedefinition(FD);
+FD->setWillHaveBody(true);
   } else {
 // If semantic analysis could not build a function declaration,
 // just throw away the late-parsed declaration.
@@ -559,10 +550,6 @@ void Parser::ParseLexedMethodDef(LexedMe
 
   ParseFunctionStatementBody(LM.D, FnScope);
 
-  // Clear the late-template-parsed bit if we set it before.
-  if (LM.D)
-LM.D->getAsFunction()->setLateTemplateParsed(false);
-
   while (Tok.isNot(tok::eof))
 ConsumeAnyToken();
 

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=310776=310775=310776=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Aug 11 18:46:03 2017
@@ -12090,8 +12090,9 @@ Decl *Sema::ActOnStartOfFunctionDef(Scop
 FD->setInvalidDecl();
   }
 
-  // See if this is a redefinition.
-  if (!FD->isLateTemplateParsed()) {
+  // See if this is a redefinition. If 'will have body' is already set, then
+  // these checks were already performed when it was set.
+  if (!FD->willHaveBody() && !FD->isLateTemplateParsed()) {