commit 8310f8a88e926cd04ca9cc2ee0aa1d811f99ef44 Author: Ben Langmuir Date: Mon Nov 16 13:39:36 2015 Allow modules specified by -fmodule-map-file to shadow implicitly found ones When modules come from module map files explicitly specified by -fmodule-map-file= arguments, allow those to override/shadow modules with the same name that are found implicitly by header search. If such a module is looked up by name (e.g. @import), we will always find the one from -fmodule-map-file. If we try to use a shadowed module by including one of its headers report an error. This enables developers to force use of a specific copy of their module to be used if there are multiple copies that would otherwise be visible, for example if they develop modules that are installed in the default search paths. rdar://problem/23467372 diff --git a/include/clang/Basic/DiagnosticCommonKinds.td b/include/clang/Basic/DiagnosticCommonKinds.td index 42e761a..2f20949 100644 --- a/include/clang/Basic/DiagnosticCommonKinds.td +++ b/include/clang/Basic/DiagnosticCommonKinds.td @@ -88,6 +88,7 @@ def err_module_unavailable : Error< "module '%0' %select{is incompatible with|requires}1 feature '%2'">; def err_module_header_missing : Error< "%select{|umbrella }0header '%1' not found">; +def err_module_shadowed : Error<"import of shadowed module '%0'">; def err_module_lock_failure : Error< "could not acquire lock file for module '%0'">, DefaultFatal; def err_module_lock_timeout : Error< diff --git a/include/clang/Basic/Module.h b/include/clang/Basic/Module.h index fa032e9..087320b 100644 --- a/include/clang/Basic/Module.h +++ b/include/clang/Basic/Module.h @@ -149,6 +149,9 @@ public: /// will be false to indicate that this (sub)module is not available. SmallVector Requirements; + /// \brief A module with the same name that shadows this module. + Module *ShadowingModule = nullptr; + /// \brief Whether this module is missing a feature from \c Requirements. unsigned IsMissingRequirement : 1; @@ -323,13 +326,20 @@ public: /// /// \param Target The target options used for the current translation unit. /// - /// \param Req If this module is unavailable, this parameter - /// will be set to one of the requirements that is not met for use of - /// this module. + /// \param Req If this module is unavailable because of a missing requirement, + /// this parameter will be set to one of the requirements that is not met for + /// use of this module. + /// + /// \param MissingHeader If this module is unavailable because of a missing + /// header, this parameter will be set to one of the missing headers. + /// + /// \param ShadowingModule If this module is unavailable because it is + /// shadowed, this parameter will be set to the shadowing module. bool isAvailable(const LangOptions &LangOpts, const TargetInfo &Target, Requirement &Req, - UnresolvedHeaderDirective &MissingHeader) const; + UnresolvedHeaderDirective &MissingHeader, + Module *&ShadowingModule) const; /// \brief Determine whether this module is a submodule. bool isSubModule() const { return Parent != nullptr; } diff --git a/include/clang/Lex/HeaderSearch.h b/include/clang/Lex/HeaderSearch.h index 6d592e1..6df2036 100644 --- a/include/clang/Lex/HeaderSearch.h +++ b/include/clang/Lex/HeaderSearch.h @@ -657,7 +657,8 @@ private: LoadModuleMapResult loadModuleMapFileImpl(const FileEntry *File, bool IsSystem, - const DirectoryEntry *Dir); + const DirectoryEntry *Dir, + bool IsExplicitlyProvided); /// \brief Try to load the module map file in the given directory. /// diff --git a/include/clang/Lex/ModuleMap.h b/include/clang/Lex/ModuleMap.h index 155943e..4bffa50 100644 --- a/include/clang/Lex/ModuleMap.h +++ b/include/clang/Lex/ModuleMap.h @@ -155,6 +155,18 @@ private: /// header. llvm::DenseMap UmbrellaDirs; + /// \brief The set of modules provided explicitly (e.g. by -fmodule-map-file), + /// which are allowed to shadow other implicitly discovered modules. + llvm::DenseSet ExplicitlyProvidedModules; + + bool CurrentModuleMapIsExplicitlyProvided = false; + + bool mayShadowModuleBeingParsed(Module *ExistingModule) { + assert(!ExistingModule->Parent && "expected top-level module"); + return !CurrentModuleMapIsExplicitlyProvided && + ExplicitlyProvidedModules.count(ExistingModule); + } + /// \brief The set of attributes that can be attached to a module. struct Attributes { Attributes() : IsSystem(), IsExternC(), IsExhaustive() {} @@ -388,6 +400,11 @@ public: Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir, bool IsSystem, Module *Parent); + /// \brief Create a new top-level module that is shadowed by + /// \p ShadowingModule. + Module *createShadowedModule(StringRef Name, bool IsFramework, + Module *ShadowingModule); + /// \brief Retrieve the module map file containing the definition of the given /// module. /// @@ -483,6 +500,8 @@ public: /// \brief Marks this header as being excluded from the given module. void excludeHeader(Module *Mod, Module::Header Header); + void setExplicitlyProvided(Module *Mod); + /// \brief Parse the given module map file, and record any modules we /// encounter. /// @@ -497,11 +516,16 @@ public: /// \param ExternModuleLoc The location of the "extern module" declaration /// that caused us to load this module map file, if any. /// + /// \param IsExplicitlyProvided Whether this module map file was provided + /// explicitly by the user (e.g. -fmodule-map-file), rather than found + /// implicitly. + /// /// \returns true if an error occurred, false otherwise. bool parseModuleMapFile(const FileEntry *File, bool IsSystem, const DirectoryEntry *HomeDir, - SourceLocation ExternModuleLoc = SourceLocation()); - + SourceLocation ExternModuleLoc = SourceLocation(), + bool IsExplicitlyProvided = false); + /// \brief Dump the contents of the module map, for debugging purposes. void dump(); diff --git a/lib/Basic/Module.cpp b/lib/Basic/Module.cpp index 0b78326..a865674 100644 --- a/lib/Basic/Module.cpp +++ b/lib/Basic/Module.cpp @@ -79,11 +79,16 @@ static bool hasFeature(StringRef Feature, const LangOptions &LangOpts, bool Module::isAvailable(const LangOptions &LangOpts, const TargetInfo &Target, Requirement &Req, - UnresolvedHeaderDirective &MissingHeader) const { + UnresolvedHeaderDirective &MissingHeader, + Module *&ShadowingModule) const { if (IsAvailable) return true; for (const Module *Current = this; Current; Current = Current->Parent) { + if (Current->ShadowingModule) { + ShadowingModule = Current->ShadowingModule; + return false; + } for (unsigned I = 0, N = Current->Requirements.size(); I != N; ++I) { if (hasFeature(Current->Requirements[I].first, LangOpts, Target) != Current->Requirements[I].second) { diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp index 04f1390..2697502 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -1605,8 +1605,13 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, // Check whether this module is available. clang::Module::Requirement Requirement; clang::Module::UnresolvedHeaderDirective MissingHeader; + clang::Module *ShadowingModule = nullptr; if (!Module->isAvailable(getLangOpts(), getTarget(), Requirement, - MissingHeader)) { + MissingHeader, ShadowingModule)) { + + assert(!ShadowingModule && + "lookup of module by name should never find shadowed module"); + if (MissingHeader.FileNameLoc.isValid()) { getDiagnostics().Report(MissingHeader.FileNameLoc, diag::err_module_header_missing) diff --git a/lib/Frontend/FrontendActions.cpp b/lib/Frontend/FrontendActions.cpp index 865fb47..8b0eff2 100644 --- a/lib/Frontend/FrontendActions.cpp +++ b/lib/Frontend/FrontendActions.cpp @@ -321,8 +321,13 @@ bool GenerateModuleAction::BeginSourceFileAction(CompilerInstance &CI, // Check whether we can build this module at all. clang::Module::Requirement Requirement; clang::Module::UnresolvedHeaderDirective MissingHeader; + clang::Module *ShadowingModule = nullptr; if (!Module->isAvailable(CI.getLangOpts(), CI.getTarget(), Requirement, - MissingHeader)) { + MissingHeader, ShadowingModule)) { + + assert(!ShadowingModule && + "lookup of module by name should never find shadowed module"); + if (MissingHeader.FileNameLoc.isValid()) { CI.getDiagnostics().Report(MissingHeader.FileNameLoc, diag::err_module_header_missing) diff --git a/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index 8a686a7..ae602c7 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/lib/Lex/HeaderSearch.cpp @@ -1215,7 +1215,8 @@ bool HeaderSearch::loadModuleMapFile(const FileEntry *File, bool IsSystem) { } } - switch (loadModuleMapFileImpl(File, IsSystem, Dir)) { + switch (loadModuleMapFileImpl(File, IsSystem, Dir, + /*IsExplictlyProvided=*/true)) { case LMM_AlreadyLoaded: case LMM_NewlyLoaded: return false; @@ -1228,7 +1229,8 @@ bool HeaderSearch::loadModuleMapFile(const FileEntry *File, bool IsSystem) { HeaderSearch::LoadModuleMapResult HeaderSearch::loadModuleMapFileImpl(const FileEntry *File, bool IsSystem, - const DirectoryEntry *Dir) { + const DirectoryEntry *Dir, + bool IsExplicitlyProvided) { assert(File && "expected FileEntry"); // Check whether we've already loaded this module map, and mark it as being @@ -1237,14 +1239,16 @@ HeaderSearch::loadModuleMapFileImpl(const FileEntry *File, bool IsSystem, if (!AddResult.second) return AddResult.first->second ? LMM_AlreadyLoaded : LMM_InvalidModuleMap; - if (ModMap.parseModuleMapFile(File, IsSystem, Dir)) { + if (ModMap.parseModuleMapFile(File, IsSystem, Dir, SourceLocation(), + IsExplicitlyProvided)) { LoadedModuleMaps[File] = false; return LMM_InvalidModuleMap; } // Try to load a corresponding private module map. if (const FileEntry *PMMFile = getPrivateModuleMap(File, FileMgr)) { - if (ModMap.parseModuleMapFile(PMMFile, IsSystem, Dir)) { + if (ModMap.parseModuleMapFile(PMMFile, IsSystem, Dir, SourceLocation(), + IsExplicitlyProvided)) { LoadedModuleMaps[File] = false; return LMM_InvalidModuleMap; } @@ -1316,8 +1320,8 @@ HeaderSearch::loadModuleMapFile(const DirectoryEntry *Dir, bool IsSystem, return KnownDir->second ? LMM_AlreadyLoaded : LMM_InvalidModuleMap; if (const FileEntry *ModuleMapFile = lookupModuleMapFile(Dir, IsFramework)) { - LoadModuleMapResult Result = - loadModuleMapFileImpl(ModuleMapFile, IsSystem, Dir); + LoadModuleMapResult Result = loadModuleMapFileImpl( + ModuleMapFile, IsSystem, Dir, /*IsExplicitlyProvided=*/false); // Add Dir explicitly in case ModuleMapFile is in a subdirectory. // E.g. Foo.framework/Modules/module.modulemap // ^Dir ^ModuleMapFile diff --git a/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp index a752402..6fa4730 100644 --- a/lib/Lex/ModuleMap.cpp +++ b/lib/Lex/ModuleMap.cpp @@ -29,6 +29,7 @@ #include "llvm/Support/FileSystem.h" #include "llvm/Support/Host.h" #include "llvm/Support/Path.h" +#include "llvm/Support/SaveAndRestore.h" #include "llvm/Support/raw_ostream.h" #include #if defined(LLVM_ON_UNIX) @@ -562,6 +563,8 @@ ModuleMap::findOrCreateModule(StringRef Name, Module *Parent, bool IsFramework, } if (!Parent) { Modules[Name] = Result; + if (CurrentModuleMapIsExplicitlyProvided) + ExplicitlyProvidedModules.insert(Result); if (!LangOpts.CurrentModule.empty() && !CompilingModule && Name == LangOpts.CurrentModule) { CompilingModule = Result; @@ -768,6 +771,19 @@ Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir, return Result; } +Module *ModuleMap::createShadowedModule(StringRef Name, bool IsFramework, + Module *ShadowingModule) { + + // Create a new module with this name. + Module *Result = + new Module(Name, SourceLocation(), /*Parent=*/nullptr, IsFramework, + /*IsExplicit=*/false, NumCreatedModules++); + Result->ShadowingModule = ShadowingModule; + Result->IsAvailable = false; + + return Result; +} + void ModuleMap::setUmbrellaHeader(Module *Mod, const FileEntry *UmbrellaHeader, Twine NameAsWritten) { Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader)); @@ -831,6 +847,11 @@ void ModuleMap::excludeHeader(Module *Mod, Module::Header Header) { Mod->Headers[Module::HK_Excluded].push_back(std::move(Header)); } +void ModuleMap::setExplicitlyProvided(Module *Mod) { + assert(Modules[Mod->Name] == Mod && "explicitly provided module is shadowed"); + ExplicitlyProvidedModules.insert(Mod); +} + const FileEntry * ModuleMap::getContainingModuleMapFile(const Module *Module) const { if (Module->DefinitionLoc.isInvalid()) @@ -1421,6 +1442,7 @@ void ModuleMapParser::parseModuleDecl() { SourceLocation LBraceLoc = consumeToken(); // Determine whether this (sub)module has already been defined. + Module *ShadowingModule = nullptr; if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) { if (Existing->DefinitionLoc.isInvalid() && !ActiveModule) { // Skip the module definition. @@ -1434,23 +1456,34 @@ void ModuleMapParser::parseModuleDecl() { } return; } - - Diags.Report(ModuleNameLoc, diag::err_mmap_module_redefinition) - << ModuleName; - Diags.Report(Existing->DefinitionLoc, diag::note_mmap_prev_definition); - - // Skip the module definition. - skipUntil(MMToken::RBrace); - if (Tok.is(MMToken::RBrace)) - consumeToken(); - - HadError = true; - return; + + if (!Existing->Parent && Map.mayShadowModuleBeingParsed(Existing)) { + ShadowingModule = Existing; + } else { + // This is not a shawdowed module decl, it is an illegal redefinition. + Diags.Report(ModuleNameLoc, diag::err_mmap_module_redefinition) + << ModuleName; + Diags.Report(Existing->DefinitionLoc, diag::note_mmap_prev_definition); + + // Skip the module definition. + skipUntil(MMToken::RBrace); + if (Tok.is(MMToken::RBrace)) + consumeToken(); + + HadError = true; + return; + } } // Start defining this module. - ActiveModule = Map.findOrCreateModule(ModuleName, ActiveModule, Framework, - Explicit).first; + if (ShadowingModule) { + ActiveModule = + Map.createShadowedModule(ModuleName, Framework, ShadowingModule); + } else { + ActiveModule = Map.findOrCreateModule(ModuleName, ActiveModule, Framework, + Explicit).first; + } + ActiveModule->DefinitionLoc = ModuleNameLoc; if (Attrs.IsSystem || IsSystem) ActiveModule->IsSystem = true; @@ -2432,7 +2465,8 @@ bool ModuleMapParser::parseModuleMapFile() { bool ModuleMap::parseModuleMapFile(const FileEntry *File, bool IsSystem, const DirectoryEntry *Dir, - SourceLocation ExternModuleLoc) { + SourceLocation ExternModuleLoc, + bool IsExplicitlyProvided) { llvm::DenseMap::iterator Known = ParsedModuleMap.find(File); if (Known != ParsedModuleMap.end()) @@ -2445,6 +2479,9 @@ bool ModuleMap::parseModuleMapFile(const FileEntry *File, bool IsSystem, if (!Buffer) return ParsedModuleMap[File] = true; + llvm::SaveAndRestore OldExplicit(CurrentModuleMapIsExplicitlyProvided); + CurrentModuleMapIsExplicitlyProvided |= IsExplicitlyProvided; + // Parse this module map file. Lexer L(ID, SourceMgr.getBuffer(ID), SourceMgr, MMapLangOpts); SourceLocation Start = L.getSourceLocation(); diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp index fd16168..01f680c 100644 --- a/lib/Lex/PPDirectives.cpp +++ b/lib/Lex/PPDirectives.cpp @@ -1683,13 +1683,17 @@ void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, if (!SuggestedModule.getModule()->isAvailable()) { clang::Module::Requirement Requirement; clang::Module::UnresolvedHeaderDirective MissingHeader; + clang::Module *ShadowingModule = nullptr; Module *M = SuggestedModule.getModule(); // Identify the cause. (void)M->isAvailable(getLangOpts(), getTargetInfo(), Requirement, - MissingHeader); + MissingHeader, ShadowingModule); if (MissingHeader.FileNameLoc.isValid()) { Diag(MissingHeader.FileNameLoc, diag::err_module_header_missing) << MissingHeader.IsUmbrella << MissingHeader.FileName; + } else if (ShadowingModule) { + Diag(M->DefinitionLoc, diag::err_module_shadowed) << M->Name; + Diag(ShadowingModule->DefinitionLoc, diag::note_previous_definition); } else { Diag(M->DefinitionLoc, diag::err_module_unavailable) << M->getFullModuleName() << Requirement.second << Requirement.first; diff --git a/test/Modules/Inputs/shadow/A1/A.h b/test/Modules/Inputs/shadow/A1/A.h new file mode 100644 index 0000000..f07c681 --- /dev/null +++ b/test/Modules/Inputs/shadow/A1/A.h @@ -0,0 +1 @@ +#define A1_A_h diff --git a/test/Modules/Inputs/shadow/A1/module.modulemap b/test/Modules/Inputs/shadow/A1/module.modulemap new file mode 100644 index 0000000..9439a43 --- /dev/null +++ b/test/Modules/Inputs/shadow/A1/module.modulemap @@ -0,0 +1,5 @@ +module A { + header "A.h" +} + +module A1 {} diff --git a/test/Modules/Inputs/shadow/A2/A.h b/test/Modules/Inputs/shadow/A2/A.h new file mode 100644 index 0000000..9880ed0 --- /dev/null +++ b/test/Modules/Inputs/shadow/A2/A.h @@ -0,0 +1 @@ +#define A2_A_h diff --git a/test/Modules/Inputs/shadow/A2/module.modulemap b/test/Modules/Inputs/shadow/A2/module.modulemap new file mode 100644 index 0000000..935d89b --- /dev/null +++ b/test/Modules/Inputs/shadow/A2/module.modulemap @@ -0,0 +1,5 @@ +module A { + header "A.h" +} + +module A2 {} diff --git a/test/Modules/shadow.m b/test/Modules/shadow.m new file mode 100644 index 0000000..44320af --- /dev/null +++ b/test/Modules/shadow.m @@ -0,0 +1,21 @@ +// RUN: rm -rf %t +// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/shadow/A1 -I %S/Inputs/shadow/A2 %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION +// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -fmodule-map-file=%S/Inputs/shadow/A2/module.modulemap %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION +// REDEFINITION: error: redefinition of module 'A' +// REDEFINITION: note: previously defined + +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -I %S/Inputs/shadow %s -verify + +@import A1; +@import A2; +@import A; + +#import "A2/A.h" // expected-note {{implicitly imported}} +// expected-error@A2/module.modulemap:1 {{import of shadowed module 'A'}} +// expected-note@A1/module.modulemap:1 {{previous definition}} + +#if defined(A2_A_h) +#error got the wrong definition of module A +#elif !defined(A1_A_h) +#error missing definition from A1 +#endif