[clang][ASTImporter] Improve import of InjectedClassNameType.

[balazske]

Summary

During AST import multiple different InjectedClassNameType objects
could be created for a single class template. This can cause problems
and failed assertions when these types are compared and found to be
not the same (because the instance is different and there is no
canonical type).
The import of this type does not use the factory method in ASTContext,
probably because the preconditions are not fulfilled at that state.
The fix tries to make the code in ASTImporter work more like the code
in ASTContext::getInjectedClassNameType. If a type is stored at the
Decl or previous Decl object, it is reused instead of creating a new
one. This avoids crash at least a part of the cases.

[balazske]

This crash is produced if the test is run without the fix:

[ RUN      ] ParameterizedTests/ASTImporterOptionSpecificTestBase.ImportInjectedClassNameType/0
ASTTests: llvm-project/clang/lib/AST/ASTContext.cpp:4678: clang::QualType clang::ASTContext::getTypedefType(const clang::TypedefNameDecl *, clang::QualType) const: Assertion `hasSameType(Decl->getUnderlyingType(), Underlying)' failed.
 #0 0x00007fd8cc15141a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) llvm-project/llvm/lib/Support/Unix/Signals.inc:567:11
 #1 0x00007fd8cc1515eb PrintStackTraceSignalHandler(void*) llvm-project/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x00007fd8cc14fb9b llvm::sys::RunSignalHandlers() llvm-project/llvm/lib/Support/Signals.cpp:103:5
 #3 0x00007fd8cc151d61 SignalHandler(int) llvm-project/llvm/lib/Support/Unix/Signals.inc:412:1
 #4 0x00007fd8cf630980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x00007fd8cb018e87 raise /build/glibc-CVJwZb/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007fd8cb01a7f1 abort /build/glibc-CVJwZb/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007fd8cb00a3fa __assert_fail_base /build/glibc-CVJwZb/glibc-2.27/assert/assert.c:89:0
 #8 0x00007fd8cb00a472 (/lib/x86_64-linux-gnu/libc.so.6+0x30472)
 #9 0x00007fd8cd95cf20 clang::ASTContext::getTypedefType(clang::TypedefNameDecl const*, clang::QualType) const llvm-project/clang/lib/AST/ASTContext.cpp:4680:26
#10 0x00007fd8cda9a574 clang::ASTNodeImporter::VisitTypedefType(clang::TypedefType const*) llvm-project/clang/lib/AST/ASTImporter.cpp:1365:34
#11 0x00007fd8cdb05191 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType>>::Visit(clang::Type const*) build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:75:1
#12 0x00007fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
#13 0x00007fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
#14 0x00007fd8cdadb809 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
#15 0x00007fd8cda9be49 clang::ASTNodeImporter::VisitElaboratedType(clang::ElaboratedType const*) llvm-project/clang/lib/AST/ASTImporter.cpp:1595:8
#16 0x00007fd8cdb04eb9 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType>>::Visit(clang::Type const*) build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:45:1
#17 0x00007fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
#18 0x00007fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
#19 0x00007fd8cdadb809 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
#20 0x00007fd8cda98d9b clang::ASTNodeImporter::VisitPointerType(clang::PointerType const*) llvm-project/clang/lib/AST/ASTImporter.cpp:1155:8
#21 0x00007fd8cdb0505d clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType>>::Visit(clang::Type const*) build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:62:1
#22 0x00007fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
#23 0x00007fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
#24 0x00007fd8cdadb809 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
#25 0x00007fd8cda99d11 clang::ASTNodeImporter::VisitFunctionProtoType(clang::FunctionProtoType const*) llvm-project/clang/lib/AST/ASTImporter.cpp:1299:10
#26 0x00007fd8cdb04ef1 clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType>>::Visit(clang::Type const*) build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:48:1
#27 0x00007fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
#28 0x00007fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
#29 0x00007fd8cdadb809 llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType>(clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
#30 0x00007fd8cdaddb5b clang::QualType clang::ASTNodeImporter::importChecked<clang::QualType>(llvm::Error&, clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:698:12
#31 0x00007fd8cdaa8c38 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:3597:12
#32 0x00007fd8cdaaabae clang::ASTNodeImporter::VisitCXXMethodDecl(clang::CXXMethodDecl*) llvm-project/clang/lib/AST/ASTImporter.cpp:3810:10
#33 0x00007fd8cdb04361 clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*>>::Visit(clang::Decl*) build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:443:1
#34 0x00007fd8cdad0a26 clang::ASTImporter::ImportImpl(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:8619:19
#35 0x00007fd8cdab3a72 clang::ASTImporter::Import(clang::Decl*) llvm-project/clang/lib/AST/ASTImporter.cpp:9008:8
#36 0x000000000064c702 clang::ast_matchers::ASTImporterTestBase::TU::import(std::shared_ptr<clang::ASTImporterSharedState> const&, clang::ASTUnit*, clang::Decl*) llvm-project/clang/unittests/AST/ASTImporterFixtures.cpp:83:12
#37 0x000000000064d4c2 clang::ast_matchers::ASTImporterTestBase::Import(clang::Decl*, clang::TestLanguage) llvm-project/clang/unittests/AST/ASTImporterFixtures.cpp:199:9
#38 0x00000000006da446 clang::FunctionDecl* clang::ast_matchers::ASTImporterTestBase::Import<clang::FunctionDecl>(clang::FunctionDecl*, clang::TestLanguage) llvm-project/clang/unittests/AST/ASTImporterFixtures.h:185:12
#39 0x00000000006c836d clang::ast_matchers::ASTImporterOptionSpecificTestBase_ImportInjectedClassNameType_Test::TestBody() llvm-project/clang/unittests/AST/ASTImporterTest.cpp:8102:15
#40 0x00007fd8cc2559c4 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm-project/third-party/unittest/googletest/src/gtest.cc:2433:3
#41 0x00007fd8cc23b132 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) llvm-project/third-party/unittest/googletest/src/gtest.cc:2488:5
#42 0x00007fd8cc223173 testing::Test::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2515:3
#43 0x00007fd8cc223a4d testing::TestInfo::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:12
#44 0x00007fd8cc22401c testing::TestSuite::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:2815:44
#45 0x00007fd8cc22cde1 testing::internal::UnitTestImpl::RunAllTests() llvm-project/third-party/unittest/googletest/src/gtest.cc:5337:24
#46 0x00007fd8cc2591e4 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm-project/third-party/unittest/googletest/src/gtest.cc:2433:3
#47 0x00007fd8cc23d472 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) llvm-project/third-party/unittest/googletest/src/gtest.cc:2488:5
#48 0x00007fd8cc22c9aa testing::UnitTest::Run() llvm-project/third-party/unittest/googletest/src/gtest.cc:4925:10
#49 0x00007fd8cfa43d01 RUN_ALL_TESTS() llvm-project/third-party/unittest/googletest/include/gtest/gtest.h:2472:3
#50 0x00007fd8cfa43c40 main llvm-project/third-party/unittest/UnitTestMain/TestMain.cpp:55:3
#51 0x00007fd8caffbc87 __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:344:0
#52 0x00000000005eeb1a _start (tools/clang/unittests/AST/ASTTests+0x5eeb1a)

[balazske]

I plan to improve the fix and change the code.

[balazske]

New patch after more thorough debugging.

[whisperity]

clang/lib/AST/ASTContext.cpp

4627 ↗

(On Diff #492106)

(Potentially unrelated change, only touching the format?)

[whisperity]

@vabridgers Please take a look at this, as per off-list discussion, in relation to D142822.

[donat.nagy]

We reviewed this commit together with @gamesh411 and it looks good to us; we see that it eliminates a possibility where a type object could "acquire" multiple aliases.

[vabridgers]

Patch D144622 should be integrated into this one when a reduced reproducer has been prepared as a unittest and/or LIT case.

I verified the patch stack D144273, D140562, and D144622 (this one) addresses the problems we've seen as a result of D133468. When the patches are ready, a brief history should be included in the commit header.

clang/lib/AST/ASTContext.cpp

4627 ↗

(On Diff #492106)

@whisperity is correct, please correct this formatting so it's not a change.

[donat.nagy]

@vabridgers Thanks for testing these commits!

As @balazske wrote on D144622, I'd suggest handling the three commits (this one = D140562, D144622 and D144273) separately, because they are fixing analogous, but independent problems. (They affect separate branches of the import process and as far as I see they do not influence each other. Among the three commits it's easy to see which modifications are connected logically; if we squished them into one big commit, they would be harder to understand.)

As this commit seems to be ready, I'd suggest merging this now (while we wait for improvements on the other two). @vabridgers What do you think about this plan? Do you see any dependency between the three commits that would make merging just one of them problematic?

[vabridgers]

Hi @donat.nagy , no problem. That's ok for me. Best!

[vabridgers]

LGTM

clang/lib/AST/ASTImporter.cpp

 //===- ASTImporter.cpp - Importing ASTs from other Contexts ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
ExpectedType ASTNodeImporter::VisitInjectedClassNameType(
 
   ExpectedType ToInjTypeOrErr = import(T->getInjectedSpecializationType());
   if (!ToInjTypeOrErr)
     return ToInjTypeOrErr.takeError();
 
   // FIXME: ASTContext::getInjectedClassNameType is not suitable for AST reading
   // See comments in InjectedClassNameType definition for details
   // return Importer.getToContext().getInjectedClassNameType(D, InjType);
+
+  // Take the actions in ASTContext::getInjectedClassNameType here as far as
+  // possible. Try to reuse a previous type if it exists.
+  // We want to avoid that multiple InjectedClassNameType objects are created
+  // for the same Decl object. It is not verified if this works always because
+  // differences in the import order and chain of PreviousDecl.
+
+  CXXRecordDecl *ToDecl = *ToDeclOrErr;
+  const Type *TypeForDecl = ToDecl->getTypeForDecl();
+  if (!TypeForDecl && ToDecl->getPreviousDecl())
+    TypeForDecl = ToDecl->getPreviousDecl()->getTypeForDecl();
+  if (TypeForDecl) {
+    assert(isa<InjectedClassNameType>(TypeForDecl));
+    return QualType(TypeForDecl, 0);
+  }
+
   enum {
     TypeAlignmentInBits = 4,
     TypeAlignment = 1 << TypeAlignmentInBits
   };
 
   return QualType(new (Importer.getToContext(), TypeAlignment)
                   InjectedClassNameType(*ToDeclOrErr, *ToInjTypeOrErr), 0);
 }

clang/unittests/AST/ASTImporterTest.cpp

 //===- unittest/AST/ASTImporterTest.cpp - AST node import test ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
 
   auto *ToBar = FirstDeclMatcher<FunctionDecl>().match(
       ToTU, functionDecl(hasName("bar")));
 
   EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportInjectedClassNameType) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <class T>
+      struct A {
+        typedef A A1;
+        void f(A1 *);
+      };
+      )",
+      Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <class T>
+      struct A {
+        typedef A A1;
+        void f(A1 *);
+      };
+      template<class T>
+      void A<T>::f(A::A1 *) {}
+      )",
+      Lang_CXX11);
+
+  auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+  ASTContext &ToCtx = ToF->getDeclContext()->getParentASTContext();
+
+  auto *ToA1 =
+      FirstDeclMatcher<TypedefDecl>().match(ToTU, typedefDecl(hasName("A1")));
+  QualType ToInjTypedef = ToA1->getUnderlyingType().getCanonicalType();
+  QualType ToInjParmVar =
+      ToF->parameters()[0]->getType().getDesugaredType(ToCtx);
+  ToInjParmVar =
+      ToInjParmVar->getAs<PointerType>()->getPointeeType().getCanonicalType();
+  EXPECT_TRUE(isa<InjectedClassNameType>(ToInjTypedef));
+  EXPECT_TRUE(isa<InjectedClassNameType>(ToInjParmVar));
+  EXPECT_TRUE(ToCtx.hasSameType(ToInjTypedef, ToInjParmVar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);