[ASTImporter] Refactor Decl creation

Summary:
Generalize the creation of Decl nodes during Import.  With this patch we do the
same things after and before a new AST node is created (::Create) The import
logic should be really simple, we create the node, then we mark that as
imported, then we recursively import the parts for that node and then set them
on that node.  However, the AST is actually a graph, so we have to handle
circles.  If we mark something as imported (`MapImported()`) then we return with
the corresponding `To` decl whenever we want to import that node again, this way
circles are handled.  In order to make this algorithm work we must ensure
things, which are handled in the generic CreateDecl<> template:
* There are no `Import()` calls in between any node creation (::Create)
and the `MapImported()` call.
* Before actually creating an AST node (::Create), we must check if
the Node had been imported already, if yes then return with that one.
One very important case for this is connected to templates: we may
start an import both from the templated decl of a template and from
the template itself.

Now, the virtual `Imported` function is called in `ASTImporter::Impor(Decl *)`,
but only once, when the `Decl` is imported.  One point of this refactor is to
separate responsibilities. The original `Imported()` had 3 responsibilities:
- notify subclasses when an import happened
- register the decl into `ImportedDecls`
- initialise the Decl (set attributes, etc)
Now all of these are in separate functions:
- `Imported`
- `MapImported`
- `InitializeImportedDecl`
I tried to check all the clients, I executed tests for `ExternalASTMerger.cpp`
and some unittests for lldb.

Reviewers: a.sidorin, balazske, xazax.hun, r.stahl

Subscribers: rnkovacs, dkrupp, cfe-commits

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

llvm-svn: 336896

clang/include/clang/AST/ASTImporter.h

@@ -314,13 +314,13 @@ class Attr;
     /// \param D A declaration in the "to" context.
     virtual void CompleteDecl(Decl* D);
     
-    /// Note that we have imported the "from" declaration by mapping it
-    /// to the (potentially-newly-created) "to" declaration.
-    ///
     /// Subclasses can override this function to observe all of the \c From ->
     /// \c To declaration mappings as they are imported.
-    virtual Decl *Imported(Decl *From, Decl *To);
-      
+    virtual Decl *Imported(Decl *From, Decl *To) { return To; }
+
+    /// Store and assign the imported declaration to its counterpart.
+    Decl *MapImported(Decl *From, Decl *To);
+
     /// Called by StructuralEquivalenceContext.  If a RecordDecl is
     /// being compared to another RecordDecl as part of import, completing the
     /// other RecordDecl may trigger importation of the first RecordDecl. This

clang/include/clang/AST/ASTStructuralEquivalence.h

@@ -30,6 +30,14 @@ class QualType;
 class RecordDecl;
 class SourceLocation;
 
+/// \brief Whether to perform a normal or minimal equivalence check.
+/// In case of `Minimal`, we do not perform a recursive check of decls with
+/// external storage.
+enum class StructuralEquivalenceKind {
+  Default,
+  Minimal,
+};
+
 struct StructuralEquivalenceContext {
   /// AST contexts for which we are checking structural equivalence.
   ASTContext &FromCtx, &ToCtx;
@@ -47,6 +55,8 @@ struct StructuralEquivalenceContext {
   /// (which we have already complained about).
   llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls;
 
+  StructuralEquivalenceKind EqKind;
+
   /// Whether we're being strict about the spelling of types when
   /// unifying two types.
   bool StrictTypeSpelling;
@@ -63,10 +73,11 @@ struct StructuralEquivalenceContext {
   StructuralEquivalenceContext(
       ASTContext &FromCtx, ASTContext &ToCtx,
       llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls,
+      StructuralEquivalenceKind EqKind,
       bool StrictTypeSpelling = false, bool Complain = true,
       bool ErrorOnTagTypeMismatch = false)
       : FromCtx(FromCtx), ToCtx(ToCtx), NonEquivalentDecls(NonEquivalentDecls),
-        StrictTypeSpelling(StrictTypeSpelling),
+        EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling),
         ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain) {}
 
   DiagnosticBuilder Diag1(SourceLocation Loc, unsigned DiagID);

clang/include/clang/AST/DeclBase.h

@@ -309,7 +309,7 @@ private:
 protected:
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
-  friend class ASTImporter;
+  friend class ASTNodeImporter;
   friend class ASTReader;
   friend class CXXClassMemberWrapper;
   friend class LinkageComputer;

clang/lib/AST/ASTStructuralEquivalence.cpp

@@ -955,6 +955,15 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   if (!D1 || !D2)
     return true;
 
+  // If any of the records has external storage and we do a minimal check (or
+  // AST import) we assmue they are equivalent. (If we didn't have this
+  // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
+  // another AST import which in turn would call the structural equivalency
+  // check again and finally we'd have an improper result.)
+  if (Context.EqKind == StructuralEquivalenceKind::Minimal)
+    if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
+      return true;
+
   if (auto *D1CXX = dyn_cast<CXXRecordDecl>(D1)) {
     if (auto *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
       if (D1CXX->hasExternalLexicalStorage() &&

clang/lib/AST/ExternalASTMerger.cpp

@@ -154,7 +154,7 @@ public:
       ToContainer->setMustBuildLookupTable();
       assert(Parent.CanComplete(ToContainer));
     }
-    return ASTImporter::Imported(From, To);
+    return To;
   }
   ASTImporter &GetReverse() { return Reverse; }
 };
@@ -229,7 +229,7 @@ void ExternalASTMerger::CompleteType(TagDecl *Tag) {
       SourceTag->getASTContext().getExternalSource()->CompleteType(SourceTag);
     if (!SourceTag->getDefinition())
       return false;
-    Forward.Imported(SourceTag, Tag);
+    Forward.MapImported(SourceTag, Tag);
     Forward.ImportDefinition(SourceTag);
     Tag->setCompleteDefinition(SourceTag->isCompleteDefinition());
     return true;
@@ -248,7 +248,7 @@ void ExternalASTMerger::CompleteType(ObjCInterfaceDecl *Interface) {
               SourceInterface);
         if (!SourceInterface->getDefinition())
           return false;
-        Forward.Imported(SourceInterface, Interface);
+        Forward.MapImported(SourceInterface, Interface);
         Forward.ImportDefinition(SourceInterface);
         return true;
       });
