bruno created this revision.
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.
We're using this internally for a couple years now, it would be nice to have it
upstreamed.
Patch originally by Ben Langmuir,
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20151116/143425.html
Based on cfe-dev discussion:
http://lists.llvm.org/pipermail/cfe-dev/2015-November/046164.html
https://reviews.llvm.org/D31269
Files:
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Module.h
include/clang/Lex/HeaderSearch.h
include/clang/Lex/ModuleMap.h
lib/Basic/Module.cpp
lib/Frontend/CompilerInstance.cpp
lib/Frontend/FrontendActions.cpp
lib/Lex/HeaderSearch.cpp
lib/Lex/ModuleMap.cpp
lib/Lex/PPDirectives.cpp
test/Modules/Inputs/shadow/A1/A.h
test/Modules/Inputs/shadow/A1/module.modulemap
test/Modules/Inputs/shadow/A2/A.h
test/Modules/Inputs/shadow/A2/module.modulemap
test/Modules/shadow.m
Index: test/Modules/shadow.m
===================================================================
--- /dev/null
+++ 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
Index: test/Modules/Inputs/shadow/A2/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+ header "A.h"
+}
+
+module A2 {}
Index: test/Modules/Inputs/shadow/A2/A.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/A.h
@@ -0,0 +1 @@
+#define A2_A_h
Index: test/Modules/Inputs/shadow/A1/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+ header "A.h"
+}
+
+module A1 {}
Index: test/Modules/Inputs/shadow/A1/A.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/A.h
@@ -0,0 +1 @@
+#define A1_A_h
Index: lib/Lex/PPDirectives.cpp
===================================================================
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1869,13 +1869,17 @@
if (!SuggestedModule.getModule()->isAvailable()) {
Module::Requirement Requirement;
Module::UnresolvedHeaderDirective MissingHeader;
+ 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;
Index: lib/Lex/ModuleMap.cpp
===================================================================
--- lib/Lex/ModuleMap.cpp
+++ 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 <stdlib.h>
#if defined(LLVM_ON_UNIX)
@@ -562,14 +563,16 @@
// Try to find an existing module with this name.
if (Module *Sub = lookupModuleQualified(Name, Parent))
return std::make_pair(Sub, false);
-
+
// Create a new module with this name.
Module *Result = new Module(Name, SourceLocation(), Parent, IsFramework,
IsExplicit, NumCreatedModules++);
if (!Parent) {
if (LangOpts.CurrentModule == Name)
SourceModule = Result;
Modules[Name] = Result;
+ if (CurrentModuleMapIsExplicitlyProvided)
+ ExplicitlyProvidedModules.insert(Result);
}
return std::make_pair(Result, true);
}
@@ -793,6 +796,19 @@
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));
@@ -865,6 +881,11 @@
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())
@@ -1462,6 +1483,7 @@
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.
@@ -1475,23 +1497,34 @@
}
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;
@@ -2524,7 +2557,8 @@
bool ModuleMap::parseModuleMapFile(const FileEntry *File, bool IsSystem,
const DirectoryEntry *Dir,
- SourceLocation ExternModuleLoc) {
+ SourceLocation ExternModuleLoc,
+ bool IsExplicitlyProvided) {
llvm::DenseMap<const FileEntry *, bool>::iterator Known
= ParsedModuleMap.find(File);
if (Known != ParsedModuleMap.end())
@@ -2537,6 +2571,9 @@
if (!Buffer)
return ParsedModuleMap[File] = true;
+ llvm::SaveAndRestore<bool> OldExplicit(CurrentModuleMapIsExplicitlyProvided);
+ CurrentModuleMapIsExplicitlyProvided |= IsExplicitlyProvided;
+
// Parse this module map file.
Lexer L(ID, SourceMgr.getBuffer(ID), SourceMgr, MMapLangOpts);
SourceLocation Start = L.getSourceLocation();
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -1336,7 +1336,8 @@
}
}
- switch (loadModuleMapFileImpl(File, IsSystem, Dir)) {
+ switch (loadModuleMapFileImpl(File, IsSystem, Dir,
+ /*IsExplictlyProvided=*/true)) {
case LMM_AlreadyLoaded:
case LMM_NewlyLoaded:
return false;
@@ -1349,23 +1350,26 @@
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
// loaded in case we recursively try to load it from itself.
auto AddResult = LoadedModuleMaps.insert(std::make_pair(File, true));
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;
}
@@ -1437,8 +1441,8 @@
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
Index: lib/Frontend/FrontendActions.cpp
===================================================================
--- lib/Frontend/FrontendActions.cpp
+++ lib/Frontend/FrontendActions.cpp
@@ -348,8 +348,13 @@
// 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)
Index: lib/Frontend/CompilerInstance.cpp
===================================================================
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1811,8 +1811,13 @@
// 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)
Index: lib/Basic/Module.cpp
===================================================================
--- lib/Basic/Module.cpp
+++ lib/Basic/Module.cpp
@@ -83,11 +83,16 @@
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) {
Index: include/clang/Lex/ModuleMap.h
===================================================================
--- include/clang/Lex/ModuleMap.h
+++ include/clang/Lex/ModuleMap.h
@@ -174,6 +174,18 @@
/// header.
llvm::DenseMap<const DirectoryEntry *, Module *> 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<const Module *> 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()
@@ -440,6 +452,11 @@
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.
///
@@ -535,6 +552,8 @@
/// \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.
///
@@ -549,11 +568,16 @@
/// \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();
Index: include/clang/Lex/HeaderSearch.h
===================================================================
--- include/clang/Lex/HeaderSearch.h
+++ include/clang/Lex/HeaderSearch.h
@@ -683,7 +683,8 @@
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.
///
Index: include/clang/Basic/Module.h
===================================================================
--- include/clang/Basic/Module.h
+++ include/clang/Basic/Module.h
@@ -157,6 +157,9 @@
/// will be false to indicate that this (sub)module is not available.
SmallVector<Requirement, 2> 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;
@@ -337,13 +340,20 @@
///
/// \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; }
Index: include/clang/Basic/DiagnosticCommonKinds.td
===================================================================
--- include/clang/Basic/DiagnosticCommonKinds.td
+++ include/clang/Basic/DiagnosticCommonKinds.td
@@ -94,6 +94,7 @@
"could not acquire lock file for module '%0': %1">, InGroup<ModuleBuild>;
def remark_module_lock_timeout : Remark<
"timed out waiting to acquire lock file for module '%0'">, InGroup<ModuleBuild>;
+def err_module_shadowed : Error<"import of shadowed module '%0'">, DefaultFatal;
def err_module_cycle : Error<"cyclic dependency in module '%0': %1">,
DefaultFatal;
def err_module_prebuilt : Error<
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
