]> granicus.if.org Git - clang/commitdiff
[ASTImporter] Store import errors for Decls
authorGabor Marton <gabor.marton@ericsson.com>
Tue, 25 Jun 2019 08:00:51 +0000 (08:00 +0000)
committerGabor Marton <gabor.marton@ericsson.com>
Tue, 25 Jun 2019 08:00:51 +0000 (08:00 +0000)
Summary:
We add a new member which is a 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).  In Import(Decl*) we use this mapping, so
if there was a previous failed import we return with the existing error.

We add/remove from the Lookuptable in consistency with ImportedFromDecls.
When we map a decl in the 'to' context to something in the 'from'
context then and only then we add it to the lookup table. When we
remove a mapping then and only then we remove it from the lookup table.

This patch is the first in a series of patches whose aim is to further
strengthen the error handling in ASTImporter.

Reviewers: a_sidorin, a.sidorin, shafik

Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D62373

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@364279 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/AST/ASTImporter.h
lib/AST/ASTImporter.cpp
test/ASTMerge/class-template-partial-spec/test.cpp
unittests/AST/ASTImporterFixtures.cpp
unittests/AST/ASTImporterFixtures.h
unittests/AST/ASTImporterTest.cpp

index b0dcece5ce9a068d0ea2bea405ce0d530b03b4bd..14fb93d1a83f888c8adba07f394c42a7c913c7bf 100644 (file)
@@ -116,6 +116,14 @@ class TypeSourceInfo;
     /// context to the corresponding declarations in the "to" context.
     llvm::DenseMap<Decl *, Decl *> 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<Decl *, ImportError> ImportDeclErrors;
+
     /// Mapping from the already-imported declarations in the "to"
     /// context to the corresponding declarations in the "from" context.
     llvm::DenseMap<Decl *, Decl *> ImportedFromDecls;
@@ -148,6 +156,9 @@ class TypeSourceInfo;
     /// decl on its own.
     virtual Expected<Decl *> 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 @@ class TypeSourceInfo;
     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 @@ class TypeSourceInfo;
     /// 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<ImportError> 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 @@ class TypeSourceInfo;
     /// \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<unsigned> getFieldIndex(Decl *F);
-
   };
 
 } // namespace clang
index d3c79eac90348d9f8f4697e0a7decc3b7bda5a04..edefa45fb561e73b7e423b9cd2b5bcc1a59a8204 100644 (file)
@@ -253,11 +253,10 @@ namespace clang {
     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<ToDeclT>(Importer.GetAlreadyImportedOrNull(FromD));
       if (ToD)
         return true; // Already imported.
@@ -5113,7 +5112,7 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(
       }
     } else { // ODR violation.
       // FIXME HandleNameConflict
-      return nullptr;
+      return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
@@ -5555,6 +5554,8 @@ ExpectedStmt ASTNodeImporter::VisitStmt(Stmt *S) {
 
 
 ExpectedStmt ASTNodeImporter::VisitGCCAsmStmt(GCCAsmStmt *S) {
+  if (Importer.returnWithErrorInTest())
+    return make_error<ImportError>(ImportError::UnsupportedConstruct);
   SmallVector<IdentifierInfo *, 4> Names;
   for (unsigned I = 0, E = S->getNumOutputs(); I != E; I++) {
     IdentifierInfo *ToII = Importer.Import(S->getOutputIdentifier(I));
@@ -7749,7 +7750,6 @@ Expected<Decl *> ASTImporter::ImportImpl(Decl *FromD) {
 
 void ASTImporter::RegisterImportedDecl(Decl *FromD, Decl *ToD) {
   MapImported(FromD, ToD);
-  AddToLookupTable(ToD);
 }
 
 Expected<QualType> ASTImporter::Import(QualType FromT) {
@@ -7822,6 +7822,11 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
     return nullptr;
 
 
+  // Check whether there was a previous failed import.
+  // If yes return the existing error.
+  if (auto Error = getImportDeclErrorIfAny(FromD))
+    return make_error<ImportError>(*Error);
+
   // Check whether we've already imported this declaration.
   Decl *ToD = GetAlreadyImportedOrNull(FromD);
   if (ToD) {
@@ -7832,24 +7837,65 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) {
 
   // 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<NamedDecl>(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<ImportError>(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<ImportError>(*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 @@ 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);
   return To;
 }
 
+llvm::Optional<ImportError>
+ASTImporter::getImportDeclErrorIfAny(Decl *FromD) const {
+  auto Pos = ImportDeclErrors.find(FromD);
+  if (Pos != ImportDeclErrors.end())
+    return Pos->second;
+  else
+    return Optional<ImportError>();
+}
+
+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<const Type *, const Type *>::iterator Pos =
index fdd0ebd4884dfea296b849266b5ece6c23b0d7b8..6ab5ed5a7d5fb28b34cf95fbb23246e4820aedad 100644 (file)
@@ -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 a0273b617e53a1dc4a5e8775f7806fd4ade19dcf..b2b848f5e57c846e2881f557688134cf80010403 100644 (file)
@@ -161,7 +161,7 @@ TranslationUnitDecl *ASTImporterTestBase::getTuDecl(StringRef SrcCode,
          }) == 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 cfe302124ffdd425d0cf4603b35007c2d399e989..4e666e37aa3aba438fb1278d4a81e8b852518efe 100644 (file)
@@ -124,7 +124,6 @@ private:
   void lazyInitLookupTable(TranslationUnitDecl *ToTU);
 
   void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName);
-  TU *findFromTU(Decl *From);
 
 protected:
   std::unique_ptr<ASTImporterLookupTable> LookupTablePtr;
@@ -133,6 +132,9 @@ public:
   // We may have several From context but only one To context.
   std::unique_ptr<ASTUnit> 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 63bcc609f06dd794902803a3d541a0b8c6d99f18..41a041f101edd4eca65c4a72c8795b8a33c3e6a5 100644 (file)
@@ -4620,6 +4620,127 @@ TEST_P(ImportFriendFunctionTemplates, LookupShouldFindPreviousFriend) {
   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 <typename T>
+      class X {};
+      template <>
+      class X<int> { int a; };
+      )",
+      Lang_CXX);
+  TranslationUnitDecl *FromTU = getTuDecl(
+      R"(
+      template <typename T>
+      class X {};
+      template <>
+      class X<int> { double b; };
+      )",
+      Lang_CXX);
+  auto *FromSpec = FirstDeclMatcher<ClassTemplateSpecializationDecl>().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<ClassTemplateSpecializationDecl>().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<ImportError> 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<FunctionDecl>().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<FunctionDecl>().match(ToTU, functionDecl(hasName("foo"))),
+      0u);
+
+  ASTImporter *Importer = findFromTU(FromFoo)->Importer.get();
+  Optional<ImportError> 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<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+  auto *FromDef =
+      LastDeclMatcher<FunctionDecl>().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<FunctionDecl>().match(ToTU, functionDecl(hasName("f"))),
+            1u);
+  // Match the fwd decl.
+  auto *ToProto =
+      FirstDeclMatcher<FunctionDecl>().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<ImportError> 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, );