[modules] Sometimes we can deserialize a class member but not have yet

encountered any definition for the class; this happens when the definition is
added by an update record that is not yet loaded. In such a case, eagerly pick
the original parent of the member as the canonical definition of the class
rather than muddling through with the canonical declaration (the latter can
lead to us failing to merge properly later if the canonical definition turns
out to be some other declaration).

llvm-svn: 226977

clang/include/clang/Serialization/ASTReader.h

@@ -435,6 +435,10 @@ private:
   llvm::SmallVector<std::pair<serialization::GlobalDeclID, Decl*>, 16>
       PendingUpdateRecords;
 
+  /// \brief The DefinitionData pointers that we faked up for class definitions
+  /// that we needed but hadn't loaded yet.
+  llvm::SmallPtrSet<void*, 4> PendingFakeDefinitionData;
+
   struct ReplacedDeclInfo {
     ModuleFile *Mod;
     uint64_t Offset;

clang/lib/AST/ASTDumper.cpp

@@ -1099,10 +1099,13 @@ void ASTDumper::VisitFunctionDecl(const FunctionDecl *D) {
        E = D->getDeclsInPrototypeScope().end(); I != E; ++I)
     dumpDecl(*I);
 
-  for (FunctionDecl::param_const_iterator I = D->param_begin(),
-                                          E = D->param_end();
-       I != E; ++I)
-    dumpDecl(*I);
+  if (!D->param_begin() && D->getNumParams())
+    dumpChild([=] { OS << "<<NULL params x " << D->getNumParams() << ">>"; });
+  else
+    for (FunctionDecl::param_const_iterator I = D->param_begin(),
+                                            E = D->param_end();
+         I != E; ++I)
+      dumpDecl(*I);
 
   if (const CXXConstructorDecl *C = dyn_cast<CXXConstructorDecl>(D))
     for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),

clang/lib/Serialization/ASTReader.cpp

@@ -8298,7 +8298,12 @@ void ASTReader::finishPendingActions() {
       loadDeclUpdateRecords(Update.first, Update.second);
     }
   }
