From b50df911788541fb5e17fe7cf3ce25330101a53a Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 13 Oct 2016 23:04:14 +0000 Subject: [PATCH] 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/include/clang/AST/ASTContext.h | 15 +++++- clang/lib/AST/ASTContext.cpp | 62 ++++++++++++++++++++-- clang/lib/Serialization/ASTReader.cpp | 19 ++----- .../Inputs/merge-template-pattern-visibility/a.h | 5 ++ .../Inputs/merge-template-pattern-visibility/b.h | 3 ++ .../Inputs/merge-template-pattern-visibility/c.h | 3 ++ .../Inputs/merge-template-pattern-visibility/d.h | 4 +- .../test/Modules/Inputs/merge-var-template-def/a.h | 6 +++ .../Modules/Inputs/merge-var-template-def/b1.h | 7 +++ .../Modules/Inputs/merge-var-template-def/b2.h | 6 +++ .../Inputs/merge-var-template-def/module.modulemap | 5 ++ .../Modules/merge-template-pattern-visibility.cpp | 8 +++ clang/test/Modules/merge-var-template-def.cpp | 7 +++ 13 files changed, 127 insertions(+), 23 deletions(-) create mode 100644 clang/test/Modules/Inputs/merge-var-template-def/a.h create mode 100644 clang/test/Modules/Inputs/merge-var-template-def/b1.h create mode 100644 clang/test/Modules/Inputs/merge-var-template-def/b2.h create mode 100644 clang/test/Modules/Inputs/merge-var-template-def/module.modulemap create mode 100644 clang/test/Modules/merge-var-template-def.cpp diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 3d77752..f5a9bd2 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -308,10 +308,18 @@ class ASTContext : public RefCountedBase { /// merged into. llvm::DenseMap MergedDecls; + /// The modules into which a definition has been merged, or a map from a + /// merged definition to its canonical definition. This is really a union of + /// a NamedDecl* and a vector of Module*. + struct MergedModulesOrCanonicalDef { + llvm::TinyPtrVector MergedModules; + NamedDecl *CanonicalDef = nullptr; + }; + /// \brief A mapping from a defining declaration to a list of modules (other /// than the owning module of the declaration) that contain merged /// definitions of that entity. - llvm::DenseMap> MergedDefModules; + llvm::DenseMap MergedDefModules; /// \brief Initializers for a module, in order. Each Decl will be either /// something that has a semantic effect on startup (such as a variable with @@ -883,6 +891,7 @@ public: /// and should be visible whenever \p M is visible. void mergeDefinitionIntoModule(NamedDecl *ND, Module *M, bool NotifyListeners = true); + void mergeDefinitionIntoModulesOf(NamedDecl *ND, NamedDecl *Other); /// \brief Clean up the merged definition list. Call this if you might have /// added duplicates into the list. void deduplicateMergedDefinitonsFor(NamedDecl *ND); @@ -893,7 +902,9 @@ public: auto MergedIt = MergedDefModules.find(Def); if (MergedIt == MergedDefModules.end()) return None; - return MergedIt->second; + if (auto *CanonDef = MergedIt->second.CanonicalDef) + return getModulesWithMergedDefinition(CanonDef); + return MergedIt->second.MergedModules; } /// Add a declaration to the list of declarations that are initialized 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 @@ void ASTContext::mergeDefinitionIntoModule(NamedDecl *ND, Module *M, 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 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 8ce118c..ac136e95 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3481,23 +3481,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); } } @@ -8632,7 +8619,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(Defn), FD); continue; } diff --git a/clang/test/Modules/Inputs/merge-template-pattern-visibility/a.h b/clang/test/Modules/Inputs/merge-template-pattern-visibility/a.h index e659202..b960ba6 100644 --- a/clang/test/Modules/Inputs/merge-template-pattern-visibility/a.h +++ b/clang/test/Modules/Inputs/merge-template-pattern-visibility/a.h @@ -4,3 +4,8 @@ template struct B; template struct A {}; template struct B : A {}; template inline auto C(T) {} + +namespace CrossModuleMerge { + template inline auto D(T) {} + struct E {}; +} diff --git a/clang/test/Modules/Inputs/merge-template-pattern-visibility/b.h b/clang/test/Modules/Inputs/merge-template-pattern-visibility/b.h index 41b52d5..260b6b3 100644 --- a/clang/test/Modules/Inputs/merge-template-pattern-visibility/b.h +++ b/clang/test/Modules/Inputs/merge-template-pattern-visibility/b.h @@ -17,4 +17,7 @@ namespace CrossModuleMerge { template struct A {}; template struct B : A {}; template inline auto C(T) {} + + template inline auto D(T) {} + struct E; } diff --git a/clang/test/Modules/Inputs/merge-template-pattern-visibility/c.h b/clang/test/Modules/Inputs/merge-template-pattern-visibility/c.h index db80eda..01594bb 100644 --- a/clang/test/Modules/Inputs/merge-template-pattern-visibility/c.h +++ b/clang/test/Modules/Inputs/merge-template-pattern-visibility/c.h @@ -5,5 +5,8 @@ namespace CrossModuleMerge { template struct A {}; template struct B : A {}; template inline auto C(T) {} + + template inline auto D(T) {} + struct E {}; } diff --git a/clang/test/Modules/Inputs/merge-template-pattern-visibility/d.h b/clang/test/Modules/Inputs/merge-template-pattern-visibility/d.h index 464f0f5..7bbd01e 100644 --- a/clang/test/Modules/Inputs/merge-template-pattern-visibility/d.h +++ b/clang/test/Modules/Inputs/merge-template-pattern-visibility/d.h @@ -1 +1,3 @@ -// d.h: empty +namespace CrossModuleMerge { + struct E {}; +} diff --git a/clang/test/Modules/Inputs/merge-var-template-def/a.h b/clang/test/Modules/Inputs/merge-var-template-def/a.h new file mode 100644 index 0000000..cce8875 --- /dev/null +++ b/clang/test/Modules/Inputs/merge-var-template-def/a.h @@ -0,0 +1,6 @@ +#ifndef A_H +#define A_H +template struct A { static bool b; }; +template bool A::b = false; +template void *get() { return &(A::b); } +#endif diff --git a/clang/test/Modules/Inputs/merge-var-template-def/b1.h b/clang/test/Modules/Inputs/merge-var-template-def/b1.h new file mode 100644 index 0000000..1ef8974 --- /dev/null +++ b/clang/test/Modules/Inputs/merge-var-template-def/b1.h @@ -0,0 +1,7 @@ +#ifndef B1_H +#define B1_H +template struct A { static bool b; }; +template bool A::b = false; +template void *get() { return &(A::b); } +#include "a.h" +#endif diff --git a/clang/test/Modules/Inputs/merge-var-template-def/b2.h b/clang/test/Modules/Inputs/merge-var-template-def/b2.h new file mode 100644 index 0000000..3c7e530 --- /dev/null +++ b/clang/test/Modules/Inputs/merge-var-template-def/b2.h @@ -0,0 +1,6 @@ +#ifndef B2_H +#define B2_H +template struct A { static bool b; }; +template bool A::b = false; +template void *get() { return &(A::b); } +#endif diff --git a/clang/test/Modules/Inputs/merge-var-template-def/module.modulemap b/clang/test/Modules/Inputs/merge-var-template-def/module.modulemap new file mode 100644 index 0000000..b2c96bd --- /dev/null +++ b/clang/test/Modules/Inputs/merge-var-template-def/module.modulemap @@ -0,0 +1,5 @@ +module a { header "a.h" export * } +module b { + module b1 { header "b1.h" export * } + module b2 { header "b2.h" export * } +} diff --git a/clang/test/Modules/merge-template-pattern-visibility.cpp b/clang/test/Modules/merge-template-pattern-visibility.cpp index ec5aa26..bb34d3b 100644 --- a/clang/test/Modules/merge-template-pattern-visibility.cpp +++ b/clang/test/Modules/merge-template-pattern-visibility.cpp @@ -7,6 +7,12 @@ // RUN: -fmodules-local-submodule-visibility -o %t/Y.pcm // RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14 -fmodule-file=%t/X.pcm -fmodule-file=%t/Y.pcm \ // RUN: -fmodules-local-submodule-visibility -verify %s -I%S/Inputs/merge-template-pattern-visibility +// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14 -fmodule-file=%t/Y.pcm -fmodule-file=%t/X.pcm \ +// RUN: -fmodules-local-submodule-visibility -verify %s -I%S/Inputs/merge-template-pattern-visibility +// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14 -fmodule-file=%t/X.pcm -fmodule-file=%t/Y.pcm \ +// RUN: -verify %s -I%S/Inputs/merge-template-pattern-visibility +// RUN: %clang_cc1 -fmodules -fno-modules-error-recovery -std=c++14 -fmodule-file=%t/Y.pcm -fmodule-file=%t/X.pcm \ +// RUN: -verify %s -I%S/Inputs/merge-template-pattern-visibility #include "b.h" #include "d.h" @@ -15,4 +21,6 @@ void g() { CrossModuleMerge::B bi; CrossModuleMerge::C(0); + CrossModuleMerge::D(0); + CrossModuleMerge::E e; } diff --git a/clang/test/Modules/merge-var-template-def.cpp b/clang/test/Modules/merge-var-template-def.cpp new file mode 100644 index 0000000..4d39cad --- /dev/null +++ b/clang/test/Modules/merge-var-template-def.cpp @@ -0,0 +1,7 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -I%S/Inputs/merge-var-template-def -verify -fmodules -Werror=undefined-internal -fmodules-local-submodule-visibility -fmodules-cache-path=%t -fimplicit-module-maps %s +// expected-no-diagnostics + +#include "b2.h" +namespace { struct X; } +void *x = get(); -- 2.7.4