From dc4d70cd0ea93226d43105b8035caff2884f5587 Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Wed, 21 Dec 2016 00:24:39 +0000 Subject: [PATCH] [modules] Handle modules with nonstandard names in module.private.modulemaps Summary: The module system supports accompanying a primary module (say Foo) with an auxiliary "private" module (defined in an adjacent module.private.modulemap file) that augments the primary module when associated private headers are available. The feature is intended to be used to augment the primary module with a submodule (say Foo.Private), however some users in the wild are choosing to augment the primary module with an additional top-level module with a "similar" name (in all cases so far: FooPrivate). This "works" when a user of the module initially imports a private header, such as '#import "Foo/something_private.h"' since the Foo import winds up importing FooPrivate in passing. But if the import is subsequently recorded in a PCH file, reloading the PCH will fail to validate because of a cross-check that attempts to find the module.modulemap (or module.private.modulemap) using HeaderSearch algorithm, applied to the "FooPrivate" name. Since it's stored in Foo.framework/Modules, not FooPrivate.framework/Modules, the check fails and the PCH is rejected. This patch adds a compensatory workaround in the HeaderSearch algorithm when searching (and failing to find) a module of the form FooPrivate: the name used to derive filesystem paths is decoupled from the module name being searched for, and if the initial search fails and the module is named "FooPrivate", the filesystem search name is altered to remove the "Private" suffix, and the algorithm is run a second time (still looking for a module named FooPrivate, but looking in directories derived from Foo). Accompanying this change is a new warning that triggers when a user loads a module.private.modulemap that defines a top-level module with a different name from the top-level module defined in its adjacent module.modulemap. Reviewers: doug.gregor, manmanren, bruno Subscribers: bruno, cfe-commits Differential Revision: https://reviews.llvm.org/D27852 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290219 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 1 + include/clang/Basic/DiagnosticLexKinds.td | 6 ++++ include/clang/Lex/HeaderSearch.h | 13 +++++++ lib/Lex/HeaderSearch.cpp | 28 ++++++++++++--- lib/Lex/ModuleMap.cpp | 36 +++++++++++++++++++ .../A.framework/Headers/a.h | 1 + .../A.framework/Headers/aprivate.h | 1 + .../A.framework/Modules/module.modulemap | 4 +++ .../Modules/module.private.modulemap | 4 +++ .../implicit-private-with-different-name.m | 20 +++++++++++ 10 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h create mode 100644 test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h create mode 100644 test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap create mode 100644 test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap create mode 100644 test/Modules/implicit-private-with-different-name.m diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index c1b831b771..ba82b36cea 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -243,6 +243,7 @@ def NonModularIncludeInModule : DiagGroup<"non-modular-include-in-module", [NonModularIncludeInFrameworkModule]>; def IncompleteModule : DiagGroup<"incomplete-module", [IncompleteUmbrella, NonModularIncludeInModule]>; +def PrivateModule : DiagGroup<"private-module">; def CXX11InlineNamespace : DiagGroup<"c++11-inline-namespace">; def InvalidNoreturn : DiagGroup<"invalid-noreturn">; diff --git a/include/clang/Basic/DiagnosticLexKinds.td b/include/clang/Basic/DiagnosticLexKinds.td index 8bcbc21c80..3b602a16c8 100644 --- a/include/clang/Basic/DiagnosticLexKinds.td +++ b/include/clang/Basic/DiagnosticLexKinds.td @@ -642,6 +642,12 @@ def err_mmap_expected_feature : Error<"expected a feature name">; def err_mmap_expected_attribute : Error<"expected an attribute name">; def warn_mmap_unknown_attribute : Warning<"unknown attribute '%0'">, InGroup; +def warn_mmap_mismatched_top_level_private : Warning< + "top-level module '%0' in private module map, expected a submodule of '%1'">, + InGroup; +def note_mmap_rename_top_level_private_as_submodule : Note< + "make '%0' a submodule of '%1' to ensure it can be found by name">, + InGroup; def warn_auto_module_import : Warning< "treating #%select{include|import|include_next|__include_macros}0 as an " diff --git a/include/clang/Lex/HeaderSearch.h b/include/clang/Lex/HeaderSearch.h index 60214df783..b145d7bae1 100644 --- a/include/clang/Lex/HeaderSearch.h +++ b/include/clang/Lex/HeaderSearch.h @@ -547,6 +547,19 @@ public: void loadTopLevelSystemModules(); private: + + /// \brief Lookup a module with the given module name and search-name. + /// + /// \param ModuleName The name of the module we're looking for. + /// + /// \param SearchName The "search-name" to derive filesystem paths from + /// when looking for the module map; this is usually equal to ModuleName, + /// but for compatibility with some buggy frameworks, additional attempts + /// may be made to find the module under a related-but-different search-name. + /// + /// \returns The module named ModuleName. + Module *lookupModule(StringRef ModuleName, StringRef SearchName); + /// \brief Retrieve a module with the given name, which may be part of the /// given framework. /// diff --git a/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index 72e8b45682..b5228fc6c8 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/lib/Lex/HeaderSearch.cpp @@ -194,16 +194,36 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch) { Module *Module = ModMap.findModule(ModuleName); if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) return Module; - + + StringRef SearchName = ModuleName; + Module = lookupModule(ModuleName, SearchName); + + // The facility for "private modules" -- adjacent, optional module maps named + // module.private.modulemap that are supposed to define private submodules -- + // is sometimes misused by frameworks that name their associated private + // module FooPrivate, rather than as a submodule named Foo.Private as + // intended. Here we compensate for such cases by looking in directories named + // Foo.framework, when we previously looked and failed to find a + // FooPrivate.framework. + if (!Module && SearchName.consume_back("Private")) + Module = lookupModule(ModuleName, SearchName); + return Module; +} + +Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) { + Module *Module = nullptr; + // Look through the various header search paths to load any available module // maps, searching for a module map that describes this module. for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) { if (SearchDirs[Idx].isFramework()) { - // Search for or infer a module map for a framework. + // Search for or infer a module map for a framework. Here we use + // SearchName rather than ModuleName, to permit finding private modules + // named FooPrivate in buggy frameworks named Foo. SmallString<128> FrameworkDirName; FrameworkDirName += SearchDirs[Idx].getFrameworkDir()->getName(); - llvm::sys::path::append(FrameworkDirName, ModuleName + ".framework"); - if (const DirectoryEntry *FrameworkDir + llvm::sys::path::append(FrameworkDirName, SearchName + ".framework"); + if (const DirectoryEntry *FrameworkDir = FileMgr.getDirectory(FrameworkDirName)) { bool IsSystem = SearchDirs[Idx].getDirCharacteristic() != SrcMgr::C_User; diff --git a/lib/Lex/ModuleMap.cpp b/lib/Lex/ModuleMap.cpp index c1b7b33c31..b7bba7ea9b 100644 --- a/lib/Lex/ModuleMap.cpp +++ b/lib/Lex/ModuleMap.cpp @@ -1490,6 +1490,42 @@ void ModuleMapParser::parseModuleDecl() { ActiveModule->NoUndeclaredIncludes = true; ActiveModule->Directory = Directory; + if (!ActiveModule->Parent) { + StringRef MapFileName(ModuleMapFile->getName()); + if (MapFileName.endswith("module.private.modulemap") || + MapFileName.endswith("module_private.map")) { + // Adding a top-level module from a private modulemap is likely a + // user error; we check to see if there's another top-level module + // defined in the non-private map in the same dir, and if so emit a + // warning. + for (auto E = Map.module_begin(); E != Map.module_end(); ++E) { + auto const *M = E->getValue(); + if (!M->Parent && + M->Directory == ActiveModule->Directory && + M->Name != ActiveModule->Name) { + Diags.Report(ActiveModule->DefinitionLoc, + diag::warn_mmap_mismatched_top_level_private) + << ActiveModule->Name << M->Name; + // The pattern we're defending against here is typically due to + // a module named FooPrivate which is supposed to be a submodule + // called Foo.Private. Emit a fixit in that case. + auto D = + Diags.Report(ActiveModule->DefinitionLoc, + diag::note_mmap_rename_top_level_private_as_submodule); + D << ActiveModule->Name << M->Name; + StringRef Bad(ActiveModule->Name); + if (Bad.consume_back("Private")) { + SmallString<128> Fixed = Bad; + Fixed.append(".Private"); + D << FixItHint::CreateReplacement(ActiveModule->DefinitionLoc, + Fixed); + } + break; + } + } + } + } + bool Done = false; do { switch (Tok.Kind) { diff --git a/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h new file mode 100644 index 0000000000..8b4b198745 --- /dev/null +++ b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h @@ -0,0 +1 @@ +extern int APUBLIC; diff --git a/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h new file mode 100644 index 0000000000..760d901aa3 --- /dev/null +++ b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h @@ -0,0 +1 @@ +extern int APRIVATE; diff --git a/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap new file mode 100644 index 0000000000..95eabf90a9 --- /dev/null +++ b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap @@ -0,0 +1,4 @@ +framework module A { + header "a.h" + export * +} diff --git a/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap new file mode 100644 index 0000000000..020d5d31d8 --- /dev/null +++ b/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap @@ -0,0 +1,4 @@ +framework module APrivate { + header "aprivate.h" + export * +} diff --git a/test/Modules/implicit-private-with-different-name.m b/test/Modules/implicit-private-with-different-name.m new file mode 100644 index 0000000000..8da331cb6a --- /dev/null +++ b/test/Modules/implicit-private-with-different-name.m @@ -0,0 +1,20 @@ +// RUN: rm -rf %t + +// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced +// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s + +// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block +// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only + +// Check the fixit +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level module 'APrivate' in private module map, expected a submodule of 'A'}} +// expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make 'APrivate' a submodule of 'A' to ensure it can be found by name}} +// CHECK: fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private" + +#ifndef HEADER +#define HEADER +#import "A/aprivate.h" +const int *y = &APRIVATE; +#endif -- 2.40.0