From a5e6590b15b17793d7a82f8b39998381acb73109 Mon Sep 17 00:00:00 2001 From: Gabor Marton Date: Fri, 27 Nov 2020 08:32:51 +0100 Subject: [PATCH] [ASTImporter] Support CXXDeductionGuideDecl with local typedef CXXDeductionGuideDecl with a local typedef has its own copy of the TypedefDecl with the CXXDeductionGuideDecl as the DeclContext of that TypedefDecl. ``` template struct A { typedef T U; A(U, T); }; A a{(int)0, (int)0}; ``` Related discussion on cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2020-November/067252.html Without this fix, when we import the CXXDeductionGuideDecl (via VisitFunctionDecl) then before creating the Decl we must import the FunctionType. However, the first parameter's type is the afore mentioned local typedef. So, we then start importing the TypedefDecl whose DeclContext is the CXXDeductionGuideDecl itself. The infinite loop is formed. ``` #0 clang::ASTNodeImporter::VisitCXXDeductionGuideDecl(clang::CXXDeductionGuideDecl*) clang/lib/AST/ASTImporter.cpp:3543:0 #1 clang::declvisitor::Base >::Visit(clang::Decl*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/DeclNodes.inc:405:0 #2 clang::ASTImporter::ImportImpl(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8038:0 #3 clang::ASTImporter::Import(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8200:0 #4 clang::ASTImporter::ImportContext(clang::DeclContext*) clang/lib/AST/ASTImporter.cpp:8297:0 #5 clang::ASTNodeImporter::ImportDeclContext(clang::Decl*, clang::DeclContext*&, clang::DeclContext*&) clang/lib/AST/ASTImporter.cpp:1852:0 #6 clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&) clang/lib/AST/ASTImporter.cpp:1628:0 #7 clang::ASTNodeImporter::VisitTypedefNameDecl(clang::TypedefNameDecl*, bool) clang/lib/AST/ASTImporter.cpp:2419:0 #8 clang::ASTNodeImporter::VisitTypedefDecl(clang::TypedefDecl*) clang/lib/AST/ASTImporter.cpp:2500:0 #9 clang::declvisitor::Base >::Visit(clang::Decl*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/DeclNodes.inc:315:0 #10 clang::ASTImporter::ImportImpl(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8038:0 #11 clang::ASTImporter::Import(clang::Decl*) clang/lib/AST/ASTImporter.cpp:8200:0 #12 llvm::Expected clang::ASTNodeImporter::import(clang::TypedefNameDecl*) clang/lib/AST/ASTImporter.cpp:165:0 #13 clang::ASTNodeImporter::VisitTypedefType(clang::TypedefType const*) clang/lib/AST/ASTImporter.cpp:1304:0 #14 clang::TypeVisitor >::Visit(clang::Type const*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/TypeNodes.inc:74:0 #15 clang::ASTImporter::Import(clang::QualType) clang/lib/AST/ASTImporter.cpp:8071:0 #16 llvm::Expected clang::ASTNodeImporter::import(clang::QualType const&) clang/lib/AST/ASTImporter.cpp:179:0 #17 clang::ASTNodeImporter::VisitFunctionProtoType(clang::FunctionProtoType const*) clang/lib/AST/ASTImporter.cpp:1244:0 #18 clang::TypeVisitor >::Visit(clang::Type const*) /home/egbomrt/WORK/llvm5/build/debug/tools/clang/include/clang/AST/TypeNodes.inc:47:0 #19 clang::ASTImporter::Import(clang::QualType) clang/lib/AST/ASTImporter.cpp:8071:0 #20 llvm::Expected clang::ASTNodeImporter::import(clang::QualType const&) clang/lib/AST/ASTImporter.cpp:179:0 #21 clang::QualType clang::ASTNodeImporter::importChecked(llvm::Error&, clang::QualType const&) clang/lib/AST/ASTImporter.cpp:198:0 #22 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) clang/lib/AST/ASTImporter.cpp:3313:0 #23 clang::ASTNodeImporter::VisitCXXDeductionGuideDecl(clang::CXXDeductionGuideDecl*) clang/lib/AST/ASTImporter.cpp:3543:0 ``` The fix is to first create the TypedefDecl and only then start to import the DeclContext. Basically, we could do this during the import of all other Decls (not just for typedefs). But it seems, there is only one another AST construct that has a similar cycle: a struct defined as a function parameter: ``` int struct_in_proto(struct data_t{int a;int b;} *d); ``` In that case, however, we had decided to return simply with an error back then because that seemed to be a very rare construct. Differential Revision: https://reviews.llvm.org/D92209 --- clang/lib/AST/ASTImporter.cpp | 49 ++++++++++++++++++++++--- clang/unittests/AST/ASTImporterTest.cpp | 16 ++++++++ 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 50632efc281ffa..ea05f2ea455299 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -388,6 +388,8 @@ namespace clang { ExpectedType VisitObjCObjectPointerType(const ObjCObjectPointerType *T); // Importing declarations + Error ImportDeclParts(NamedDecl *D, DeclarationName &Name, NamedDecl *&ToD, + SourceLocation &Loc); Error ImportDeclParts( NamedDecl *D, DeclContext *&DC, DeclContext *&LexicalDC, DeclarationName &Name, NamedDecl *&ToD, SourceLocation &Loc); @@ -1647,6 +1649,25 @@ Error ASTNodeImporter::ImportDeclParts( return Error::success(); } +Error ASTNodeImporter::ImportDeclParts(NamedDecl *D, DeclarationName &Name, + NamedDecl *&ToD, SourceLocation &Loc) { + + // Import the name of this declaration. + if (Error Err = importInto(Name, D->getDeclName())) + return Err; + + // Import the location of this declaration. + if (Error Err = importInto(Loc, D->getLocation())) + return Err; + + ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D)); + if (ToD) + if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD)) + return Err; + + return Error::success(); +} + Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) { if (!FromD) return Error::success(); @@ -2415,22 +2436,29 @@ ExpectedDecl ASTNodeImporter::VisitNamespaceAliasDecl(NamespaceAliasDecl *D) { ExpectedDecl ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) { // Import the major distinguishing characteristics of this typedef. - DeclContext *DC, *LexicalDC; DeclarationName Name; SourceLocation Loc; NamedDecl *ToD; - if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc)) + // Do not import the DeclContext, we will import it once the TypedefNameDecl + // is created. + if (Error Err = ImportDeclParts(D, Name, ToD, Loc)) return std::move(Err); if (ToD) return ToD; + DeclContext *DC = cast_or_null( + Importer.GetAlreadyImportedOrNull(cast(D->getDeclContext()))); + DeclContext *LexicalDC = + cast_or_null(Importer.GetAlreadyImportedOrNull( + cast(D->getLexicalDeclContext()))); + // If this typedef is not in block scope, determine whether we've // seen a typedef with the same name (that we can merge with) or any // other entity by that name (which name lookup could conflict with). // Note: Repeated typedefs are not valid in C99: // 'typedef int T; typedef int T;' is invalid // We do not care about this now. - if (!DC->isFunctionOrMethod()) { + if (DC && !DC->isFunctionOrMethod()) { SmallVector ConflictingDecls; unsigned IDNS = Decl::IDNS_Ordinary; auto FoundDecls = Importer.findDeclsInToCtx(DC, Name); @@ -2487,8 +2515,15 @@ ASTNodeImporter::VisitTypedefNameDecl(TypedefNameDecl *D, bool IsAlias) { Name.getAsIdentifierInfo(), ToTypeSourceInfo)) return ToTypedef; - ToTypedef->setAccess(D->getAccess()); + // Import the DeclContext and set it to the Typedef. + if ((Err = ImportDeclContext(D, DC, LexicalDC))) + return std::move(Err); + ToTypedef->setDeclContext(DC); ToTypedef->setLexicalDeclContext(LexicalDC); + // Add to the lookupTable because we could not do that in MapImported. + Importer.AddToLookupTable(ToTypedef); + + ToTypedef->setAccess(D->getAccess()); // Templated declarations should not appear in DeclContext. TypeAliasDecl *FromAlias = IsAlias ? cast(D) : nullptr; @@ -9198,7 +9233,11 @@ Decl *ASTImporter::MapImported(Decl *From, Decl *To) { // This mapping should be maintained only in this function. Therefore do not // check for additional consistency. ImportedFromDecls[To] = From; - AddToLookupTable(To); + // In the case of TypedefNameDecl we create the Decl first and only then we + // import and set its DeclContext. So, the DC is still not set when we reach + // here from GetImportedOrCreateDecl. + if (To->getDeclContext()) + AddToLookupTable(To); return To; } diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index e4c73a832a146e..2c14244a2eaa95 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -5998,6 +5998,22 @@ TEST_P(ImportFunctions, CTADUserDefinedExplicit) { EXPECT_TRUE(ToD->isExplicit()); } +TEST_P(ImportFunctions, CTADWithLocalTypedef) { + Decl *TU = getTuDecl( + R"( + template struct A { + typedef T U; + A(U); + }; + A a{(int)0}; + )", + Lang_CXX17, "input.cc"); + auto *FromD = FirstDeclMatcher().match( + TU, cxxDeductionGuideDecl()); + auto *ToD = Import(FromD, Lang_CXX17); + ASSERT_TRUE(ToD); +} + // FIXME Move these tests out of ASTImporterTest. For that we need to factor // out the ASTImporter specific pars from ASTImporterOptionSpecificTestBase // into a new test Fixture. Then we should lift up this Fixture to its own