[modules] If a module declares an entity and then imports another declaration

of that entity, ensure that the redeclaration chain is reordered properly on
reload. Otherwise, the result of name lookup for that entity may point to an
entity that is too old; if that's an injected friend name or the like, that
can result in the name not being found at all.

llvm-svn: 228371

diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 5650607..970ec84 100644 (file)
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -124,6 +124,7 @@ namespace clang {
     class RedeclarableResult {
       ASTReader &Reader;
       GlobalDeclID FirstID;
+      Decl *MergeWith;
       mutable bool Owning;
       Decl::Kind DeclKind;
       
@@ -131,13 +132,14 @@ namespace clang {
       
     public:
       RedeclarableResult(ASTReader &Reader, GlobalDeclID FirstID,
-                         Decl::Kind DeclKind)
-        : Reader(Reader), FirstID(FirstID), Owning(true), DeclKind(DeclKind) { }
+                         Decl *MergeWith, Decl::Kind DeclKind)
+        : Reader(Reader), FirstID(FirstID), MergeWith(MergeWith),
+          Owning(true), DeclKind(DeclKind) {}
 
       RedeclarableResult(const RedeclarableResult &Other)
-        : Reader(Other.Reader), FirstID(Other.FirstID), Owning(Other.Owning) ,
-          DeclKind(Other.DeclKind)
-      { 
+        : Reader(Other.Reader), FirstID(Other.FirstID),
+          MergeWith(Other.MergeWith), Owning(Other.Owning),
+          DeclKind(Other.DeclKind) {
         Other.Owning = false;
       }
 
@@ -149,7 +151,11 @@ namespace clang {
       
       /// \brief Retrieve the first ID.
       GlobalDeclID getFirstID() const { return FirstID; }
-      
+
+      /// \brief Get a known declaration that this should be merged with, if
+      /// any.
+      Decl *getKnownMergeTarget() const { return MergeWith; }
+
       /// \brief Do not introduce this declaration ID into the set of pending
       /// declaration chains.
       void suppress() {
@@ -2077,15 +2083,23 @@ ASTDeclReader::VisitDeclContext(DeclContext *DC) {
 }
 
 template <typename T>
-ASTDeclReader::RedeclarableResult 
+ASTDeclReader::RedeclarableResult
 ASTDeclReader::VisitRedeclarable(Redeclarable<T> *D) {
   DeclID FirstDeclID = ReadDeclID(Record, Idx);
-  
+  Decl *MergeWith = nullptr;
+
   // 0 indicates that this declaration was the only declaration of its entity,
   // and is used for space optimization.
   if (FirstDeclID == 0)
     FirstDeclID = ThisDeclID;
-  
+  else if (Record[Idx++]) {
+    // We need to merge with FirstDeclID. Read it now to ensure that it is
+    // before us in the redecl chain, then forget we saw it so that we will
+    // merge with it.
+    MergeWith = Reader.GetDecl(FirstDeclID);
+    FirstDeclID = ThisDeclID;
+  }
+
   T *FirstDecl = cast_or_null<T>(Reader.GetDecl(FirstDeclID));
   if (FirstDecl != D) {
     // We delay loading of the redeclaration chain to avoid deeply nested calls.
@@ -2100,7 +2114,7 @@ ASTDeclReader::VisitRedeclarable(Redeclarable<T> *D) {
                              
   // The result structure takes care to note that we need to load the 
   // other declaration chains for this ID.
-  return RedeclarableResult(Reader, FirstDeclID,
+  return RedeclarableResult(Reader, FirstDeclID, MergeWith,
                             static_cast<T *>(D)->getKind());
 }
 
@@ -2127,7 +2141,10 @@ void ASTDeclReader::mergeRedeclarable(Redeclarable<T> *DBase,
   if (!Reader.getContext().getLangOpts().Modules)
     return;
 
-  if (FindExistingResult ExistingRes = findExisting(D))
+  if (auto *Existing = Redecl.getKnownMergeTarget())
+    // We already know of an existing declaration we should merge with.
+    mergeRedeclarable(D, cast<T>(Existing), Redecl, TemplatePatternID);
+  else if (FindExistingResult ExistingRes = findExisting(D))
     if (T *Existing = ExistingRes)
       mergeRedeclarable(D, Existing, Redecl, TemplatePatternID);
 }
@@ -2148,7 +2165,7 @@ void ASTDeclReader::mergeTemplatePattern(RedeclarableTemplateDecl *D,
   auto *DPattern = D->getTemplatedDecl();
   auto *ExistingPattern = Existing->getTemplatedDecl();
   RedeclarableResult Result(Reader, DPattern->getCanonicalDecl()->getGlobalID(),
-                            DPattern->getKind());
+                            /*MergeWith*/ExistingPattern, DPattern->getKind());
 
   if (auto *DClass = dyn_cast<CXXRecordDecl>(DPattern)) {
     // Merge with any existing definition.

diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 4017ec6..6b37659 100644 (file)
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1448,25 +1448,42 @@ void ASTDeclWriter::VisitDeclContext(DeclContext *DC, uint64_t LexicalOffset,
 template <typename T>
 void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) {
   T *First = D->getFirstDecl();
-  if (First->getMostRecentDecl() != First) {
+  T *MostRecent = First->getMostRecentDecl();
+  if (MostRecent != First) {
     assert(isRedeclarableDeclKind(static_cast<T *>(D)->getKind()) &&
            "Not considered redeclarable?");
-    
+
+    auto *Previous = D->getPreviousDecl();
+    auto *FirstToEmit = First;
+    if (Context.getLangOpts().Modules && Writer.Chain && !Previous) {
+      // In a modules build, we can have imported declarations after a local
+      // canonical declaration. If we do, we want to treat the first imported
+      // declaration as our canonical declaration on reload, in order to
+      // rebuild the redecl chain in the right order.
+      for (auto *Redecl = MostRecent; Redecl;
+           Redecl = Redecl->getPreviousDecl())
+        if (Redecl->isFromASTFile())
+          FirstToEmit = Redecl;
+    }
+
     // There is more than one declaration of this entity, so we will need to
     // write a redeclaration chain.
-    Writer.AddDeclRef(First, Record);
+    Writer.AddDeclRef(FirstToEmit, Record);
+    Record.push_back(FirstToEmit != First);
     Writer.Redeclarations.insert(First);
 
     // Make sure that we serialize both the previous and the most-recent 
     // declarations, which (transitively) ensures that all declarations in the
     // chain get serialized.
-    (void)Writer.GetDeclRef(D->getPreviousDecl());
-    (void)Writer.GetDeclRef(First->getMostRecentDecl());
+    //
+    // FIXME: This is not correct; when we reach an imported declaration we
+    // won't emit its previous declaration.
+    (void)Writer.GetDeclRef(Previous);
+    (void)Writer.GetDeclRef(MostRecent);
   } else {
     // We use the sentinel value 0 to indicate an only declaration.
     Record.push_back(0);
   }
-  
 }
 
 void ASTDeclWriter::VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *D) {
@@ -1948,9 +1965,10 @@ void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
 
   // Determine the ID for this declaration.
   serialization::DeclID ID;
-  if (D->isFromASTFile())
+  if (D->isFromASTFile()) {
+    assert(isRewritten(D) && "should not be emitting imported decl");
     ID = getDeclID(D);
-  else {
+  } else {
     serialization::DeclID &IDR = DeclIDs[D];
     if (IDR == 0)
       IDR = NextDeclID++;

diff --git a/clang/test/Modules/Inputs/merge-friends/decl.h b/clang/test/Modules/Inputs/merge-friends/decl.h
new file mode 100644 (file)
index 0000000..f658f94
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-friends/decl.h
@@ -0,0 +1 @@
+namespace N { struct foo; }

diff --git a/clang/test/Modules/Inputs/merge-friends/friend.h b/clang/test/Modules/Inputs/merge-friends/friend.h
new file mode 100644 (file)
index 0000000..bbbd8ed
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-friends/friend.h
@@ -0,0 +1,2 @@
+namespace N { struct n8 { friend struct foo; }; }
+#include "decl.h"

diff --git a/clang/test/Modules/Inputs/merge-friends/module.modulemap b/clang/test/Modules/Inputs/merge-friends/module.modulemap
new file mode 100644 (file)
index 0000000..1fa52c9
--- /dev/null
+++ b/clang/test/Modules/Inputs/merge-friends/module.modulemap
@@ -0,0 +1,2 @@
+module decl { header "decl.h" export * }
+module friend { header "friend.h" export * }

diff --git a/clang/test/Modules/cxx-templates.cpp b/clang/test/Modules/cxx-templates.cpp
index d9c8a8c..cbe2b65 100644 (file)
--- a/clang/test/Modules/cxx-templates.cpp
+++ b/clang/test/Modules/cxx-templates.cpp
@@ -28,14 +28,8 @@ void g() {
   f<double>(1.0);
   f<int>();
   f(); // expected-error {{no matching function}}
-#ifdef EARLY_IMPORT
-  // FIXME: The textual inclusion above shouldn't affect this!
-  // expected-note@Inputs/cxx-templates-a.h:3 {{couldn't infer template argument}}
-  // expected-note@Inputs/cxx-templates-a.h:4 {{requires 1 argument}}
-#else
   // expected-note@Inputs/cxx-templates-b.h:3 {{couldn't infer template argument}}
   // expected-note@Inputs/cxx-templates-b.h:4 {{requires single argument}}
-#endif
 
   N::f(0);
   N::f<double>(1.0);

diff --git a/clang/test/Modules/merge-friends.cpp b/clang/test/Modules/merge-friends.cpp
new file mode 100644 (file)
index 0000000..8284bfe
--- /dev/null
+++ b/clang/test/Modules/merge-friends.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/merge-friends -verify %s
+// expected-no-diagnostics
+#include "friend.h"
+N::foo *use;