-  
+
+  // At this point, all update records for loaded decls are in place, so any
+  // fake class definitions should have become real.
+  assert(PendingFakeDefinitionData.empty() &&
+         "faked up a class definition but never saw the real one");
+
   // If we deserialized any C++ or Objective-C class definitions, any
   // Objective-C protocol definitions, or any redeclarable templates, make sure
   // that all redeclarations point to the definitions. Note that this can only 

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -107,7 +107,7 @@ namespace clang {
     void ReadCXXDefinitionData(struct CXXRecordDecl::DefinitionData &Data,
                                const RecordData &R, unsigned &I);
     void MergeDefinitionData(CXXRecordDecl *D,
-                             struct CXXRecordDecl::DefinitionData &NewDD);
+                             struct CXXRecordDecl::DefinitionData &&NewDD);
 
     static NamedDecl *getAnonymousDeclForMerging(ASTReader &Reader,
                                                  DeclContext *DC,
@@ -205,6 +205,8 @@ namespace clang {
       operator T*() const { return dyn_cast_or_null<T>(Existing); }
     };
 
+    static DeclContext *getPrimaryContextForMerging(ASTReader &Reader,
+                                                    DeclContext *DC);
     FindExistingResult findExisting(NamedDecl *D);
 
   public:
@@ -1353,11 +1355,25 @@ void ASTDeclReader::ReadCXXDefinitionData(
 }
 
 void ASTDeclReader::MergeDefinitionData(
-    CXXRecordDecl *D, struct CXXRecordDecl::DefinitionData &MergeDD) {
+    CXXRecordDecl *D, struct CXXRecordDecl::DefinitionData &&MergeDD) {
   assert(D->DefinitionData.getNotUpdated() &&
          "merging class definition into non-definition");
   auto &DD = *D->DefinitionData.getNotUpdated();
 
+  if (Reader.PendingFakeDefinitionData.count(&DD)) {
+    // 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?");
+
+    // Don't change which declaration is the definition; that is required
+    // to be invariant once we select it.
+    auto *Def = DD.Definition;
+    DD = std::move(MergeDD);
+    DD.Definition = Def;
+    return;
+  }
+
   // If the new definition has new special members, let the name lookup
   // code know that it needs to look in the new definition too.
   //
@@ -1460,17 +1476,18 @@ void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) {
   // We might already have a definition for this record. This can happen either
   // because we're reading an update record, or because we've already done some
   // merging. Either way, just merge into it.
-  if (auto *CanonDD = D->DefinitionData.getNotUpdated()) {
+  CXXRecordDecl *Canon = D->getCanonicalDecl();
+  if (auto *CanonDD = Canon->DefinitionData.getNotUpdated()) {
     if (CanonDD->Definition != DD->Definition)
       Reader.MergedDeclContexts.insert(
           std::make_pair(DD->Definition, CanonDD->Definition));
-    MergeDefinitionData(D, *DD);
+    MergeDefinitionData(Canon, std::move(*DD));
+    D->DefinitionData = Canon->DefinitionData;
     return;
   }
 
   // Propagate the DefinitionData pointer to the canonical declaration, so
   // that all other deserialized declarations will see it.
-  CXXRecordDecl *Canon = D->getCanonicalDecl();
   if (Canon == D) {
     D->DefinitionData = DD;
     D->IsCompleteDefinition = true;
@@ -1482,7 +1499,7 @@ void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D) {
         std::make_pair(D, CanonDD->Definition));
     D->DefinitionData = Canon->DefinitionData;
     D->IsCompleteDefinition = false;
-    MergeDefinitionData(D, *DD);
+    MergeDefinitionData(D, std::move(*DD));
   } else {
     Canon->DefinitionData = DD;
     D->DefinitionData = Canon->DefinitionData;
@@ -1827,7 +1844,7 @@ ASTDeclReader::VisitClassTemplateSpecializationDeclImpl(
         // definition.
         if (auto *DDD = D->DefinitionData.getNotUpdated()) {
           if (auto *CanonDD = CanonSpec->DefinitionData.getNotUpdated()) {
-            MergeDefinitionData(CanonSpec, *DDD);
+            MergeDefinitionData(CanonSpec, std::move(*DDD));
             Reader.PendingDefinitions.erase(D);
             Reader.MergedDeclContexts.insert(
                 std::make_pair(D, CanonDD->Definition));
@@ -2125,6 +2142,7 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D,
   auto *ExistingPattern = Existing->getTemplatedDecl();
   RedeclarableResult Result(Reader, DPattern->getCanonicalDecl()->getGlobalID(),
                             DPattern->getKind());
+
   if (auto *DClass = dyn_cast<CXXRecordDecl>(DPattern)) {
     // Merge with any existing definition.
     // FIXME: This is duplicated in several places. Refactor.
@@ -2132,7 +2150,7 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D,
         cast<CXXRecordDecl>(ExistingPattern)->getCanonicalDecl();
     if (auto *DDD = DClass->DefinitionData.getNotUpdated()) {
       if (auto *ExistingDD = ExistingClass->DefinitionData.getNotUpdated()) {
-        MergeDefinitionData(ExistingClass, *DDD);
+        MergeDefinitionData(ExistingClass, std::move(*DDD));
         Reader.PendingDefinitions.erase(DClass);
         Reader.MergedDeclContexts.insert(
             std::make_pair(DClass, ExistingDD->Definition));
@@ -2533,18 +2551,33 @@ static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
 
 /// Find the context in which we should search for previous declarations when
 /// looking for declarations to merge.
-static DeclContext *getPrimaryContextForMerging(DeclContext *DC) {
+DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
+                                                        DeclContext *DC) {
   if (NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC))
     return ND->getOriginalNamespace();
 
-  // There is one tricky case here: if DC is a class with no definition, then
-  // we're merging a declaration whose definition is added by an update record,
-  // but we've not yet loaded that update record. In this case, we use the
-  // canonical declaration for merging until we get a real definition.
-  // FIXME: When we add a definition, we may need to move the partial lookup
-  // information from the canonical declaration onto the chosen definition.
-  if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC))
-    return RD->getPrimaryContext();
+  if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
+    // Try to dig out the definition.
+    auto *DD = RD->DefinitionData.getNotUpdated();
+    if (!DD)
+      DD = RD->getCanonicalDecl()->DefinitionData.getNotUpdated();
+
+    // If there's no definition yet, then DC's definition is added by an update
+    // record, but we've not yet loaded that update record. In this case, we
+    // commit to DC being the canonical definition now, and will fix this when
+    // we load the update record.
+    if (!DD) {
+      DD = new (Reader.Context) struct CXXRecordDecl::DefinitionData(RD);
+      RD->IsCompleteDefinition = true;
+      RD->DefinitionData = DD;
+      RD->getCanonicalDecl()->DefinitionData = DD;
+
+      // Track that we did this horrible thing so that we can fix it later.
+      Reader.PendingFakeDefinitionData.insert(DD);
+    }
+
+    return DD->Definition;
+  }
 
   if (EnumDecl *ED = dyn_cast<EnumDecl>(DC))
     return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
@@ -2574,7 +2607,7 @@ ASTDeclReader::FindExistingResult::~FindExistingResult() {
                                AnonymousDeclNumber, New);
   } else if (DC->isTranslationUnit() && Reader.SemaObj) {
     Reader.SemaObj->IdResolver.tryAddTopLevelDecl(New, Name);
-  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) {
+  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) {
     // Add the declaration to its redeclaration context so later merging
     // lookups will find it.
     MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
@@ -2720,7 +2753,7 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
           return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
                                     TypedefNameForLinkage);
     }
-  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) {
+  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) {
     DeclContext::lookup_result R = MergeDC->noload_lookup(Name);
     for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
       if (NamedDecl *Existing = getDeclForMerging(*I, TypedefNameForLinkage))
@@ -3605,11 +3638,13 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile,
 
     case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: {
       auto *RD = cast<CXXRecordDecl>(D);
-      bool HadDefinition = RD->getDefinition();
+      bool HadRealDefinition = RD->getDefinition() &&
+                               !Reader.PendingFakeDefinitionData.count(
+                                   RD->DefinitionData.getNotUpdated());
       ReadCXXRecordDefinition(RD);
       // Visible update is handled separately.
       uint64_t LexicalOffset = Record[Idx++];
-      if (!HadDefinition && LexicalOffset) {
+      if (!HadRealDefinition && LexicalOffset) {
         RD->setHasExternalLexicalStorage(true);
         Reader.ReadDeclContextStorage(ModuleFile, ModuleFile.DeclsCursor,
                                           std::make_pair(LexicalOffset, 0),

clang/test/Modules/Inputs/merge-nested-templates/a.h

@@ -0,0 +1 @@
+#include "string.ii"

clang/test/Modules/Inputs/merge-nested-templates/b.h

@@ -0,0 +1,2 @@
+#include "a.h"
+std::wstring::iterator j;

clang/test/Modules/Inputs/merge-nested-templates/c.h

@@ -0,0 +1,3 @@
+#include "string.ii"
+std::wstring::iterator i;
+#include "b.h"

clang/test/Modules/Inputs/merge-nested-templates/module.modulemap

@@ -0,0 +1,3 @@
+module a { header "a.h" export * }
+module b { header "b.h" export * }
+module c { header "c.h" export * }

clang/test/Modules/Inputs/merge-nested-templates/string.ii

@@ -0,0 +1,14 @@
+namespace std {
+  template <typename, typename Container> struct normal_iterator {
+    normal_iterator() {}
+
+    template <typename I>
+    normal_iterator(normal_iterator<I, typename Container::iterator>) {}
+  };
+
+  template <typename pointer> struct basic_string {
+    typedef normal_iterator<pointer, basic_string> iterator;
+  };
+
+  typedef basic_string<wchar_t *> wstring;
+}

clang/test/Modules/merge-nested-templates.cpp

@@ -0,0 +1,4 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/merge-nested-templates -verify %s
+// expected-no-diagnostics
+#include "c.h"