[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfeba03340cf3: [clang][ASTImporter] Improve import of 
InjectedClassNameType. (authored by balazske).

Changed prior to commit:
  https://reviews.llvm.org/D140562?vs=492106=501401#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8137,6 +8137,247 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+struct ImportInjectedClassNameType : public ASTImporterOptionSpecificTestBase {
+protected:
+  const CXXRecordDecl *findInjected(const CXXRecordDecl *Parent) {
+for (Decl *Found : Parent->decls()) {
+  const auto *Record = dyn_cast(Found);
+  if (Record && Record->isInjectedClassName())
+return Record;
+}
+return nullptr;
+  }
+
+  void checkInjType(const CXXRecordDecl *D) {
+// The whole redecl chain should have the same InjectedClassNameType
+// instance. The injected record declaration is a separate chain, this
+// should contain the same type too.
+const Type *Ty = nullptr;
+for (const Decl *ReD : D->redecls()) {
+  const auto *ReRD = cast(ReD);
+  EXPECT_TRUE(ReRD->getTypeForDecl());
+  EXPECT_TRUE(!Ty || Ty == ReRD->getTypeForDecl());
+  Ty = ReRD->getTypeForDecl();
+}
+ASSERT_TRUE(Ty);
+const auto *InjTy = Ty->castAs();
+EXPECT_TRUE(InjTy);
+if (CXXRecordDecl *Def = D->getDefinition()) {
+  const CXXRecordDecl *InjRD = findInjected(Def);
+  EXPECT_TRUE(InjRD);
+  EXPECT_EQ(InjRD->getTypeForDecl(), InjTy);
+}
+  }
+
+  void testImport(Decl *ToTU, Decl *FromTU, Decl *FromD) {
+checkInjType(cast(FromD));
+Decl *ToD = Import(FromD, Lang_CXX11);
+if (auto *ToRD = dyn_cast(ToD))
+  checkInjType(ToRD);
+  }
+
+  const char *ToCodeA =
+  R"(
+  template 
+  struct A;
+  )";
+  const char *ToCodeADef =
+  R"(
+  template 
+  struct A {
+typedef A T1;
+  };
+  )";
+  const char *ToCodeC =
+  R"(
+  template 
+  struct C;
+  )";
+  const char *ToCodeCDef =
+  R"(
+  template 
+  struct A {
+typedef A T1;
+  };
+
+  template 
+  struct B {};
+
+  template
+  struct C {
+typedef typename A::T1 T1;
+typedef B T2;
+typedef B T3;
+  };
+  )";
+  const char *FromCode =
+  R"(
+  template 
+  struct A;
+  template 
+  struct A {
+typedef A T1;
+  };
+  template 
+  struct A;
+
+  template 
+  struct B {};
+
+  template 
+  struct C;
+  template 
+  struct C {
+typedef typename A::T1 T1;
+typedef B T2;
+typedef B T3;
+  };
+  template 
+  struct C;
+
+  template 
+  struct D {
+void f(typename C::T3 *);
+  };
+  )";
+};
+
+TEST_P(ImportInjectedClassNameType, ImportADef) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportAFirst) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportALast) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = LastDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportADefToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportAFirstToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportALastToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = LastDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers accepted this revision.
vabridgers added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

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

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

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.




