2 files changed, 62 insertions, 20 deletions

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 9e801b5d7ae..e5d7ef244fb 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -890,10 +890,59 @@ void ASTContext::mergeDefinitionIntoModule(NamedDecl *ND, Module *M,
     if (auto *Listener = getASTMutationListener())
       Listener->RedefinedHiddenDefinition(ND, M);
 
-  if (getLangOpts().ModulesLocalVisibility)
-    MergedDefModules[ND].push_back(M);
-  else
+  if (getLangOpts().ModulesLocalVisibility) {
+    auto *Merged = &MergedDefModules[ND];
+    if (auto *CanonDef = Merged->CanonicalDef)
+      Merged = &MergedDefModules[CanonDef];
+    Merged->MergedModules.push_back(M);
+  } else {
+    auto MergedIt = MergedDefModules.find(ND);
+    if (MergedIt != MergedDefModules.end() && MergedIt->second.CanonicalDef)
+      ND = MergedIt->second.CanonicalDef;
     ND->setHidden(false);
+  }
+}
+
+void ASTContext::mergeDefinitionIntoModulesOf(NamedDecl *Def,
+                                              NamedDecl *Other) {
+  // We need to know the owning module of the merge source.
+  assert(Other->isFromASTFile() && "merge of non-imported decl not supported");
+  assert(Def != Other && "merging definition into itself");
+
+  if (!getLangOpts().ModulesLocalVisibility && !Other->isHidden()) {
+    Def->setHidden(false);
+    return;
+  }
+  assert(Other->getImportedOwningModule() &&
+         "hidden, imported declaration has no owning module");
+
+  // Mark Def as the canonical definition of merged definition Other.
+  {
+    auto &OtherMerged = MergedDefModules[Other];
+    assert((!OtherMerged.CanonicalDef || OtherMerged.CanonicalDef == Def) &&
+           "mismatched canonical definitions for declaration");
+    OtherMerged.CanonicalDef = Def;
+  }
+
+  auto &Merged = MergedDefModules[Def];
+  // Grab this again, we potentially just invalidated our reference.
+  auto &OtherMerged = MergedDefModules[Other];
+
+  if (Module *M = Other->getImportedOwningModule())
+    Merged.MergedModules.push_back(M);
+
+  // If this definition had any others merged into it, they're now merged into
+  // the canonical definition instead.
+  if (!OtherMerged.MergedModules.empty()) {
+    assert(!Merged.CanonicalDef && "canonical definition not canonical");
+    if (Merged.MergedModules.empty())
+      Merged.MergedModules = std::move(OtherMerged.MergedModules);
+    else
+      Merged.MergedModules.insert(Merged.MergedModules.end(),
+                                  OtherMerged.MergedModules.begin(),
+                                  OtherMerged.MergedModules.end());
+    OtherMerged.MergedModules.clear();
+  }
 }
 
 void ASTContext::deduplicateMergedDefinitonsFor(NamedDecl *ND) {
@@ -901,7 +950,13 @@ void ASTContext::deduplicateMergedDefinitonsFor(NamedDecl *ND) {
   if (It == MergedDefModules.end())
     return;
 
-  auto &Merged = It->second;
+  if (auto *CanonDef = It->second.CanonicalDef) {
+    It = MergedDefModules.find(CanonDef);
+    if (It == MergedDefModules.end())
+      return;
+  }
+
+  auto &Merged = It->second.MergedModules;
   llvm::DenseSet<Module*> Found;
   for (Module *&M : Merged)
     if (!Found.insert(M).second)
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 9b51f3dae52..a46d2d21d2f 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3474,23 +3474,10 @@ void ASTReader::makeModuleVisible(Module *Mod,
 /// visible.
 void ASTReader::mergeDefinitionVisibility(NamedDecl *Def,
                                           NamedDecl *MergedDef) {
-  // FIXME: This doesn't correctly handle the case where MergedDef is visible
-  // in modules other than its owning module. We should instead give the
-  // ASTContext a list of merged definitions for Def.
   if (Def->isHidden()) {
     // If MergedDef is visible or becomes visible, make the definition visible.
-    if (!MergedDef->isHidden())
-      Def->Hidden = false;
-    else if (getContext().getLangOpts().ModulesLocalVisibility) {
-      getContext().mergeDefinitionIntoModule(
-          Def, MergedDef->getImportedOwningModule(),
-          /*NotifyListeners*/ false);
-      PendingMergedDefinitionsToDeduplicate.insert(Def);
-    } else {
-      auto SubmoduleID = MergedDef->getOwningModuleID();
-      assert(SubmoduleID && "hidden definition in no module");
-      HiddenNamesMap[getSubmodule(SubmoduleID)].push_back(Def);
-    }
+    getContext().mergeDefinitionIntoModulesOf(Def, MergedDef);
+    PendingMergedDefinitionsToDeduplicate.insert(Def);
   }
 }
 
@@ -8621,7 +8608,7 @@ void ASTReader::finishPendingActions() {
       const FunctionDecl *Defn = nullptr;
       if (!getContext().getLangOpts().Modules || !FD->hasBody(Defn))
         FD->setLazyBody(PB->second);
-      else
+      else if (FD != Defn)
         mergeDefinitionVisibility(const_cast<FunctionDecl*>(Defn), FD);
       continue;
     }