@@ -304,7 +304,7 @@ void ExternalASTMerger::ForceRecordOrigin(const DeclContext *ToDC,
 void ExternalASTMerger::RecordOriginImpl(const DeclContext *ToDC, DCOrigin Origin,
                                          ASTImporter &Importer) {
   Origins[ToDC] = Origin;
-  Importer.ASTImporter::Imported(cast<Decl>(Origin.DC), const_cast<Decl*>(cast<Decl>(ToDC)));
+  Importer.ASTImporter::MapImported(cast<Decl>(Origin.DC), const_cast<Decl*>(cast<Decl>(ToDC)));
 }
 
 ExternalASTMerger::ExternalASTMerger(const ImporterTarget &Target,

clang/lib/Sema/SemaType.cpp

@@ -7537,6 +7537,7 @@ bool Sema::hasStructuralCompatLayout(Decl *D, Decl *Suggested) {
   // and isolate from other C++ specific checks.
   StructuralEquivalenceContext Ctx(
       D->getASTContext(), Suggested->getASTContext(), NonEquivalentDecls,
+      StructuralEquivalenceKind::Default,
       false /*StrictTypeSpelling*/, true /*Complain*/,
       true /*ErrorOnTagTypeMismatch*/);
   return Ctx.IsStructurallyEquivalent(D, Suggested);

clang/unittests/AST/ASTImporterTest.cpp

@@ -284,12 +284,15 @@ class ASTImporterTestBase : public ParameterizedTestsFixture {
   // 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;
     TU(StringRef Code, StringRef FileName, ArgVector Args)
         : Code(Code), FileName(FileName),
           Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
@@ -297,6 +300,16 @@ class ASTImporterTestBase : public ParameterizedTestsFixture {
           TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
       Unit->enableSourceFileDiagnostics();
     }
+
+    Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+      assert(ToAST);
+      if (!Importer) {
+        Importer.reset(new ASTImporter(
+            ToAST->getASTContext(), ToAST->getFileManager(),
+            Unit->getASTContext(), Unit->getFileManager(), false));
+      }
+      return Importer->Import(FromDecl);
+    }
   };
 
   // We may have several From contexts and related translation units. In each
@@ -329,14 +342,10 @@ public:
     ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
     ToAST->enableSourceFileDiagnostics();
 
-    ASTContext &FromCtx = FromTU.Unit->getASTContext(),
-               &ToCtx = ToAST->getASTContext();
+    ASTContext &FromCtx = FromTU.Unit->getASTContext();
 
     createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
 
-    ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
-                         FromTU.Unit->getFileManager(), false);
-
     IdentifierInfo *ImportedII = &FromCtx.Idents.get(Identifier);
     assert(ImportedII && "Declaration with the given identifier "
                          "should be specified in test!");
@@ -347,7 +356,8 @@ public:
 
     assert(FoundDecls.size() == 1);
 
-    Decl *Imported = Importer.Import(FoundDecls.front());
+    Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+
     assert(Imported);
     return std::make_tuple(*FoundDecls.begin(), Imported);
   }
@@ -401,11 +411,7 @@ public:
     assert(It != FromTUs.end());
     createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
 
-    ASTContext &FromCtx = From->getASTContext(),
-               &ToCtx = ToAST->getASTContext();
-    ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
-                         FromCtx.getSourceManager().getFileManager(), false);
-    return Importer.Import(From);
+    return It->import(ToAST.get(), From);
   }
 
   ~ASTImporterTestBase() {
@@ -1089,8 +1095,7 @@ TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfClassTemplateDecl) {
   EXPECT_EQ(ToTemplated1, ToTemplated);
 }
 