Comment at: clang/lib/AST/ASTContext.cpp:4627
   } else {
-Type *newType =
-  new (*this, TypeAlignment) InjectedClassNameType(Decl, TST);
+Type *newType = new (*this, TypeAlignment) InjectedClassNameType(Decl, 
TST);
 Decl->TypeForDecl = newType;

whisperity wrote:
> (Potentially unrelated change, only touching the format?)
@whisperity is correct, please correct this formatting so it's not a change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy accepted this revision.
donat.nagy added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: vabridgers.
whisperity added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:4627
   } else {
-Type *newType =
-  new (*this, TypeAlignment) InjectedClassNameType(Decl, TST);
+Type *newType = new (*this, TypeAlignment) InjectedClassNameType(Decl, 
TST);
 Decl->TypeForDecl = newType;

(Potentially unrelated change, only touching the format?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 492106.
balazske added a comment.

New patch after more thorough debugging.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8084,6 +8084,247 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+struct ImportInjectedClassNameType : public ASTImporterOptionSpecificTestBase {
+protected:
+  const CXXRecordDecl *findInjected(const CXXRecordDecl *Parent) {
+for (Decl *Found : Parent->decls()) {
+  const auto *Record = dyn_cast(Found);
+  if (Record && Record->isInjectedClassName())
+return Record;
+}
+return nullptr;
+  }
+
+  void checkInjType(const CXXRecordDecl *D) {
+// The whole redecl chain should have the same InjectedClassNameType
+// instance. The injected record declaration is a separate chain, this
+// should contain the same type too.
+const Type *Ty = nullptr;
+for (const Decl *ReD : D->redecls()) {
+  const auto *ReRD = cast(ReD);
+  EXPECT_TRUE(ReRD->getTypeForDecl());
+  EXPECT_TRUE(!Ty || Ty == ReRD->getTypeForDecl());
+  Ty = ReRD->getTypeForDecl();
+}
+ASSERT_TRUE(Ty);
+const auto *InjTy = Ty->castAs();
+EXPECT_TRUE(InjTy);
+if (CXXRecordDecl *Def = D->getDefinition()) {
+  const CXXRecordDecl *InjRD = findInjected(Def);
+  EXPECT_TRUE(InjRD);
+  EXPECT_EQ(InjRD->getTypeForDecl(), InjTy);
+}
+  }
+
+  void testImport(Decl *ToTU, Decl *FromTU, Decl *FromD) {
+checkInjType(cast(FromD));
+Decl *ToD = Import(FromD, Lang_CXX11);
+if (auto *ToRD = dyn_cast(ToD))
+  checkInjType(ToRD);
+  }
+
+  const char *ToCodeA =
+  R"(
+  template 
+  struct A;
+  )";
+  const char *ToCodeADef =
+  R"(
+  template 
+  struct A {
+typedef A T1;
+  };
+  )";
+  const char *ToCodeC =
+  R"(
+  template 
+  struct C;
+  )";
+  const char *ToCodeCDef =
+  R"(
+  template 
+  struct A {
+typedef A T1;
+  };
+
+  template 
+  struct B {};
+
+  template
+  struct C {
+typedef typename A::T1 T1;
+typedef B T2;
+typedef B T3;
+  };
+  )";
+  const char *FromCode =
+  R"(
+  template 
+  struct A;
+  template 
+  struct A {
+typedef A T1;
+  };
+  template 
+  struct A;
+
+  template 
+  struct B {};
+
+  template 
+  struct C;
+  template 
+  struct C {
+typedef typename A::T1 T1;
+typedef B T2;
+typedef B T3;
+  };
+  template 
+  struct C;
+
+  template 
+  struct D {
+void f(typename C::T3 *);
+  };
+  )";
+};
+
+TEST_P(ImportInjectedClassNameType, ImportADef) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportAFirst) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportALast) {
+  Decl *ToTU = getToTuDecl(ToCodeA, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = LastDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportADefToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A"), isDefinition()));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportAFirstToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportALastToDef) {
+  Decl *ToTU = getToTuDecl(ToCodeADef, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromA = LastDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("A")));
+  testImport(ToTU, FromTU, FromA);
+}
+
+TEST_P(ImportInjectedClassNameType, ImportCDef) {
+  Decl *ToTU = getToTuDecl(ToCodeC, Lang_CXX11);
+  Decl *FromTU = getTuDecl(FromCode, Lang_CXX11);
+  auto *FromC = 

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Balázs Kéri via Phabricator via cfe-commits]

balazske planned changes to this revision.
balazske added a comment.

I plan to improve the fix and change the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140562/new/

https://reviews.llvm.org/D140562

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

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 0x7fd8cc15141a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:567:11
   #1 0x7fd8cc1515eb PrintStackTraceSignalHandler(void*) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:641:1
   #2 0x7fd8cc14fb9b llvm::sys::RunSignalHandlers() 
llvm-project/llvm/lib/Support/Signals.cpp:103:5
   #3 0x7fd8cc151d61 SignalHandler(int) 
llvm-project/llvm/lib/Support/Unix/Signals.inc:412:1
   #4 0x7fd8cf630980 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
   #5 0x7fd8cb018e87 raise 
/build/glibc-CVJwZb/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
   #6 0x7fd8cb01a7f1 abort 
/build/glibc-CVJwZb/glibc-2.27/stdlib/abort.c:81:0
   #7 0x7fd8cb00a3fa __assert_fail_base 
/build/glibc-CVJwZb/glibc-2.27/assert/assert.c:89:0
   #8 0x7fd8cb00a472 (/lib/x86_64-linux-gnu/libc.so.6+0x30472)
   #9 0x7fd8cd95cf20 
clang::ASTContext::getTypedefType(clang::TypedefNameDecl const*, 
clang::QualType) const llvm-project/clang/lib/AST/ASTContext.cpp:4680:26
  #10 0x7fd8cda9a574 
