Page MenuHomePhabricator

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

Authored by zahiraam on Sep 22 2020, 12:54 PM.

Diff Detail

Event Timeline

zahiraam requested review of this revision.Sep 22 2020, 12:54 PM
zahiraam created this revision.
zahiraam added reviewers: mikerice, rnk.
rnk added inline comments.Sep 22 2020, 1:16 PM
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 added inline comments.Sep 22 2020, 2:33 PM
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?

zahiraam updated this revision to Diff 293713.Sep 23 2020, 6:11 AM
zahiraam marked an inline comment as done.
rnk added a comment.Sep 28 2020, 10:22 AM

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.

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:

  1. 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 updated this revision to Diff 296466.Oct 6 2020, 8:05 AM

Any feedback please?

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);
};
zahiraam updated this revision to Diff 299753.Oct 21 2020, 11:02 AM
aaron.ballman accepted this revision.Oct 21 2020, 11:32 AM

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).

This revision is now accepted and ready to land.Oct 21 2020, 11:32 AM
zahiraam updated this revision to Diff 299801.Oct 21 2020, 2:01 PM
zahiraam marked 2 inline comments as done.