commit b50df911788541fb5e17fe7cf3ce25330101a53a
author Richard Smith <richard-llvm@metafoo.co.uk> Thu Oct 13 23:04:14 2016 +0000
committer Richard Smith <richard-llvm@metafoo.co.uk> Thu Oct 13 23:04:14 2016 +0000
tree 5dfbd3ed0b7015b114563b7e54ae820b75cbfd36
parent 7705c4f1be36d8c3384f71b71fdd6751a49d7a70

Reinstate r281429, reverted in r281452, with a fix for its mishandling of
compiles without -fmodules-local-submodule-visibility. Original commit message:

[modules] When merging one definition into another, propagate the list of
re-exporting modules from the discarded definition to the retained definition.

llvm-svn: 284176

clang/lib/AST/ASTContext.cpp

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index e2fe6d25..e1be749 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -888,18 +888,72 @@
     if (auto *Listener = getASTMutationListener())
       Listener->RedefinedHiddenDefinition(ND, M);
 
-  if (getLangOpts().ModulesLocalVisibility)
-    MergedDefModules[ND].push_back(M);
-  else
+  auto *Merged = &MergedDefModules[ND];
+  if (auto *CanonDef = Merged->CanonicalDef) {
+    ND = CanonDef;
+    Merged = &MergedDefModules[ND];
+  }
+  assert(!Merged->CanonicalDef && "canonical def not canonical");
+
+  Merged->MergedModules.push_back(M);
+
+  if (!getLangOpts().ModulesLocalVisibility)
     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);
+  else
+    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) {
   auto It = MergedDefModules.find(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)

clang/lib/Serialization/ASTReader.cpp

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 8ce118c..ac136e95 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3481,23 +3481,10 @@
 /// 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);
   }
 }
 
@@ -8632,7 +8619,7 @@
       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;
     }