Index: cfe/trunk/include/clang/AST/ASTImporter.h =================================================================== --- cfe/trunk/include/clang/AST/ASTImporter.h +++ cfe/trunk/include/clang/AST/ASTImporter.h @@ -116,6 +116,14 @@ /// context to the corresponding declarations in the "to" context. llvm::DenseMap ImportedDecls; + /// Mapping from the already-imported declarations in the "from" + /// context to the error status of the import of that declaration. + /// This map contains only the declarations that were not correctly + /// imported. The same declaration may or may not be included in + /// ImportedDecls. This map is updated continuously during imports and never + /// cleared (like ImportedDecls). + llvm::DenseMap ImportDeclErrors; + /// Mapping from the already-imported declarations in the "to" /// context to the corresponding declarations in the "from" context. llvm::DenseMap ImportedFromDecls; @@ -148,6 +156,9 @@ /// decl on its own. virtual Expected ImportImpl(Decl *From); + /// Used only in unittests to verify the behaviour of the error handling. + virtual bool returnWithErrorInTest() { return false; }; + public: /// \param ToContext The context we'll be importing into. @@ -394,6 +405,8 @@ void RegisterImportedDecl(Decl *FromD, Decl *ToD); /// Store and assign the imported declaration to its counterpart. + /// It may happen that several decls from the 'from' context are mapped to + /// the same decl in the 'to' context. Decl *MapImported(Decl *From, Decl *To); /// Called by StructuralEquivalenceContext. If a RecordDecl is @@ -404,6 +417,14 @@ /// importation, eliminating this loop. virtual Decl *GetOriginalDecl(Decl *To) { return nullptr; } + /// Return if import of the given declaration has failed and if yes + /// the kind of the problem. This gives the first error encountered with + /// the node. + llvm::Optional getImportDeclErrorIfAny(Decl *FromD) const; + + /// Mark (newly) imported declaration with error. + void setImportDeclError(Decl *From, ImportError Error); + /// Determine whether the given types are structurally /// equivalent. bool IsStructurallyEquivalent(QualType From, QualType To, @@ -414,7 +435,6 @@ /// \returns The index of the field in its parent context (starting from 0). /// On error `None` is returned (parent context is non-record). static llvm::Optional getFieldIndex(Decl *F); - }; } // namespace clang Index: cfe/trunk/lib/AST/ASTImporter.cpp =================================================================== --- cfe/trunk/lib/AST/ASTImporter.cpp +++ cfe/trunk/lib/AST/ASTImporter.cpp @@ -253,11 +253,10 @@ LLVM_NODISCARD bool GetImportedOrCreateSpecialDecl(ToDeclT *&ToD, CreateFunT CreateFun, FromDeclT *FromD, Args &&... args) { - // FIXME: This code is needed later. - //if (Importer.getImportDeclErrorIfAny(FromD)) { - // ToD = nullptr; - // return true; // Already imported but with error. - //} + if (Importer.getImportDeclErrorIfAny(FromD)) { + ToD = nullptr; + return true; // Already imported but with error. + } ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(FromD)); if (ToD) return true; // Already imported. @@ -5113,7 +5112,7 @@ } } else { // ODR violation. // FIXME HandleNameConflict - return nullptr; + return make_error(ImportError::NameConflict); } } @@ -5555,6 +5554,8 @@ ExpectedStmt ASTNodeImporter::VisitGCCAsmStmt(GCCAsmStmt *S) { + if (Importer.returnWithErrorInTest()) + return make_error(ImportError::UnsupportedConstruct); SmallVector Names; for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++) { IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(I)); @@ -7749,7 +7750,6 @@ void ASTImporter::RegisterImportedDecl(Decl *FromD, Decl *ToD) { MapImported(FromD, ToD); - AddToLookupTable(ToD); } Expected ASTImporter::Import(QualType FromT) { @@ -7822,6 +7822,11 @@ return nullptr; + // Check whether there was a previous failed import. + // If yes return the existing error. + if (auto Error = getImportDeclErrorIfAny(FromD)) + return make_error(*Error); + // Check whether we've already imported this declaration. Decl *ToD = GetAlreadyImportedOrNull(FromD); if (ToD) { @@ -7832,24 +7837,65 @@ // Import the declaration. ExpectedDecl ToDOrErr = ImportImpl(FromD); - if (!ToDOrErr) - return ToDOrErr; + if (!ToDOrErr) { + // Failed to import. + + auto Pos = ImportedDecls.find(FromD); + if (Pos != ImportedDecls.end()) { + // Import failed after the object was created. + // Remove all references to it. + auto *ToD = Pos->second; + ImportedDecls.erase(Pos); + + // ImportedDecls and ImportedFromDecls are not symmetric. It may happen + // (e.g. with namespaces) that several decls from the 'from' context are + // mapped to the same decl in the 'to' context. If we removed entries + // from the LookupTable here then we may end up removing them multiple + // times. + + // The Lookuptable contains decls only which are in the 'to' context. + // Remove from the Lookuptable only if it is *imported* into the 'to' + // context (and do not remove it if it was added during the initial + // traverse of the 'to' context). + auto PosF = ImportedFromDecls.find(ToD); + if (PosF != ImportedFromDecls.end()) { + if (LookupTable) + if (auto *ToND = dyn_cast(ToD)) + LookupTable->remove(ToND); + ImportedFromDecls.erase(PosF); + } + + // FIXME: AST may contain remaining references to the failed object. + } + + // Error encountered for the first time. + assert(!getImportDeclErrorIfAny(FromD) && + "Import error already set for Decl."); + + // After takeError the error is not usable any more in ToDOrErr. + // Get a copy of the error object (any more simple solution for this?). + ImportError ErrOut; + handleAllErrors(ToDOrErr.takeError(), + [&ErrOut](const ImportError &E) { ErrOut = E; }); + setImportDeclError(FromD, ErrOut); + // Do not return ToDOrErr, error was taken out of it. + return make_error(ErrOut); + } + ToD = *ToDOrErr; - // FIXME Use getImportDeclErrorIfAny() here (and return with the error) once - // the error handling is finished in GetImportedOrCreateSpecialDecl(). + // FIXME: Handle the "already imported with error" case. We can get here + // nullptr only if GetImportedOrCreateDecl returned nullptr (after a + // previously failed create was requested). + // Later GetImportedOrCreateDecl can be updated to return the error. if (!ToD) { - return nullptr; + auto Err = getImportDeclErrorIfAny(FromD); + assert(Err); + return make_error(*Err); } // Make sure that ImportImpl registered the imported decl. assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?"); - - // Once the decl is connected to the existing declarations, i.e. when the - // redecl chain is properly set then we populate the lookup again. - // This way the primary context will be able to find all decls. - AddToLookupTable(ToD); - // Notify subclasses. Imported(FromD, ToD); @@ -8560,9 +8606,25 @@ // This mapping should be maintained only in this function. Therefore do not // check for additional consistency. ImportedFromDecls[To] = From; + AddToLookupTable(To); return To; } +llvm::Optional +ASTImporter::getImportDeclErrorIfAny(Decl *FromD) const { + auto Pos = ImportDeclErrors.find(FromD); + if (Pos != ImportDeclErrors.end()) + return Pos->second; + else + return Optional(); +} + +void ASTImporter::setImportDeclError(Decl *From, ImportError Error) { + assert(ImportDeclErrors.find(From) == ImportDeclErrors.end() && + "Setting import error allowed only once for a Decl."); + ImportDeclErrors[From] = Error; +} + bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To, bool Complain) { llvm::DenseMap::iterator Pos = Index: cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp =================================================================== --- cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp +++ cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp @@ -1,5 +1,3 @@ -// FIXME: Crashes after r357394 -// XFAIL: * // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/class-template-partial-spec1.cpp // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/class-template-partial-spec2.cpp // RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s Index: cfe/trunk/unittests/AST/ASTImporterFixtures.h =================================================================== --- cfe/trunk/unittests/AST/ASTImporterFixtures.h +++ cfe/trunk/unittests/AST/ASTImporterFixtures.h @@ -124,7 +124,6 @@ void lazyInitLookupTable(TranslationUnitDecl *ToTU); void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName); - TU *findFromTU(Decl *From); protected: std::unique_ptr LookupTablePtr; @@ -133,6 +132,9 @@ // We may have several From context but only one To context. std::unique_ptr ToAST; + // Returns with the TU associated with the given Decl. + TU *findFromTU(Decl *From); + // Creates an AST both for the From and To source code and imports the Decl // of the identifier into the To context. // Must not be called more than once within the same test. Index: cfe/trunk/unittests/AST/ASTImporterFixtures.cpp =================================================================== --- cfe/trunk/unittests/AST/ASTImporterFixtures.cpp +++ cfe/trunk/unittests/AST/ASTImporterFixtures.cpp @@ -161,7 +161,7 @@ }) == FromTUs.end()); ArgVector Args = getArgVectorForLanguage(Lang); - FromTUs.emplace_back(SrcCode, FileName, Args); + FromTUs.emplace_back(SrcCode, FileName, Args, Creator); TU &Tu = FromTUs.back(); return Tu.TUDecl; Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp =================================================================== --- cfe/trunk/unittests/AST/ASTImporterTest.cpp +++ cfe/trunk/unittests/AST/ASTImporterTest.cpp @@ -4620,6 +4620,127 @@ EXPECT_EQ(Imported->getPreviousDecl(), Friend); } +struct ASTImporterWithFakeErrors : ASTImporter { + using ASTImporter::ASTImporter; + bool returnWithErrorInTest() override { return true; } +}; + +struct ErrorHandlingTest : ASTImporterOptionSpecificTestBase { + ErrorHandlingTest() { + Creator = [](ASTContext &ToContext, FileManager &ToFileManager, + ASTContext &FromContext, FileManager &FromFileManager, + bool MinimalImport, ASTImporterLookupTable *LookupTable) { + return new ASTImporterWithFakeErrors(ToContext, ToFileManager, + FromContext, FromFileManager, + MinimalImport, LookupTable); + }; + } + // In this test we purposely report an error (UnsupportedConstruct) when + // importing the below stmt. + static constexpr auto* ErroneousStmt = R"( asm(""); )"; +}; + +// Check a case when no new AST node is created in the AST before encountering +// the error. +TEST_P(ErrorHandlingTest, ErrorHappensBeforeCreatingANewNode) { + TranslationUnitDecl *ToTU = getToTuDecl( + R"( + template + class X {}; + template <> + class X { int a; }; + )", + Lang_CXX); + TranslationUnitDecl *FromTU = getTuDecl( + R"( + template + class X {}; + template <> + class X { double b; }; + )", + Lang_CXX); + auto *FromSpec = FirstDeclMatcher().match( + FromTU, classTemplateSpecializationDecl(hasName("X"))); + ClassTemplateSpecializationDecl *ImportedSpec = Import(FromSpec, Lang_CXX); + EXPECT_FALSE(ImportedSpec); + + // The original Decl is kept, no new decl is created. + EXPECT_EQ(DeclCounter().match( + ToTU, classTemplateSpecializationDecl(hasName("X"))), + 1u); + + // But an error is set to the counterpart in the "from" context. + ASTImporter *Importer = findFromTU(FromSpec)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromSpec); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::NameConflict); +} + +// Check a case when a new AST node is created but not linked to the AST before +// encountering the error. +TEST_P(ErrorHandlingTest, + ErrorHappensAfterCreatingTheNodeButBeforeLinkingThatToTheAST) { + TranslationUnitDecl *FromTU = getTuDecl( + std::string("void foo() { ") + ErroneousStmt + " }", + Lang_CXX); + auto *FromFoo = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("foo"))); + + FunctionDecl *ImportedFoo = Import(FromFoo, Lang_CXX); + EXPECT_FALSE(ImportedFoo); + + TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl(); + // Created, but not linked. + EXPECT_EQ( + DeclCounter().match(ToTU, functionDecl(hasName("foo"))), + 0u); + + ASTImporter *Importer = findFromTU(FromFoo)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromFoo); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); +} + +// Check a case when a new AST node is created and linked to the AST before +// encountering the error. The error is set for the counterpart of the nodes in +// the "from" context. +TEST_P(ErrorHandlingTest, ErrorHappensAfterNodeIsCreatedAndLinked) { + TranslationUnitDecl *FromTU = getTuDecl( + std::string(R"( + void f(); + void f() { )") + ErroneousStmt + R"( } + )", + Lang_CXX); + auto *FromProto = FirstDeclMatcher().match( + FromTU, functionDecl(hasName("f"))); + auto *FromDef = + LastDeclMatcher().match(FromTU, functionDecl(hasName("f"))); + FunctionDecl *ImportedProto = Import(FromProto, Lang_CXX); + EXPECT_FALSE(ImportedProto); // Could not import. + // However, we created two nodes in the AST. 1) the fwd decl 2) the + // definition. The definition is not added to its DC, but the fwd decl is + // there. + TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl(); + EXPECT_EQ(DeclCounter().match(ToTU, functionDecl(hasName("f"))), + 1u); + // Match the fwd decl. + auto *ToProto = + FirstDeclMatcher().match(ToTU, functionDecl(hasName("f"))); + EXPECT_TRUE(ToProto); + // An error is set to the counterpart in the "from" context both for the fwd + // decl and the definition. + ASTImporter *Importer = findFromTU(FromProto)->Importer.get(); + Optional OptErr = Importer->getImportDeclErrorIfAny(FromProto); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); + OptErr = Importer->getImportDeclErrorIfAny(FromDef); + ASSERT_TRUE(OptErr); + EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct); +} + +INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest, + DefaultTestValuesForRunOptions, ); + INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest, DefaultTestValuesForRunOptions, );