diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h --- a/clang/include/clang/AST/ASTImporter.h +++ b/clang/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; @@ -394,6 +402,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 +414,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, diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/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. @@ -5108,7 +5107,7 @@ } } else { // ODR violation. // FIXME HandleNameConflict - return nullptr; + return make_error(ImportError::NameConflict); } } @@ -7730,7 +7729,6 @@ void ASTImporter::RegisterImportedDecl(Decl *FromD, Decl *ToD) { MapImported(FromD, ToD); - AddToLookupTable(ToD); } Expected ASTImporter::Import(QualType FromT) { @@ -7803,6 +7801,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) { @@ -7813,24 +7816,65 @@ // Import the declaration. ExpectedDecl ToDOrErr = ImportImpl(FromD); - if (!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. + } + + if (!getImportDeclErrorIfAny(FromD)) { + // Error encountered for the first time. + // 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); + } return ToDOrErr; + } + 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); @@ -8541,9 +8585,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 = diff --git a/clang/test/ASTMerge/class-template-partial-spec/test.cpp b/clang/test/ASTMerge/class-template-partial-spec/test.cpp --- a/clang/test/ASTMerge/class-template-partial-spec/test.cpp +++ b/clang/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