[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

diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 91ad34bd1ca14bdea669f14e6e1f8226ba59673a..8dd9b145c05d4de61303b92b79b3f8423bb3f25f 100644 (file)
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/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;

diff --git a/clang/lib/AST/ASTDumper.cpp b/clang/lib/AST/ASTDumper.cpp
index ebf5e651ef9ae2dc25017d8067bd8571d47566d5..edd836041fc24d3db638947a6702a256c45d965b 100644 (file)
--- a/clang/lib/AST/ASTDumper.cpp
+++ b/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(),

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 416164ebb7d244bed03906caede3bb7033420a41..f7822b7476925704ecde8950c759a793cfa350a5 100644 (file)
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/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 

diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index d43f3eaacb72a9dbb2c4c6bcf57d40500d046e03..754b0a0a5359b8305b5a2bd21f720a0b7c89221b 100644 (file)
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/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),

diff --git a/clang/test/Modules/Inputs/merge-nested-templates/a.h b/clang/test/Modules/Inputs/merge-nested-templates/a.h
new file mode 100644 (file)
index 0000000..826d257
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-nested-templates/a.h
@@ -0,0 +1 @@
+#include "string.ii"

diff --git a/clang/test/Modules/Inputs/merge-nested-templates/b.h b/clang/test/Modules/Inputs/merge-nested-templates/b.h
new file mode 100644 (file)
index 0000000..516694e
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-nested-templates/b.h
@@ -0,0 +1,2 @@
+#include "a.h"
+std::wstring::iterator j;

diff --git a/clang/test/Modules/Inputs/merge-nested-templates/c.h b/clang/test/Modules/Inputs/merge-nested-templates/c.h
new file mode 100644 (file)
index 0000000..ab95e14
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-nested-templates/c.h
@@ -0,0 +1,3 @@
+#include "string.ii"
+std::wstring::iterator i;
+#include "b.h"

diff --git a/clang/test/Modules/Inputs/merge-nested-templates/module.modulemap b/clang/test/Modules/Inputs/merge-nested-templates/module.modulemap
new file mode 100644 (file)
index 0000000..77e0a89
--- /dev/null
+++ b/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 * }

diff --git a/clang/test/Modules/Inputs/merge-nested-templates/string.ii b/clang/test/Modules/Inputs/merge-nested-templates/string.ii
new file mode 100644 (file)
index 0000000..136d8e7
--- /dev/null
+++ b/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;
+}

diff --git a/clang/test/Modules/merge-nested-templates.cpp b/clang/test/Modules/merge-nested-templates.cpp
new file mode 100644 (file)
index 0000000..42764ea
--- /dev/null
+++ b/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"