[modules] Be sure to load the lexical definition of a class template

specialization from an update record exactly once, even if we needed to fake up
the definition.

llvm-svn: 227939

clang/include/clang/Serialization/ASTReader.h

@@ -435,9 +435,11 @@ private:
   llvm::SmallVector<std::pair<serialization::GlobalDeclID, Decl*>, 16>
       PendingUpdateRecords;
 
+  enum class PendingFakeDefinitionKind { NotFake, Fake, FakeLoaded };
+
   /// \brief The DefinitionData pointers that we faked up for class definitions
   /// that we needed but hadn't loaded yet.
-  llvm::SmallPtrSet<void*, 4> PendingFakeDefinitionData;
+  llvm::DenseMap<void *, PendingFakeDefinitionKind> PendingFakeDefinitionData;
 
   struct ReplacedDeclInfo {
     ModuleFile *Mod;

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -103,7 +103,7 @@ namespace clang {
       return Reader.getSubmodule(readSubmoduleID(R, I));
     }
 
-    void ReadCXXRecordDefinition(CXXRecordDecl *D);
+    void ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update);
     void ReadCXXDefinitionData(struct CXXRecordDecl::DefinitionData &Data,
                                const RecordData &R, unsigned &I);
     void MergeDefinitionData(CXXRecordDecl *D,
@@ -1360,11 +1360,13 @@ void ASTDeclReader::MergeDefinitionData(
          "merging class definition into non-definition");
   auto &DD = *D->DefinitionData.getNotUpdated();
 
-  if (Reader.PendingFakeDefinitionData.count(&DD)) {
+  auto PFDI = Reader.PendingFakeDefinitionData.find(&DD);
+  if (PFDI != Reader.PendingFakeDefinitionData.end() &&
+      PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
     // We faked up this definition data because we found a class for which we'd
     // not yet loaded the definition. Replace it with the real thing now.
-    Reader.PendingFakeDefinitionData.erase(&DD);
     assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
+    PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
 
     // Don't change which declaration is the definition; that is required
     // to be invariant once we select it.
@@ -1458,7 +1460,7 @@ void ASTDeclReader::MergeDefinitionData(
     Reader.PendingOdrMergeFailures[DD.Definition].push_back(MergeDD.Definition);
 }
 
-void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) {
+void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) {
   struct CXXRecordDecl::DefinitionData *DD;
   ASTContext &C = Reader.getContext();
 
@@ -1491,6 +1493,11 @@ void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) {
   if (Canon == D) {
     D->DefinitionData = DD;
     D->IsCompleteDefinition = true;
+
+    // If this is an update record, we can have redeclarations already. Make a
+    // note that we need to propagate the DefinitionData pointer onto them.
+    if (Update)
+      Reader.PendingDefinitions.insert(D);
   } else if (auto *CanonDD = Canon->DefinitionData.getNotUpdated()) {
     // We have already deserialized a definition of this record. This
     // definition is no longer really a definition. Note that the pre-existing
@@ -1556,7 +1563,7 @@ ASTDeclReader::VisitCXXRecordDeclImpl(CXXRecordDecl *D) {
 
   bool WasDefinition = Record[Idx++];
   if (WasDefinition)
-    ReadCXXRecordDefinition(D);
+    ReadCXXRecordDefinition(D, /*Update*/false);
   else
     // Propagate DefinitionData pointer from the canonical declaration.
     D->DefinitionData = D->getCanonicalDecl()->DefinitionData;
@@ -2573,7 +2580,8 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
       RD->getCanonicalDecl()->DefinitionData = DD;
 
       // Track that we did this horrible thing so that we can fix it later.
-      Reader.PendingFakeDefinitionData.insert(DD);
+      Reader.PendingFakeDefinitionData.insert(
+          std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
     }
 
     return DD->Definition;
@@ -3638,18 +3646,19 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile,
 
     case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: {
       auto *RD = cast<CXXRecordDecl>(D);
-      bool HadRealDefinition = RD->getDefinition() &&
-                               !Reader.PendingFakeDefinitionData.count(
-                                   RD->DefinitionData.getNotUpdated());
-      ReadCXXRecordDefinition(RD);
+      auto *OldDD = RD->DefinitionData.getNotUpdated();
+      bool HadRealDefinition =
+          OldDD && !Reader.PendingFakeDefinitionData.count(OldDD);
+      ReadCXXRecordDefinition(RD, /*Update*/true);
+
       // Visible update is handled separately.
       uint64_t LexicalOffset = Record[Idx++];
       if (!HadRealDefinition && LexicalOffset) {
         RD->setHasExternalLexicalStorage(true);
         Reader.ReadDeclContextStorage(ModuleFile, ModuleFile.DeclsCursor,
-                                          std::make_pair(LexicalOffset, 0),
-                                          ModuleFile.DeclContextInfos[RD]);
-        Reader.PendingDefinitions.insert(RD);
+                                      std::make_pair(LexicalOffset, 0),
+                                      ModuleFile.DeclContextInfos[RD]);
+        Reader.PendingFakeDefinitionData.erase(OldDD);
       }
 
       auto TSK = (TemplateSpecializationKind)Record[Idx++];

clang/test/Modules/Inputs/merge-template-members/a.h

@@ -0,0 +1,9 @@
+namespace N {
+  template <typename> struct A {
+    int n;
+    A() : n() {}
+  };
+
+  // Create declaration of A<int>.
+  typedef A<int> AI;
+}

clang/test/Modules/Inputs/merge-template-members/b.h

@@ -0,0 +1,6 @@
+#include "a.h"
+
+// Add update record for definition of A<int> and constructors.
+// We need an eagerly-emitted function here to get the problematic
+// deserialization ordering.
+void foobar() { N::A<int> x; }

clang/test/Modules/Inputs/merge-template-members/c.h

@@ -0,0 +1,14 @@
+namespace N {
+  template <typename> struct A {
+    int n;
+    A() : n() {}
+  };
+
+  // Trigger instantiation of definition of A<int>.
+  struct C {
+    A<int> a;
+  };
+}
+
+// Merge in another declaration and update records.
+#include "b.h"

clang/test/Modules/Inputs/merge-template-members/module.modulemap

@@ -1,2 +1,6 @@
 module def { header "def.h" export * }
 module update { header "update.h" export * }
+
+module a { header "a.h" export * }
+module b { header "b.h" export * }
+module c { header "c.h" export * }

clang/test/Modules/merge-template-members.cpp

@@ -1,7 +1,10 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/merge-template-members -verify %s
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/merge-template-members -verify -emit-llvm-only %s
 // expected-no-diagnostics
 
+#include "c.h"
+N::A<int> ai;
+
 template<typename> struct A { int n; };
 template<typename> struct B { typedef A<void> C; };
 template class B<int>;