[clang][ASTImporter] Merge implicit ctors with definition

[danix800]

Summary

Implicit ctors generated with definition should be merged into To context.

This is triggered whenever the staled ctor's initializer in To context is accessed (dumping node or being added into CallGraph):

#0 0x00007f2b311fcd1a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:602:11
 #1 0x00007f2b311fcecb PrintStackTraceSignalHandler(void*) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:675:1
 #2 0x00007f2b311fb436 llvm::sys::RunSignalHandlers() /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00007f2b311fd6e5 SignalHandler(int) /home/xxxxx/Sources/llvm-project-main/llvm/lib/Support/Unix/Signals.inc:413:1
 #4 0x00007f2b30c5af90 (/lib/x86_64-linux-gnu/libc.so.6+0x3bf90)
 #5 0x00007f2b32b5fbf5 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::VisitFunctionDecl(clang::FunctionDecl const*) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:437:26
 #6 0x00007f2b32b5fced clang::declvisitor::Base<llvm::make_const_ptr, clang::ASTDumper, void>::VisitCXXMethodDecl(clang::CXXMethodDecl const*) /home/xxxxx/Sources/llvm-project-main/build/tools/clang/include/clang/AST/DeclNodes.inc:443:1
 #7 0x00007f2b32b5fd1d clang::declvisitor::Base<llvm::make_const_ptr, clang::ASTDumper, void>::VisitCXXConstructorDecl(clang::CXXConstructorDecl const*) /home/xxxxx/Sources/llvm-project-main/build/tools/clang/include/clang/AST/DeclNodes.inc:447:1
 #8 0x00007f2b32b5e493 clang::declvisitor::Base<llvm::make_const_ptr, clang::ASTDumper, void>::Visit(clang::Decl const*) /home/xxxxx/Sources/llvm-project-main/build/tools/clang/include/clang/AST/DeclNodes.inc:447:1
 #9 0x00007f2b32b5ddd5 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()::operator()() const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:97:34
#10 0x00007f2b32b752c9 void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)::operator()(bool) const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/TextNodeDumper.h:0:7
#11 0x00007f2b32b75156 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'() std::__invoke_impl<void, void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)&, bool>(std::__invoke_other, void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61:7
#12 0x00007f2b32b750f2 std::enable_if<is_invocable_r_v<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'(), void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)&, bool>, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>::type std::__invoke_r<void, void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)&, bool>(void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:117:5
#13 0x00007f2b32b74f82 std::_Function_handler<void (bool), void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'())::'lambda'(bool)>::_M_invoke(std::_Any_data const&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:290:2
#14 0x00007f2b32b5cca9 std::function<void (bool)>::operator()(bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:591:2
#15 0x00007f2b32b5dd2a void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/TextNodeDumper.h:118:24
#16 0x00007f2b32b5db75 void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/TextNodeDumper.h:52:5
#17 0x00007f2b32b5b956 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:120:3
#18 0x00007f2b32b5e925 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::dumpDeclContext(clang::DeclContext const*) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:297:24
#19 0x00007f2b32b5df17 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()::operator()() const /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:118:7
#20 0x00007f2b32b5dbfe void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(llvm::StringRef, clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/TextNodeDumper.h:0:7
#21 0x00007f2b32b5db75 void clang::TextTreeStructure::AddChild<clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()>(clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*)::'lambda'()) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/TextNodeDumper.h:52:5
#22 0x00007f2b32b5b956 clang::ASTNodeTraverser<clang::ASTDumper, clang::TextNodeDumper>::Visit(clang::Decl const*) /home/xxxxx/Sources/llvm-project-main/clang/include/clang/AST/ASTNodeTraverser.h:120:3
#23 0x00007f2b32b56fa3 clang::Decl::dump(llvm::raw_ostream&, bool, clang::ASTDumpOutputFormat) const /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ASTDumper.cpp:222:3
#24 0x00007f2b32b56e59 clang::Decl::dump() const /home/xxxxx/Sources/llvm-project-main/clang/lib/AST/ASTDumper.cpp:206:64
#25 0x000055abce41c3eb clang::ast_matchers::ImportImplicitMethods_MergeImplicitMethodWithDefinition_Test::TestBody() /home/xxxxx/Sources/llvm-project-main/clang/unittests/AST/ASTImporterTest.cpp:3201:23
#26 0x00007f2b3436d76b void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2433:3
#27 0x00007f2b34352e07 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2488:5
#28 0x00007f2b3433b963 testing::Test::Run() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2515:3
#29 0x00007f2b3433c1ba testing::TestInfo::Run() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2687:12
#30 0x00007f2b3433c71b testing::TestSuite::Run() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2815:44
#31 0x00007f2b34344ef9 testing::internal::UnitTestImpl::RunAllTests() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:5337:24
#32 0x00007f2b34370a3b bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2433:3
#33 0x00007f2b34355247 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:2488:5
#34 0x00007f2b34344adf testing::UnitTest::Run() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/src/gtest.cc:4925:10
#35 0x00007f2b34d03d71 RUN_ALL_TESTS() /home/xxxxx/Sources/llvm-project-main/third-party/unittest/googletest/include/gtest/gtest.h:2472:3
#36 0x00007f2b34d03cb4 main /home/xxxxx/Sources/llvm-project-main/third-party/unittest/UnitTestMain/TestMain.cpp:55:3
#37 0x00007f2b30c4618a __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#38 0x00007f2b30c46245 call_init ./csu/../csu/libc-start.c:128:20
#39 0x00007f2b30c46245 __libc_start_main ./csu/../csu/libc-start.c:368:5
#40 0x000055abce386221 _start (./build/tools/clang/unittests/AST/ASTTests+0x48b221)
Segmentation fault