-TEST_P(ASTImporterTestBase,
-       DISABLED_ImportOfTemplatedDeclOfFunctionTemplateDecl) {
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfFunctionTemplateDecl) {
   Decl *FromTU = getTuDecl("template<class X> void f(){}", Lang_CXX);
   auto From = FirstDeclMatcher<FunctionTemplateDecl>().match(
       FromTU, functionTemplateDecl());
@@ -1166,7 +1171,7 @@ TEST_P(ASTImporterTestBase, ImportCorrectTemplatedDecl) {
   ASSERT_EQ(ToTemplated1, ToTemplated);
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_ImportFunctionWithBackReferringParameter) {
+TEST_P(ASTImporterTestBase, ImportFunctionWithBackReferringParameter) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
       R"(
@@ -1711,6 +1716,49 @@ TEST_P(ASTImporterTestBase, ObjectsWithUnnamedStructType) {
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag) {
+  auto Pattern = varDecl(hasName("x"));
+  VarDecl *Imported1;
+  {
+    Decl *FromTU = getTuDecl("extern int x;", Lang_CXX, "input0.cc");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+    Imported1 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  }
+  VarDecl *Imported2;
+  {
+    Decl *FromTU = getTuDecl("int x;", Lang_CXX, "input1.cc");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+    Imported2 = cast<VarDecl>(Import(FromD, Lang_CXX));
+  }
+  EXPECT_EQ(Imported1->getCanonicalDecl(), Imported2->getCanonicalDecl());
+  EXPECT_FALSE(Imported2->isUsed(false));
+  {
+    Decl *FromTU =
+        getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
+    auto *FromD =
+        FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+    Import(FromD, Lang_CXX);
+  }
+  EXPECT_TRUE(Imported2->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
+  auto Pattern = varDecl(hasName("x"));
+
+  Decl *FromTU = getTuDecl("int x;", Lang_CXX, "input0.cc");
+  auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+
+  auto *Imported1 = cast<VarDecl>(Import(FromD, Lang_CXX));
+
+  ASSERT_FALSE(Imported1->isUsed(false));
+
+  FromD->setIsUsed();
+  auto *Imported2 = cast<VarDecl>(Import(FromD, Lang_CXX));
+
+  EXPECT_EQ(Imported1, Imported2);
+  EXPECT_TRUE(Imported2->isUsed(false));
+}
+
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
@@ -2043,14 +2091,10 @@ TEST_P(ImportFriendFunctions,
   EXPECT_EQ(ToFD->getPreviousDecl(), ImportedD);
 }
 
-// This test is disabled, because ATM we create a redundant FunctionDecl.  We
-// start the import with the definition of `f` then we continue with the import
-// of the type of `f` which involves `X`. During the import of `X` we start
-// again the import of the definition of `f` and then finally we create the
-// node. But then in the first frame of `VisitFunctionDecl` we create a node
-// again since we do not check if such a node exists yet or not. This is being
-// fixed in a separate patch: https://reviews.llvm.org/D47632
-// FIXME enable this test once the above patch is approved.
+// Disabled temporarily, because the new structural equivalence check
+// (https://reviews.llvm.org/D48628) breaks it.
+// PreviousDecl is not set because there is no structural match.
+// FIXME Enable!
 TEST_P(ImportFriendFunctions,
     DISABLED_ImportFriendFunctionRedeclChainDefWithClass) {
   auto Pattern = functionDecl(hasName("f"));
@@ -2080,16 +2124,12 @@ TEST_P(ImportFriendFunctions,
             (*ImportedD->param_begin())->getOriginalType());
 }
 
-// This test is disabled, because ATM we create a redundant FunctionDecl.  We
-// start the import with the definition of `f` then we continue with the import
-// of the type of `f` which involves `X`. During the import of `X` we start
-// again the import of the definition of `f` and then finally we create the
-// node. But then in the first frame of `VisitFunctionDecl` we create a node
-// again since we do not check if such a node exists yet or not. This is being
-// fixed in a separate patch: https://reviews.llvm.org/D47632
-// FIXME enable this test once the above patch is approved.
+// Disabled temporarily, because the new structural equivalence check
+// (https://reviews.llvm.org/D48628) breaks it.
+// PreviousDecl is not set because there is no structural match.
+// FIXME Enable!
 TEST_P(ImportFriendFunctions,
-       DISABLED_ImportFriendFunctionRedeclChainDefWithClass_ImportTheProto) {
+    DISABLED_ImportFriendFunctionRedeclChainDefWithClass_ImportTheProto) {
   auto Pattern = functionDecl(hasName("f"));
 
   Decl *FromTU = getTuDecl(

clang/unittests/AST/StructuralEquivalenceTest.cpp

@@ -64,8 +64,9 @@ struct StructuralEquivalenceTest : ::testing::Test {
 
   bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
     llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
-    StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
-                                     NonEquivalentDecls, false, false);
+    StructuralEquivalenceContext Ctx(
+        D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
+        StructuralEquivalenceKind::Default, false, false);
     return Ctx.IsStructurallyEquivalent(D0, D1);
   }