clang::ASTNodeImporter::VisitTypedefType(clang::TypedefType const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:1365:34
  #11 0x7fd8cdb05191 clang::TypeVisitor>::Visit(clang::Type const*) 
build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:75:1
  #12 0x7fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
  #13 0x7fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
  #14 0x7fd8cdadb809 llvm::Expected 
clang::ASTNodeImporter::import(clang::QualType const&) 
llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
  #15 0x7fd8cda9be49 
clang::ASTNodeImporter::VisitElaboratedType(clang::ElaboratedType const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:1595:8
  #16 0x7fd8cdb04eb9 clang::TypeVisitor>::Visit(clang::Type const*) 
build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:45:1
  #17 0x7fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
  #18 0x7fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
  #19 0x7fd8cdadb809 llvm::Expected 
clang::ASTNodeImporter::import(clang::QualType const&) 
llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
  #20 0x7fd8cda98d9b 
clang::ASTNodeImporter::VisitPointerType(clang::PointerType const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:1155:8
  #21 0x7fd8cdb0505d clang::TypeVisitor>::Visit(clang::Type const*) 
build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:62:1
  #22 0x7fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
  #23 0x7fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
  #24 0x7fd8cdadb809 llvm::Expected 
clang::ASTNodeImporter::import(clang::QualType const&) 
llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
  #25 0x7fd8cda99d11 
clang::ASTNodeImporter::VisitFunctionProtoType(clang::FunctionProtoType const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:1299:10
  #26 0x7fd8cdb04ef1 clang::TypeVisitor>::Visit(clang::Type const*) 
build/Debug/tools/clang/include/clang/AST/TypeNodes.inc:48:1
  #27 0x7fd8cdad0d35 clang::ASTImporter::Import(clang::Type const*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8651:8
  #28 0x7fd8cdad0ea8 clang::ASTImporter::Import(clang::QualType) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8665:8
  #29 0x7fd8cdadb809 llvm::Expected 
clang::ASTNodeImporter::import(clang::QualType const&) 
llvm-project/clang/lib/AST/ASTImporter.cpp:219:23
  #30 0x7fd8cdaddb5b clang::QualType 
clang::ASTNodeImporter::importChecked(llvm::Error&, 
clang::QualType const&) llvm-project/clang/lib/AST/ASTImporter.cpp:698:12
  #31 0x7fd8cdaa8c38 
clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:3597:12
  #32 0x7fd8cdaaabae 
clang::ASTNodeImporter::VisitCXXMethodDecl(clang::CXXMethodDecl*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:3810:10
  #33 0x7fd8cdb04361 clang::declvisitor::Base>::Visit(clang::Decl*) 
build/Debug/tools/clang/include/clang/AST/DeclNodes.inc:443:1
  #34 0x7fd8cdad0a26 clang::ASTImporter::ImportImpl(clang::Decl*) 
llvm-project/clang/lib/AST/ASTImporter.cpp:8619:19
  #35 0x7fd8cdab3a72 clang::ASTImporter::Import(clang::Decl*) 

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: steakhal, martong, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: All.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140562

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,46 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportInjectedClassNameType) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  struct A {
+typedef A A1;
+void f(A1 *);
+  };
+  )",
+  Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  struct A {
+typedef A A1;
+void f(A1 *);
+  };
+  template
+  void A::f(A::A1 *) {}
+  )",
+  Lang_CXX11);
+
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+  ASTContext  = ToF->getDeclContext()->getParentASTContext();
+
+  auto *ToA1 =
+  FirstDeclMatcher().match(ToTU, typedefDecl(hasName("A1")));
+  QualType ToInjTypedef = ToA1->getUnderlyingType().getCanonicalType();
+  QualType ToInjParmVar =
+  ToF->parameters()[0]->getType().getDesugaredType(ToCtx);
+  ToInjParmVar =
+  ToInjParmVar->getAs()->getPointeeType().getCanonicalType();
+  EXPECT_TRUE(isa(ToInjTypedef));
+  EXPECT_TRUE(isa(ToInjParmVar));
+  EXPECT_TRUE(ToCtx.hasSameType(ToInjTypedef, ToInjParmVar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1468,6 +1468,22 @@
   // 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(TypeForDecl));
+return QualType(TypeForDecl, 0);
+  }
+
   enum {
 TypeAlignmentInBits = 4,
 TypeAlignment = 1 << TypeAlignmentInBits


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,46 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportInjectedClassNameType) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  struct A {
+typedef A A1;
+void f(A1 *);
+  };
+  )",
+  Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  struct A {
+typedef A A1;
+void f(A1 *);
+  };
+  template
+  void A::f(A::A1 *) {}
+  )",
+  Lang_CXX11);
+
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+  ASTContext  = ToF->getDeclContext()->getParentASTContext();
+
+  auto *ToA1 =
+  FirstDeclMatcher().match(ToTU, typedefDecl(hasName("A1")));
+  QualType ToInjTypedef =