[ASTImporter] Mark erroneous nodes in shared st

[martong]

Summary

This is the third step of the full error handling. We store the errors
for the Decls in the "to" context too. For that, however, we have to put
these errors in a shared state (among all the ASTImporter objects which
handle the same "to" context but different "from" contexts).

After a series of imports from different "from" TUs we have a "to" context
which may have erroneous nodes in it. (Remember, the AST is immutable so
there is no way to delete a node once we had created it and we realized
the error later.) All these erroneous nodes are marked in
ASTImporterSharedState::ImportErrors. Clients of the ASTImporter may
use this as an input. E.g. the static analyzer engine may not try to
analyze a function if that is marked as erroneous (it can be queried via
ASTImporterSharedState::getImportDeclErrorIfAny()).

[a_sidorin]

Hi Gabor!
I haven't find the import sequence examples we try to fix these ways in any of the three patches these change consists of. Could you please provide some (or point if I missed them)?

clang/include/clang/AST/ASTImporterSharedState.h

46 ↗

(On Diff #201182)

= default?

clang/lib/AST/ASTImporter.cpp

7724 ↗

(On Diff #201182)

if (auto* LookupTable = ..._)
  ...
    LookupTable->add();

looks a bit cleaner to me.

7830 ↗

(On Diff #201182)

I'm not sure if it is safe to compare declarations from different TUs by pointers because they belong to different allocators.

7831 ↗

(On Diff #201182)

getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it intentional?

7867 ↗

(On Diff #201182)

I think we can encapsulate these conditions into `SharedState::[add/remove]Decl[To/From]Lookup methods.

7924 ↗

(On Diff #201182)

I don' think that an import failure from another TU means that the import from the current TU will fail.

[martong]

Alexei,

Thanks for the review!
I have provided test cases for the 3 previous patches on which this one depends on. I will provide additional tests next week for this one, and of course will address the other comments.

[martong]

• Add FIXMEs
• Set error for FromD if it maps to an existing Decl which has an error set
• Add test

[martong]

• Set error for FromD if it maps to an existing Decl which has an error set

[martong]

clang/lib/AST/ASTImporter.cpp

7830 ↗

(On Diff #201182)

In the SharedState we keep pointers only from the "to" context.

7831 ↗

(On Diff #201182)

ASTImporter::getImportDeclErrorIfAny() is different from ASTImporterSharedState::getImportDeclErrorIfAny().

The former keeps track of the erroneous decls from the "from" context.

In the latter we keep track of those decls (and their error) which are in the "to" context.
The "to" context is common for all ASTImporter objects in the cross translation unit context.
They share the same ASTImporterLookupTable object too. Thus the name ASTImporterSharedState.

7924 ↗

(On Diff #201182)

It should. At least if we map the newly imported decl to an existing but already messed up decl in the "to" context. Please refer to the added test case.

[martong]

clang/lib/AST/ASTImporter.cpp

7867 ↗

(On Diff #201182)

Good catch, thanks!

[martong]

• Encapsulate by adding addDeclToLookup and removeDeclFromLookup to the shared state

[a_sidorin]

Thanks for the explanation!
It will be good if someone else takes a look at this patch.

clang/include/clang/AST/ASTImporterSharedState.h

40 ↗

(On Diff #206441)

ErrorsInToContext?

[martong]

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.

[martong]

Jenkins had one unrelated failure at
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29812/
Test Result (1 failure / +1)
LLDB.Reproducer.TestGDBRemoteRepro.test

After that the next build is green:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29813/

cfe/trunk/include/clang/AST/ASTImporter.h

 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
 #include <utility>
 
 namespace clang {
 
 class ASTContext;
+class ASTImporterSharedState;
 class Attr;
-class ASTImporterLookupTable;
 class CXXBaseSpecifier;
 class CXXCtorInitializer;
 class Decl;
 class DeclContext;
 class Expr;
 class FileManager;
 class NamedDecl;
 class Stmt;
private:
       VecTy Nodes;
       // Auxiliary container to be able to answer "Do we have a cycle ending
       // at last element?" as fast as possible.
       // We count each Decl's occurrence over the path.
       llvm::SmallDenseMap<Decl *, int, 32> Aux;
     };
 
   private:
-
-    /// Pointer to the import specific lookup table, which may be shared
-    /// amongst several ASTImporter objects.
-    /// This is an externally managed resource (and should exist during the
-    /// lifetime of the ASTImporter object)
-    /// If not set then the original C/C++ lookup is used.
-    ASTImporterLookupTable *LookupTable = nullptr;
+    std::shared_ptr<ASTImporterSharedState> SharedState = nullptr;
 
     /// The path which we go through during the import of a given AST node.
     ImportPathTy ImportPath;
     /// Sometimes we have to save some part of an import path, so later we can
     /// set up properties to the saved nodes.
     /// We may have several of these import paths associated to one Decl.
     using SavedImportPathsForOneDecl =
         llvm::SmallVector<ImportPathTy::VecTy, 32>;
public:
     /// declarations that can be completed at a later point.
     ///
     /// \param LookupTable The importer specific lookup table which may be
     /// shared amongst several ASTImporter objects.
     /// If not set then the original C/C++ lookup is used.
     ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                 ASTContext &FromContext, FileManager &FromFileManager,
                 bool MinimalImport,
-                ASTImporterLookupTable *LookupTable = nullptr);
+                std::shared_ptr<ASTImporterSharedState> SharedState = nullptr);
 
     virtual ~ASTImporter();
 
     /// Whether the importer will perform a minimal import, creating
     /// to-be-completed forward declarations when possible.
     bool isMinimalImport() const { return Minimal; }
 
     /// \brief Import the given object, returns the result.

cfe/trunk/include/clang/AST/ASTImporterSharedState.h

+//===- ASTImporterSharedState.h - ASTImporter specific state --*- C++ -*---===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the ASTImporter specific state, which may be shared
+//  amongst several ASTImporter objects.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
+#define LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H
+
+#include "clang/AST/ASTImporterLookupTable.h"
+#include "llvm/ADT/DenseMap.h"
+// FIXME We need this because of ImportError.
+#include "clang/AST/ASTImporter.h"
+
+namespace clang {
+
+class TranslationUnitDecl;
+
+/// Importer specific state, which may be shared amongst several ASTImporter
+/// objects.
+class ASTImporterSharedState {
+
+  /// Pointer to the import specific lookup table.
+  std::unique_ptr<ASTImporterLookupTable> LookupTable;
+
+  /// Mapping from the already-imported declarations in the "to"
+  /// 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
+  /// ImportedFromDecls. This map is updated continuously during imports and
+  /// never cleared (like ImportedFromDecls).
+  llvm::DenseMap<Decl *, ImportError> ImportErrors;
+
+  // FIXME put ImportedFromDecls here!
+  // And from that point we can better encapsulate the lookup table.
+
+public:
+  ASTImporterSharedState() = default;
+
+  ASTImporterSharedState(TranslationUnitDecl &ToTU) {
+    LookupTable = llvm::make_unique<ASTImporterLookupTable>(ToTU);
+  }
+
+  ASTImporterLookupTable *getLookupTable() { return LookupTable.get(); }
+
+  void addDeclToLookup(Decl *D) {
+    if (LookupTable)
+      if (auto *ND = dyn_cast<NamedDecl>(D))
+        LookupTable->add(ND);
+  }
+
+  void removeDeclFromLookup(Decl *D) {
+    if (LookupTable)
+      if (auto *ND = dyn_cast<NamedDecl>(D))
+        LookupTable->remove(ND);
+  }
+
+  llvm::Optional<ImportError> getImportDeclErrorIfAny(Decl *ToD) const {
+    auto Pos = ImportErrors.find(ToD);
+    if (Pos != ImportErrors.end())
+      return Pos->second;
+    else
+      return Optional<ImportError>();
+  }
+
+  void setImportDeclError(Decl *To, ImportError Error) {
+    ImportErrors[To] = Error;
+  }
+};
+
+} // namespace clang
+#endif // LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H

cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h

 //===--- CrossTranslationUnit.h - -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file provides an interface to load binary AST dumps on demand. This
 //  feature can be utilized for tools that require cross translation unit
 //  support.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 #define LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 class CompilerInstance;
public:
 
   /// Get a name to identify a named decl.
   static std::string getLookupName(const NamedDecl *ND);
 
   /// Emit diagnostics for the user for potential configuration errors.
   void emitCrossTUDiagnostics(const IndexError &IE);
 
 private:
-  void lazyInitLookupTable(TranslationUnitDecl *ToTU);
+  void lazyInitImporterSharedSt(TranslationUnitDecl *ToTU);
   ASTImporter &getOrCreateASTImporter(ASTContext &From);
   template <typename T>
   llvm::Expected<const T *> getCrossTUDefinitionImpl(const T *D,
                                                      StringRef CrossTUDir,
                                                      StringRef IndexName,
                                                      bool DisplayCTUProgress);
   template <typename T>
   const T *findDefInDeclContext(const DeclContext *DC,
                                 StringRef LookupName);
   template <typename T>
   llvm::Expected<const T *> importDefinitionImpl(const T *D);
 
   llvm::StringMap<std::unique_ptr<clang::ASTUnit>> FileASTUnitMap;
   llvm::StringMap<clang::ASTUnit *> NameASTUnitMap;
   llvm::StringMap<std::string> NameFileMap;
   llvm::DenseMap<TranslationUnitDecl *, std::unique_ptr<ASTImporter>>
       ASTUnitImporterMap;
   CompilerInstance &CI;
   ASTContext &Context;
-  std::unique_ptr<ASTImporterLookupTable> LookupTable;
+  std::shared_ptr<ASTImporterSharedState> ImporterSharedSt;
 };
 
 } // namespace cross_tu
 } // namespace clang
 
 #endif // LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H

cfe/trunk/lib/AST/ASTImporter.cpp

 //===- ASTImporter.cpp - Importing ASTs from other Contexts ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ASTImporter class which imports AST nodes from one
 //  context into another context.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclAccessPair.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
for (auto *FromOverriddenMethod : FromMethod->overridden_methods()) {
     else
       consumeError(ImportedOrErr.takeError());
   }
 }
 
 ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          ASTContext &FromContext, FileManager &FromFileManager,
                          bool MinimalImport,
-                         ASTImporterLookupTable *LookupTable)
-    : LookupTable(LookupTable), ToContext(ToContext), FromContext(FromContext),
+                         std::shared_ptr<ASTImporterSharedState> SharedState)
+    : SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),
       Minimal(MinimalImport) {
 
+  // Create a default state without the lookup table: LLDB case.
+  if (!SharedState) {
+    this->SharedState = std::make_shared<ASTImporterSharedState>();
+  }
+
   ImportedDecls[FromContext.getTranslationUnitDecl()] =
       ToContext.getTranslationUnitDecl();
 }
 
 ASTImporter::~ASTImporter() = default;
 
 Optional<unsigned> ASTImporter::getFieldIndex(Decl *F) {
   assert(F && (isa<FieldDecl>(*F) || isa<IndirectFieldDecl>(*F)) &&
ASTImporter::findDeclsInToCtx(DeclContext *DC, DeclarationName Name) {
   // We search in the redecl context because of transparent contexts.
   // E.g. a simple C language enum is a transparent context:
   //   enum E { A, B };
   // Now if we had a global variable in the TU
   //   int A;
   // then the enum constant 'A' and the variable 'A' violates ODR.
   // We can diagnose this only if we search in the redecl context.
   DeclContext *ReDC = DC->getRedeclContext();
-  if (LookupTable) {
+  if (SharedState->getLookupTable()) {
     ASTImporterLookupTable::LookupResult LookupResult =
-        LookupTable->lookup(ReDC, Name);
+        SharedState->getLookupTable()->lookup(ReDC, Name);
     return FoundDeclsTy(LookupResult.begin(), LookupResult.end());
   } else {
     // FIXME Can we remove this kind of lookup?
     // Or lldb really needs this C/C++ lookup?
     FoundDeclsTy Result;
     ReDC->localUncachedLookup(Name, Result);
     return Result;
   }
 }
 
 void ASTImporter::AddToLookupTable(Decl *ToD) {
-  if (LookupTable)
-    if (auto *ToND = dyn_cast<NamedDecl>(ToD))
-      LookupTable->add(ToND);
+  SharedState->addDeclToLookup(ToD);
 }
 
 Expected<Decl *> ASTImporter::ImportImpl(Decl *FromD) {
   // Import the decl using ASTNodeImporter.
   ASTNodeImporter Importer(*this);
   return Importer.Visit(FromD);
 }
 
Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   // 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) {
+    // Already imported (possibly from another TU) and with an error.
+    if (auto Error = SharedState->getImportDeclErrorIfAny(ToD)) {
+      setImportDeclError(FromD, *Error);
+      return make_error<ImportError>(*Error);
+    }
+
     // If FromD has some updated flags after last import, apply it
     updateFlags(FromD, ToD);
     // If we encounter a cycle during an import then we save the relevant part
     // of the import path associated to the Decl.
     if (ImportPath.hasCycleAtBack())
       SavedImportPaths[FromD].push_back(ImportPath.copyCycleAtBack());
     return ToD;
   }
if (Pos != ImportedDecls.end()) {
       // 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);
+        SharedState->removeDeclFromLookup(ToD);
         ImportedFromDecls.erase(PosF);
       }
 
       // FIXME: AST may contain remaining references to the failed object.
+      // However, the ImportDeclErrors in the shared state contains all the
+      // failed objects together with their error.
     }
 
-    // After takeError the error is not usable anymore in ToDOrErr.
+    // 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);
+    // Set the error for the mapped to Decl, which is in the "to" context.
+    if (Pos != ImportedDecls.end())
+      SharedState->setImportDeclError(Pos->second, ErrOut);
 
     // Set the error for all nodes which have been created before we
     // recognized the error.
     for (const auto &Path : SavedImportPaths[FromD])
-      for (Decl *Di : Path)
-        setImportDeclError(Di, ErrOut);
+      for (Decl *FromDi : Path) {
+        setImportDeclError(FromDi, ErrOut);
+        //FIXME Should we remove these Decls from ImportedDecls?
+        // Set the error for the mapped to Decl, which is in the "to" context.
+        auto Ii = ImportedDecls.find(FromDi);
+        if (Ii != ImportedDecls.end())
+          SharedState->setImportDeclError(Ii->second, ErrOut);
+          // FIXME Should we remove these Decls from the LookupTable,
+          // and from ImportedFromDecls?
+      }
     SavedImportPaths[FromD].clear();
 
     // Do not return ToDOrErr, error was taken out of it.
     return make_error<ImportError>(ErrOut);
   }
 
   ToD = *ToDOrErr;
 
   // 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) {
     auto Err = getImportDeclErrorIfAny(FromD);
     assert(Err);
     return make_error<ImportError>(*Err);
   }
 
+  // We could import from the current TU without error.  But previously we
+  // already had imported a Decl as `ToD` from another TU (with another
+  // ASTImporter object) and with an error.
+  if (auto Error = SharedState->getImportDeclErrorIfAny(ToD)) {
+    setImportDeclError(FromD, *Error);
+    return make_error<ImportError>(*Error);
+  }
+
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
+
   // Notify subclasses.
   Imported(FromD, ToD);
 
   updateFlags(FromD, ToD);
   SavedImportPaths[FromD].clear();
   return ToDOrErr;
 }
 

cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp

CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
   return importDefinitionImpl(FD);
 }
 
 llvm::Expected<const VarDecl *>
 CrossTranslationUnitContext::importDefinition(const VarDecl *VD) {
   return importDefinitionImpl(VD);
 }
 
-void CrossTranslationUnitContext::lazyInitLookupTable(
+void CrossTranslationUnitContext::lazyInitImporterSharedSt(
     TranslationUnitDecl *ToTU) {
-  if (!LookupTable)
-    LookupTable = llvm::make_unique<ASTImporterLookupTable>(*ToTU);
+  if (!ImporterSharedSt)
+    ImporterSharedSt = std::make_shared<ASTImporterSharedState>(*ToTU);
 }
 
 ASTImporter &
 CrossTranslationUnitContext::getOrCreateASTImporter(ASTContext &From) {
   auto I = ASTUnitImporterMap.find(From.getTranslationUnitDecl());
   if (I != ASTUnitImporterMap.end())
     return *I->second;
-  lazyInitLookupTable(Context.getTranslationUnitDecl());
+  lazyInitImporterSharedSt(Context.getTranslationUnitDecl());
   ASTImporter *NewImporter = new ASTImporter(
       Context, Context.getSourceManager().getFileManager(), From,
-      From.getSourceManager().getFileManager(), false, LookupTable.get());
+      From.getSourceManager().getFileManager(), false, ImporterSharedSt);
   ASTUnitImporterMap[From.getTranslationUnitDecl()].reset(NewImporter);
   return *NewImporter;
 }
 
 } // namespace cross_tu
 } // namespace clang

cfe/trunk/lib/Frontend/ASTMerge.cpp

 //===-- ASTMerge.cpp - AST Merging Frontend Action --------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 
 using namespace clang;
 
 std::unique_ptr<ASTConsumer>
 ASTMergeAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {

 void ASTMergeAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   CI.getDiagnostics().getClient()->BeginSourceFile(
                                              CI.getASTContext().getLangOpts());
   CI.getDiagnostics().SetArgToStringFn(&FormatASTNodeDiagnosticArgument,
                                        &CI.getASTContext());
   IntrusiveRefCntPtr<DiagnosticIDs>
       DiagIDs(CI.getDiagnostics().getDiagnosticIDs());
-  ASTImporterLookupTable LookupTable(
+  auto SharedState = std::make_shared<ASTImporterSharedState>(
       *CI.getASTContext().getTranslationUnitDecl());
   for (unsigned I = 0, N = ASTFiles.size(); I != N; ++I) {
     IntrusiveRefCntPtr<DiagnosticsEngine>
         Diags(new DiagnosticsEngine(DiagIDs, &CI.getDiagnosticOpts(),
                                     new ForwardingDiagnosticConsumer(
                                           *CI.getDiagnostics().getClient()),
                                     /*ShouldOwnClient=*/true));
     std::unique_ptr<ASTUnit> Unit = ASTUnit::LoadFromASTFile(
         ASTFiles[I], CI.getPCHContainerReader(), ASTUnit::LoadEverything, Diags,
         CI.getFileSystemOpts(), false);
 
     if (!Unit)
       continue;
 
     ASTImporter Importer(CI.getASTContext(), CI.getFileManager(),
                          Unit->getASTContext(), Unit->getFileManager(),
-                         /*MinimalImport=*/false, &LookupTable);
+                         /*MinimalImport=*/false, SharedState);
 
     TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
     for (auto *D : TU->decls()) {
       // Don't re-import __va_list_tag, __builtin_va_list.
       if (const auto *ND = dyn_cast<NamedDecl>(D))
         if (IdentifierInfo *II = ND->getIdentifier())
           if (II->isStr("__va_list_tag") || II->isStr("__builtin_va_list"))
             continue;

cfe/trunk/unittests/AST/ASTImporterFixtures.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_UNITTESTS_AST_IMPORTER_FIXTURES_H
 #define LLVM_CLANG_UNITTESTS_AST_IMPORTER_FIXTURES_H
 
 #include "gmock/gmock.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/Frontend/ASTUnit.h"
+#include "clang/AST/ASTImporterSharedState.h"
 
 #include "DeclMatcher.h"
 #include "Language.h"
 
 namespace clang {
 
 class ASTImporter;
-class ASTImporterLookupTable;
+class ASTImporterSharedState;
 class ASTUnit;
 
 namespace ast_matchers {
 
 const StringRef DeclToImportID = "declToImport";
 const StringRef DeclToVerifyID = "declToVerify";
 
 // Creates a virtual file and assigns that to the context of given AST. If the

 // this fixture makes it possible to import from several "From" contexts.
 class ASTImporterTestBase : public CompilerOptionSpecificTest {
 
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
 public:
   /// Allocates an ASTImporter (or one of its subclasses).
-  typedef std::function<ASTImporter *(ASTContext &, FileManager &, ASTContext &,
-                                      FileManager &, bool,
-                                      ASTImporterLookupTable *)>
+  typedef std::function<ASTImporter *(
+      ASTContext &, FileManager &, ASTContext &, FileManager &, bool,
+      const std::shared_ptr<ASTImporterSharedState> &SharedState)>
       ImporterConstructor;
 
   // The lambda that constructs the ASTImporter we use in this test.
   ImporterConstructor Creator;
 
 private:
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
   // Represents a "From" translation unit and holds an importer object which we
   // use to import from this translation unit.
   struct TU {
     // Buffer for the context, must live in the test scope.
     std::string Code;
     std::string FileName;
     std::unique_ptr<ASTUnit> Unit;
     TranslationUnitDecl *TUDecl = nullptr;
     std::unique_ptr<ASTImporter> Importer;
     ImporterConstructor Creator;
 
     TU(StringRef Code, StringRef FileName, ArgVector Args,
        ImporterConstructor C = ImporterConstructor());
     ~TU();
 
-    void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST);
-    Decl *import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
-                 Decl *FromDecl);
-    QualType import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
-                    QualType FromType);
+    void
+    lazyInitImporter(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                     ASTUnit *ToAST);
+    Decl *import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                 ASTUnit *ToAST, Decl *FromDecl);
+    QualType import(const std::shared_ptr<ASTImporterSharedState> &SharedState,
+                    ASTUnit *ToAST, QualType FromType);
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
   // moving of the stored objects because that would mean breaking the
   // references in the AST. By using a vector a move could happen when the
   // vector is expanding, with the list we won't have these issues.
   std::list<TU> FromTUs;
 
-  // Initialize the lookup table if not initialized already.
-  void lazyInitLookupTable(TranslationUnitDecl *ToTU);
+  // Initialize the shared state if not initialized already.
+  void lazyInitSharedState(TranslationUnitDecl *ToTU);
 
   void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName);
 
 protected:
-  std::unique_ptr<ASTImporterLookupTable> LookupTablePtr;
+  std::shared_ptr<ASTImporterSharedState> SharedStatePtr;
 
 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);
 

cfe/trunk/unittests/AST/ASTImporterFixtures.cpp

 //===- unittest/AST/ASTImporterFixtures.cpp - AST unit test support -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// Implementation of fixture classes for testing the ASTImporter.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ASTImporterFixtures.h"
 
 #include "clang/AST/ASTImporter.h"
-#include "clang/AST/ASTImporterLookupTable.h"
+#include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Tooling/Tooling.h"
 
 namespace clang {
 namespace ast_matchers {
 
 void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
                                std::unique_ptr<llvm::MemoryBuffer> &&Buffer) {
: Code(Code), FileName(FileName),
       TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C) {
   Unit->enableSourceFileDiagnostics();
 
   // If the test doesn't need a specific ASTImporter, we just create a
   // normal ASTImporter with it.
   if (!Creator)
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
-                 bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporter(ToContext, ToFileManager, FromContext,
-                             FromFileManager, MinimalImport, LookupTable);
+                             FromFileManager, MinimalImport, SharedState);
     };
 }
 
 ASTImporterTestBase::TU::~TU() {}
 
 void ASTImporterTestBase::TU::lazyInitImporter(
-    ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
+    const std::shared_ptr<ASTImporterSharedState> &SharedState,
+    ASTUnit *ToAST) {
   assert(ToAST);
   if (!Importer)
     Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                            Unit->getASTContext(), Unit->getFileManager(), false,
-                           &LookupTable));
+                           SharedState));
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
 
-Decl *ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
-                                      ASTUnit *ToAST, Decl *FromDecl) {
-  lazyInitImporter(LookupTable, ToAST);
+Decl *ASTImporterTestBase::TU::import(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    Decl *FromDecl) {
+  lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromDecl))
     return *ImportedOrErr;
   else {
     llvm::consumeError(ImportedOrErr.takeError());
     return nullptr;
   }
 }
 
-QualType ASTImporterTestBase::TU::import(ASTImporterLookupTable &LookupTable,
-                                         ASTUnit *ToAST, QualType FromType) {
-  lazyInitImporter(LookupTable, ToAST);
+QualType ASTImporterTestBase::TU::import(
+    const std::shared_ptr<ASTImporterSharedState> &SharedState, ASTUnit *ToAST,
+    QualType FromType) {
+  lazyInitImporter(SharedState, ToAST);
   if (auto ImportedOrErr = Importer->Import(FromType))
     return *ImportedOrErr;
   else {
     llvm::consumeError(ImportedOrErr.takeError());
     return QualType{};
   }
 }
 
-void ASTImporterTestBase::lazyInitLookupTable(TranslationUnitDecl *ToTU) {
+void ASTImporterTestBase::lazyInitSharedState(TranslationUnitDecl *ToTU) {
   assert(ToTU);
-  if (!LookupTablePtr)
-    LookupTablePtr = llvm::make_unique<ASTImporterLookupTable>(*ToTU);
+  if (!SharedStatePtr)
+    SharedStatePtr = std::make_shared<ASTImporterSharedState>(*ToTU);
 }
 
 void ASTImporterTestBase::lazyInitToAST(Language ToLang, StringRef ToSrcCode,
                                         StringRef FileName) {
   if (ToAST)
     return;
   ArgVector ToArgs = getArgVectorForLanguage(ToLang);
   // Source code must be a valid live buffer through the tests lifetime.
   ToCode = ToSrcCode;
   // Build the AST from an empty file.
   ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, FileName);
   ToAST->enableSourceFileDiagnostics();
-  lazyInitLookupTable(ToAST->getASTContext().getTranslationUnitDecl());
+  lazyInitSharedState(ToAST->getASTContext().getTranslationUnitDecl());
 }
 
 ASTImporterTestBase::TU *ASTImporterTestBase::findFromTU(Decl *From) {
   // Create a virtual file in the To Ctx which corresponds to the file from
   // which we want to import the `From` Decl. Without this source locations
   // will be invalid in the ToCtx.
   auto It = llvm::find_if(FromTUs, [From](const TU &E) {
     return E.TUDecl == From->getTranslationUnitDecl();
ASTImporterTestBase::getImportedDecl(StringRef FromSrcCode, Language FromLang,
   DeclarationName ImportDeclName(ImportedII);
   SmallVector<NamedDecl *, 1> FoundDecls;
   FromCtx.getTranslationUnitDecl()->localUncachedLookup(ImportDeclName,
                                                         FoundDecls);
 
   assert(FoundDecls.size() == 1);
 
   Decl *Imported =
-      FromTU.import(*LookupTablePtr, ToAST.get(), FoundDecls.front());
+      FromTU.import(SharedStatePtr, ToAST.get(), FoundDecls.front());
 
   assert(Imported);
   return std::make_tuple(*FoundDecls.begin(), Imported);
 }
 
 TranslationUnitDecl *ASTImporterTestBase::getTuDecl(StringRef SrcCode,
                                                     Language Lang,
                                                     StringRef FileName) {
TranslationUnitDecl *ASTImporterTestBase::getToTuDecl(StringRef ToSrcCode,
   assert(!ToAST);
   lazyInitToAST(ToLang, ToSrcCode, OutputFileName);
   return ToAST->getASTContext().getTranslationUnitDecl();
 }
 
 Decl *ASTImporterTestBase::Import(Decl *From, Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(From);
-  assert(LookupTablePtr);
-  return FromTU->import(*LookupTablePtr, ToAST.get(), From);
+  assert(SharedStatePtr);
+  Decl *To = FromTU->import(SharedStatePtr, ToAST.get(), From);
+  return To;
 }
 
 QualType ASTImporterTestBase::ImportType(QualType FromType, Decl *TUDecl,
                                          Language ToLang) {
   lazyInitToAST(ToLang, "", OutputFileName);
   TU *FromTU = findFromTU(TUDecl);
-  assert(LookupTablePtr);
-  return FromTU->import(*LookupTablePtr, ToAST.get(), FromType);
+  assert(SharedStatePtr);
+  return FromTU->import(SharedStatePtr, ToAST.get(), FromType);
 }
 
 ASTImporterTestBase::~ASTImporterTestBase() {
   if (!::testing::Test::HasFailure())
     return;
 
   for (auto &Tu : FromTUs) {
     assert(Tu.Unit);

cfe/trunk/unittests/AST/ASTImporterTest.cpp

 };
 
 } // namespace
 
 struct RedirectingImporterTest : ASTImporterOptionSpecificTestBase {
   RedirectingImporterTest() {
     Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
                  ASTContext &FromContext, FileManager &FromFileManager,
-                 bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new RedirectingImporter(ToContext, ToFileManager, FromContext,
                                      FromFileManager, MinimalImport,
-                                     LookupTable);
+                                     SharedState);
     };
   }
 };
 
 // Test that an ASTImporter subclass can intercept an import call.
 TEST_P(RedirectingImporterTest, InterceptImport) {
   Decl *From, *To;
   std::tie(From, To) =
auto *ToClass = FirstDeclMatcher<CXXRecordDecl>().match(
         ToTU, ClassMatcher);
 
     ASSERT_EQ(DeclCounter<CXXMethodDecl>().match(ToClass, MethodMatcher), 1u);
 
     {
       CXXMethodDecl *Method =
           FirstDeclMatcher<CXXMethodDecl>().match(ToClass, MethodMatcher);
       ToClass->removeDecl(Method);
-      LookupTablePtr->remove(Method);
+      SharedStatePtr->getLookupTable()->remove(Method);
     }
 
     ASSERT_EQ(DeclCounter<CXXMethodDecl>().match(ToClass, MethodMatcher), 0u);
 
     Decl *ImportedClass = nullptr;
     {
       Decl *FromTU = getTuDecl(Code, Lang_CXX11, "input1.cc");
       auto *FromClass = FirstDeclMatcher<CXXRecordDecl>().match(
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) {
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
       return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
                                            FromContext, FromFileManager,
-                                           MinimalImport, LookupTable);
+                                           MinimalImport, SharedState);
     };
   }
   // 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
auto *FromOK = FirstDeclMatcher<FunctionDecl>().match(
       FromTU, functionDecl(hasName("ok")));
   OptErr = Importer->getImportDeclErrorIfAny(FromOK);
   EXPECT_FALSE(OptErr);
   // And we should be able to import.
   FunctionDecl *ImportedOK = Import(FromOK, Lang_CXX);
   EXPECT_TRUE(ImportedOK);
 }
 
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+       ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+  // We already have a fwd decl.
+  TranslationUnitDecl *ToTU = getToTuDecl(
+      "class X;", Lang_CXX);
+  // Then we import a definition.
+  {
+    TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+        class X {
+          void f() { )") + ErroneousStmt + R"( }
+          void ok();
+        };
+        )",
+        Lang_CXX);
+    auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+        FromTU, cxxRecordDecl(hasName("X")));
+    CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+    // An error is set for X ...
+    EXPECT_FALSE(ImportedX);
+    ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+    Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+    ASSERT_TRUE(OptErr);
+    EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  }
+  // ... but the node had been created.
+  auto *ToXDef = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+  // An error is set for "ToXDef" in the shared state.
+  Optional<ImportError> OptErr =
+      SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+  auto *ToXFwd = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition())));
+  // An error is NOT set for the fwd Decl of X in the shared state.
+  OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+  ASSERT_FALSE(OptErr);
+
+  // Try to import  X again but from another TU.
+  {
+    TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+        class X {
+          void f() { )") + ErroneousStmt + R"( }
+          void ok();
+        };
+        )",
+        Lang_CXX, "input1.cc");
+
+    auto *FromX = FirstDeclMatcher<CXXRecordDecl>().match(
+        FromTU, cxxRecordDecl(hasName("X")));
+    CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+    // If we did not save the errors for the "to" context then the below checks
+    // would fail, because the lookup finds the fwd Decl of the existing
+    // definition in the "to" context. We can reach the existing definition via
+    // the found fwd Decl. That existing definition is structurally equivalent
+    // (we check only the fields) with this one we want to import, so we return
+    // with the existing definition, which is erroneous (one method is missing).
+
+    // The import should fail.
+    EXPECT_FALSE(ImportedX);
+    ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+    Optional<ImportError> OptErr = Importer->getImportDeclErrorIfAny(FromX);
+    // And an error is set for this new X in the "from" ctx.
+    ASSERT_TRUE(OptErr);
+    EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
                         ::testing::Values(ArgVector()), );