https://github.com/llvm/llvm-project/issues/64171

[balazske]

It looks not good to remove an invalid node from the DeclContext that otherwise remains in the AST. I checked the problem and found that the existing move constructor (originally in the To AST which had no definition) gets a getNumCtorInitializers value of 1 but the init_begin returns 0. This causes crash even when dumping it. I did not find the cause of this situation (the first time at line 3822 it is already changed, and ASTImporter has this single position to change the value). Normally what should happen is that a new move constructor is imported (with a definition) and linked after the existing one (and the existing is not modified). We get an AST that does not occur after a normal compile, I do not know if this causes problems or if this is the real reason for this patch. What should be done is find the existing constructor and update it with the definition and return it from the import. This can be done with any type of constructor.

[danix800]

In D156461#4583806, @balazske wrote:

It looks not good to remove an invalid node from the DeclContext that otherwise remains in the AST. I checked the problem and found that the existing move constructor (originally in the To AST which had no definition) gets a getNumCtorInitializers value of 1 but the init_begin returns 0. This causes crash even when dumping it. I did not find the cause of this situation (the first time at line 3822 it is already changed, and ASTImporter has this single position to change the value). Normally what should happen is that a new move constructor is imported (with a definition) and linked after the existing one (and the existing is not modified). We get an AST that does not occur after a normal compile, I do not know if this causes problems or if this is the real reason for this patch. What should be done is find the existing constructor and update it with the definition and return it from the import. This can be done with any type of constructor.

The root cause might be that FunctionDeclBitfields.NumFunctionDeclBits is not in syncing with updated FunctionDeclBitfields.DeductionCandidateKind. See D158145.

[danix800]

In D156461#4583806, @balazske wrote:

Normally what should happen is that a new move constructor is imported (with a definition) and linked after the existing one (and the existing is not modified).

Does this violate the constraint that ctor shouldn't be re-declared?

We get an AST that does not occur after a normal compile, I do not know if this causes problems or if this is the real reason for this patch.

Agreed. The original AST should not be touched.

What should be done is find the existing constructor and update it with the definition and return it from the import. This can be done with any type of constructor.

So does this mean that importer might be improved on this part?

[balazske]

After import we get a new AST that looks like this:

