Clang is crashing after generating the right diagnostic for a re-declaration of a friend method - Fix for PR47544

[rnk]

clang/lib/Sema/SemaDeclCXX.cpp

16626 ↗

(On Diff #293553)

This condition will become dead after your change, I think. Is there a way to carry on making the friend declaration even if the named declaration is invalid?

[zahiraam]

clang/lib/Sema/SemaDeclCXX.cpp

16626 ↗

(On Diff #293553)

Yes. The FrD can be generated. Would it be better to return null at that pointer after set the FrD to Invalid too?

[rnk]

I don't feel like there is enough information in the bug or here to understand if this is the correct fix. Why is clang crashing? Why should we return null here instead of returning a decl that has already been marked invalid? Maybe the caller should treat the invalid declaration more carefully. I usually try to answer these questions by reading the existing code to try to understand why it does what it does.

I see there are two call sites to this function, one for definitions ({}) and one for declarations {;}:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseCXXInlineMethods.cpp#L39
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2825

In this case, it seems like the second is the one that is crashing. Reading forward from that function call, I found these ifs here, which treat null and invalid member declarations differently:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2890
That means there is some set of things we do for invalid decls, and some that we do for null decls. I guess the question is now, should those things be done for these types of friend declarations? I guess the important thing is that the invalid declaration is added to the declgroup, which is ultimately what gets returned out of here, so maybe the crash happens later, but I can't say for sure.

[zahiraam]

In D88112#2298597, @rnk wrote:

I don't feel like there is enough information in the bug or here to understand if this is the correct fix. Why is clang crashing? Why should we return null here instead of returning a decl that has already been marked invalid? Maybe the caller should treat the invalid declaration more carefully. I usually try to answer these questions by reading the existing code to try to understand why it does what it does.

I see there are two call sites to this function, one for definitions ({}) and one for declarations {;}:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseCXXInlineMethods.cpp#L39
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2825

In this case, it seems like the second is the one that is crashing. Reading forward from that function call, I found these ifs here, which treat null and invalid member declarations differently:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseDeclCXX.cpp#L2890
That means there is some set of things we do for invalid decls, and some that we do for null decls. I guess the question is now, should those things be done for these types of friend declarations? I guess the important thing is that the invalid declaration is added to the declgroup, which is ultimately what gets returned out of here, so maybe the crash happens later, but I can't say for sure.

The crash is happening when entering ParseLexedMethodDeclarationat line 408, when trying to parse late decls that were added to LateParsedDeclarations stack. The cast to FunctionDecl is crashing because LM.Method is in this case a FunctionTemplateDecl. It may also crash if LM.Method is a FunctionDecl that doesn't have a getPreviousDecl, which may happen.

HandleMemberFunctionDeclDelays pushes Decls in LateParsedDeclarations stack when needed. This function is called by ParseCXXInlineMethodDef when dealing with a definition and by ParseCXXClassMemberDeclaration when it is a declaration.
In both cases the Decl is pushed into LateParsedDeclarations when Decl is not null, regardless of validity of the Decl.

We can either:

1. Avoid pushing invalid decls into this stack? This way the invalid Decl will be pushed into DeclsInGroup but not handled?

diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 290b3c5..6f6a03c 100644

• a/clang/lib/Parse/ParseDeclCXX.cpp

+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2184,7 +2184,8 @@ void Parser::HandleMemberFunctionDeclDelays(Declarator& DeclaratorInfo,

// Push this method onto the stack of late-parsed method
// declarations.
auto LateMethod = new LateParsedMethodDeclaration(this, ThisDecl);

• getCurrentClass().LateParsedDeclarations.push_back(LateMethod);

+ if (!ThisDecl->isInvalidDecl())
+ getCurrentClass().LateParsedDeclarations.push_back(LateMethod);

// Stash the exception-specification tokens in the late-pased method.

Or:

2. Push the invalid Decl to the stack and handle the params only for valid Decl?
   
   diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp

index d05332b..67aadbf 100644

• a/clang/lib/Parse/ParseCXXInlineMethods.cpp

+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -403,9 +403,12 @@ void Parser::ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM) {

if (Tok.is(tok::eof) && Tok.getEofData() == Param)
  ConsumeAnyToken();

• } else if (HasUnparsed) {

+ } else if (HasUnparsed && !LM.Method->isInvalidDecl()) {

assert(Param->hasInheritedDefaultArg());
FunctionDecl *Old = cast<FunctionDecl>(LM.Method)->getPreviousDecl();

Your thoughts?

Thanks.

[zahiraam]

Any feedback please?

[aaron.ballman]

Adding a few more reviewers in case I've got something wrong.

clang/lib/Parse/ParseCXXInlineMethods.cpp

407–408

It seems to me that we have two bugs to handle here:

1. we should probably handle a FunctionTemplateDecl here, since it's plausible to get one. You can also see we handle them on line 446 of this file as well.
2. we should handle the case where getPreviousDecl() returns nullptr.

I think if you handle these two things, you won't need the check for an invalid decl above (I'm pretty sure all of the code here should be fine on an invalid decl).

clang/test/CodeGenCXX/invalid-decl.cpp

1 ↗

(On Diff #296466)

There's no need for the test to be a codegen test that emits llvm. The crash still happens with -fsyntax-only, so I think this should be a SemaCXX test instead.

9 ↗

(On Diff #296466)

An even simpler example is:

class test {
  friend int bar(bool = true);
  friend int bar(bool);
};

[aaron.ballman]

LGTM aside from some small nits. You should wait a day or two before landing in case @rnk or other reviewers have further comments.

clang/lib/Parse/ParseCXXInlineMethods.cpp

409

I think you should use const FunctionDecl *Old; above and use const auto * here (because the type is spelled out in the initialization, you don't need to repeat the type).

clang/test/SemaCXX/invalid-decl.cpp

2

I don't think you need -std=c++11 here either, as there's nothing C++11 specific about the test (it still crashes for me in C++03 on godbolt, for instance).

[zahiraam]

Merged in https://reviews.llvm.org/rGe562a40871da.

[shafik]

Closing since this was merged

clang/lib/Parse/ParseCXXInlineMethods.cpp

if (Toks) {
       // There could be leftover tokens (e.g. because of an error).
       // Skip through until we reach the 'end of default argument' token.
       while (Tok.isNot(tok::eof))
         ConsumeAnyToken();
 
       if (Tok.is(tok::eof) && Tok.getEofData() == Param)
         ConsumeAnyToken();
     } else if (HasUnparsed) {
       assert(Param->hasInheritedDefaultArg());
-      FunctionDecl *Old = cast<FunctionDecl>(LM.Method)->getPreviousDecl();
-      ParmVarDecl *OldParam = Old->getParamDecl(I);
-      assert (!OldParam->hasUnparsedDefaultArg());
+      const FunctionDecl *Old;
+      if (const auto *FunTmpl =
+              dyn_cast<FunctionTemplateDecl>(LM.Method))
+        Old =
+            cast<FunctionDecl>(FunTmpl->getTemplatedDecl())->getPreviousDecl();
+      else
+        Old = cast<FunctionDecl>(LM.Method)->getPreviousDecl();
+      if (Old) {
+        ParmVarDecl *OldParam = const_cast<ParmVarDecl*>(Old->getParamDecl(I));
+        assert(!OldParam->hasUnparsedDefaultArg());
         if (OldParam->hasUninstantiatedDefaultArg())
           Param->setUninstantiatedDefaultArg(
               OldParam->getUninstantiatedDefaultArg());
         else
           Param->setDefaultArg(OldParam->getInit());
       }
     }
+  }
 
   // Parse a delayed exception-specification, if there is one.
   if (CachedTokens *Toks = LM.ExceptionSpecTokens) {
     ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
     // Add the 'stop' token.
     Token LastExceptionSpecToken = Toks->back();
     Token ExceptionSpecEnd;

clang/test/SemaCXX/invalid-decl.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class test1 {
+  template<typename> friend int bar(bool = true) {} // expected-note {{previous declaration is here}}
+  template<typename> friend int bar(bool); // expected-error {{friend declaration specifying a default argument must be the only declaration}}
+};
+
+class test2 {
+  friend int bar(bool = true) {} // expected-note {{previous declaration is here}}
+  friend int bar(bool); // expected-error{{friend declaration specifying a default argument must be the only declaration}}
+};