[clang] Fix a clang crash on invalid code in C++20 mode.

[hokein]

Summary

This crash is a combination of recovery-expr + new SemaInit.cpp code
introduced by by https://reviews.llvm.org/D129531.

[shafik]

Thank you for the fix, can you explain the failure case in more detail and why checking that RD is defined is the correct fix.

[hokein]

In D140587#4014437, @shafik wrote:

Thank you for the fix, can you explain the failure case in more detail and why checking that RD is defined is the correct fix.

The RD->isAggregate() needs to access the member of data(), for the code in the testcase, data() is null, we ends up with accessing a nullptr. see the stack trace:

lang: llvm-project/clang/include/clang/AST/DeclCXX.h:448: struct DefinitionData &clang::CXXRecordDecl::data() const: Assertion `DD && "queried property of class with no definition"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
 #0 0x000055bef3cd7ff3 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:567:13
 #1 0x000055bef3cd6260 llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:105:18
 #2 0x000055bef3cd837a SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:412:1
 #3 0x00007f024183daa0 (/lib/x86_64-linux-gnu/libc.so.6+0x3daa0)
 #4 0x00007f024188957c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007f024183da02 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x00007f0241828469 abort ./stdlib/abort.c:81:7
 #7 0x00007f0241828395 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #8 0x00007f0241836ab2 (/lib/x86_64-linux-gnu/libc.so.6+0x36ab2)
 #9 0x000055bef3fd2b78 (./bin/clang+0x38eab78)
#10 0x000055bef5e78f07 clang::CXXRecordDecl::isAggregate() const llvm-project/clang/include/clang/AST/DeclCXX.h:1119:37
#11 0x000055bef5e78f07 clang::InitializationSequence::InitializeFrom(clang::Sema&, clang::InitializedEntity const&, clang::InitializationKind const&, llvm::MutableArrayRef<clang::Expr*>, bool, bool) llvm-project/clang/lib/Sema/SemaInit.cpp:6180:15
#12 0x000055bef5e89a75 clang::InitializedEntity::getKind() const llvm-project/clang/include/clang/Sema/Initialization.h:427:39
#13 0x000055bef5e89a75 clang::InitializedEntity::isParameterKind() const llvm-project/clang/include/clang/Sema/Initialization.h:461:13
#14 0x000055bef5e89a75 clang::Sema::PerformCopyInitialization(clang::InitializedEntity const&, clang::SourceLocation, clang::ActionResult<clang::Expr*, true>, bool, bool) llvm-project/clang/lib/Sema/SemaInit.cpp:10253:14
#15 0x000055bef5ffecd3 clang::Sema::PerformMoveOrCopyInitialization(clang::InitializedEntity const&, clang::Sema::NamedReturnInfo const&, clang::Expr*, bool) /usr/

[ilya-biryukov]

clang/lib/Sema/SemaInit.cpp

6177

@ayzhao it's a bit surprising that this code path runs for braced initializations even though the patch was written for parenthesized inits. Is this intended?

6179

NIT: could you add a comment that we don't need to call RequireCompleteType as TryConstructorInitialization already does this for us?

RequireCompleteType should be the default choice in general as we might need to pick the right template specialization and do the corresponding template instantiation. However, in this case it has already been done.

clang/test/SemaCXX/recovery-expr-type.cpp

181

Could you add a test for A<int, int>(1) too?
The code path somehow runs for {} too, but I think it's unintended and the relevant code path might only run for parenthesized init.

[ilya-biryukov]

LGTM to unbreak clangd, this seems to pop up a lot for Chrome developers.

[hokein]

address comments

clang/lib/Sema/SemaInit.cpp

if (Kind.getKind() == InitializationKind::IK_Direct ||
          (Context.hasSameUnqualifiedType(SourceType, DestType) ||
           S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType)))) {
       TryConstructorInitialization(S, Entity, Kind, Args, DestType, DestType,
                                    *this);
 
       // We fall back to the "no matching constructor" path if the
       // failed candidate set has functions other than the three default
       // constructors. For example, conversion function.
       if (const auto *RD =
               dyn_cast<CXXRecordDecl>(DestType->getAs<RecordType>()->getDecl());
-          S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+          S.getLangOpts().CPlusPlus20 && RD && RD->hasDefinition() &&
+          RD->isAggregate() && Failed() &&
           getFailureKind() == FK_ConstructorOverloadFailed) {
         // C++20 [dcl.init] 17.6.2.2:
         //   - Otherwise, if no constructor is viable, the destination type is
         //   an
         //      aggregate class, and the initializer is a parenthesized
         //      expression-list.
         TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
                                           /*VerifyOnly=*/true);

clang/test/SemaCXX/recovery-expr-type.cpp

-// RUN: %clang_cc1 -triple=x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -o - %s -std=gnu++17 -fsyntax-only -verify
+// RUN: %clang_cc1 -triple=x86_64-unknown-unknown -o - %s -std=gnu++17 -fsyntax-only -verify
+// RUN: %clang_cc1 -triple=x86_64-unknown-unknown -o - %s -std=gnu++20 -fsyntax-only -verify
+
 
 namespace test0 {
 struct Indestructible {
   // Indestructible();
   ~Indestructible() = delete; // expected-note 2{{deleted}}
 };
 Indestructible make_indestructible();
 

 namespace test15 {
 void f() {
   struct {
     void m(int (&)[undefined()]) {} // expected-error {{undeclared identifier}}
   } S;
   S.m(1); // no crash
 }
 }
+
+namespace test16 {
+// verify we do not crash on incomplete class type.
+template<typename T, typename U> struct A; // expected-note 3{{template is declared here}}
+A<int, int> foo() { // expected-error {{implicit instantiation of undefined template}}
+  return A<int, int>{1}; // expected-error 2{{implicit instantiation of undefined template}}
+}
+}