|-CXXConstructorDecl 0x556e6a172d58 <col:16> col:16 implicit used constexpr A 'void (A &&)' inline default trivial noexcept-unevaluated 0x556e6a172d58
| `-ParmVarDecl 0x556e6a172e78 <col:16> col:16 'A &&'
`-CXXConstructorDecl 0x556e6a1ac250 prev 0x556e6a172d58 <input.cc:2:16> col:16 implicit used constexpr A 'void (A &&) noexcept' inline default trivial
  |-ParmVarDecl 0x556e6a1730e0 <col:16> col:16 used 'A &&'
  |-CXXCtorInitializer Field 0x556e6a172850 'm' 'int'
  | `-ImplicitCastExpr 0x556e6a1ac3a0 <col:16> 'int' <LValueToRValue>
  |   `-MemberExpr 0x556e6a1ac370 <col:16> 'int' xvalue .m 0x556e6a172850
  |     `-CXXStaticCastExpr 0x556e6a1ac340 <col:16> 'A':'A' xvalue static_cast<A &&> <NoOp>
  |       `-DeclRefExpr 0x556e6a1ac308 <col:16> 'A':'A' lvalue ParmVar 0x556e6a1730e0 '' 'A &&'
  `-CompoundStmt 0x556e6a1ac3e0 <col:16>

It contains a re-declaration of the (implicit) move constructor, but I do not know if this AST causes problems practically. To avoid this situation we must change the existing To AST in the import process to add the definition of the move constructor (to the existing which has no definition). Theoretically this may be possible, but it is different from how the ASTImporter currently works, now every imported function declaration is created and linked to the existing ones. (But there are already some values that are updated in the To AST.)

clang/lib/AST/ASTImporter.cpp

             NameInfo, T, TInfo, D->getStorageClass(), D->UsesFPIntrin(),
             D->isInlineSpecified(), D->hasWrittenPrototype(),
             D->getConstexprKind(), TrailingRequiresClause))
       return ToFunction;
   }
 
   // Connect the redecl chain.
   if (FoundByLookup) {
-    auto *Recent = const_cast<FunctionDecl *>(
-          FoundByLookup->getMostRecentDecl());
-    ToFunction->setPreviousDecl(Recent);
+//    if (auto *Ctor = dyn_cast<CXXConstructorDecl>(D);
+//        Ctor && Ctor->isImplicit())
+//      DC->removeDecl(FoundByLookup);
+//    else {
+      auto *Recent =
+          const_cast<FunctionDecl *>(FoundByLookup->getMostRecentDecl());
+      ToFunction->setPreviousDecl(Recent);
+//    }
     // FIXME Probably we should merge exception specifications.  E.g. In the
     // "To" context the existing function may have exception specification with
     // noexcept-unevaluated, while the newly imported function may have an
     // evaluated noexcept.  A call to adjustExceptionSpec() on the imported
     // decl and its redeclarations may be required.
   }
 
   ToFunction->setQualifierInfo(ToQualifierLoc);

clang/unittests/AST/ASTImporterTest.cpp

 
 TEST_P(ImportImplicitMethods, DoNotImportOtherMethod) {
   auto Code = R"(
       struct A { void f() { } };
       )";
   testNoImportOf(cxxMethodDecl(hasName("f")), Code);
 }
 
+TEST_P(ImportImplicitMethods, MergeImplicitMethodWithDefinition) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"s(
+        struct A {
+          A() : m() {}
+          int m;
+        };
+
+        A foo() { A a; return a; }
+        A bar() { return {}; }
+      )s",
+      Lang_CXX17,
+      R"s(
+        struct A {
+          A() : m() {}
+          int m;
+        };
+      )s",
+      Lang_CXX17, "A");
+
+  MatchVerifier<Decl> Verifier;
+  auto HasCopyCtor = has(cxxConstructorDecl(isCopyConstructor(), isImplicit()));
+  auto HasCtorInit =
+      hasAnyConstructorInitializer(cxxCtorInitializer(isMemberInitializer()));
+  auto HasMoveCtor =
+      has(cxxConstructorDecl(isMoveConstructor(), isImplicit(), HasCtorInit));
+  auto Pattern = cxxRecordDecl(HasCopyCtor, HasMoveCtor);
+  ASSERT_TRUE(Verifier.match(From, Pattern));
+  EXPECT_TRUE(Verifier.match(To, Pattern));
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ImportOfEquivalentRecord) {
   Decl *ToR1;
   {
     Decl *FromTU = getTuDecl("struct A { };", Lang_CXX03, "input0.cc");
     auto *FromR = FirstDeclMatcher<CXXRecordDecl>().match(
         FromTU, cxxRecordDecl(hasName("A")));
 
     ToR1 = Import(FromR, Lang_CXX03);