From a15364152cf4d6267a82b616151de301291f2f5b Mon Sep 17 00:00:00 2001 From: Yuka Takahashi Date: Tue, 10 Jul 2018 12:17:34 +0000 Subject: [PATCH] [modules] Fix 37878; Autoload subdirectory modulemaps with specific LangOpts Summary: Reproducer and errors: https://bugs.llvm.org/show_bug.cgi?id=37878 lookupModule was falling back to loadSubdirectoryModuleMaps when it couldn't find ModuleName in (proper) search paths. This was causing iteration over all files in the search path subdirectories for example "/usr/include/foobar" in bugzilla case. Users don't expect Clang to load modulemaps in subdirectories implicitly, and also the disk access is not cheap. if (AllowExtraModuleMapSearch) true with ObjC with @import ModuleName. Reviewers: rsmith, aprantl, bruno Subscribers: cfe-commits, teemperor, v.g.vassilev Differential Revision: https://reviews.llvm.org/D48367 llvm-svn: 336660 --- clang/include/clang/Lex/HeaderSearch.h | 12 ++++++++++-- clang/lib/Frontend/CompilerInstance.cpp | 15 ++++++++++----- clang/lib/Lex/HeaderSearch.cpp | 17 ++++++++++------- clang/lib/Serialization/ASTReader.cpp | 4 +++- clang/test/Modules/Inputs/autoload-subdirectory/a.h | 9 +++++++++ clang/test/Modules/Inputs/autoload-subdirectory/b.h | 1 + clang/test/Modules/Inputs/autoload-subdirectory/c.h | 7 +++++++ .../autoload-subdirectory/include/module.modulemap | 3 +++ .../Inputs/autoload-subdirectory/module.modulemap | 3 +++ clang/test/Modules/autoload-subdirectory.cpp | 10 ++++++++++ 10 files changed, 66 insertions(+), 15 deletions(-) create mode 100644 clang/test/Modules/Inputs/autoload-subdirectory/a.h create mode 100644 clang/test/Modules/Inputs/autoload-subdirectory/b.h create mode 100644 clang/test/Modules/Inputs/autoload-subdirectory/c.h create mode 100644 clang/test/Modules/Inputs/autoload-subdirectory/include/module.modulemap create mode 100644 clang/test/Modules/Inputs/autoload-subdirectory/module.modulemap create mode 100644 clang/test/Modules/autoload-subdirectory.cpp diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index fd52000..b7147c5 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -529,8 +529,12 @@ public: /// search directories to produce a module definition. If not, this lookup /// will only return an already-known module. /// + /// \param AllowExtraModuleMapSearch Whether we allow to search modulemaps + /// in subdirectories. + /// /// \returns The module with the given name. - Module *lookupModule(StringRef ModuleName, bool AllowSearch = true); + Module *lookupModule(StringRef ModuleName, bool AllowSearch = true, + bool AllowExtraModuleMapSearch = false); /// Try to find a module map file in the given directory, returning /// \c nullptr if none is found. @@ -595,8 +599,12 @@ private: /// but for compatibility with some buggy frameworks, additional attempts /// may be made to find the module under a related-but-different search-name. /// + /// \param AllowExtraModuleMapSearch Whether we allow to search modulemaps + /// in subdirectories. + /// /// \returns The module named ModuleName. - Module *lookupModule(StringRef ModuleName, StringRef SearchName); + Module *lookupModule(StringRef ModuleName, StringRef SearchName, + bool AllowExtraModuleMapSearch = false); /// Retrieve a module with the given name, which may be part of the /// given framework. diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp index 5727aae..155ead4 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1653,8 +1653,10 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, // Retrieve the cached top-level module. Module = Known->second; } else if (ModuleName == getLangOpts().CurrentModule) { - // This is the module we're building. - Module = PP->getHeaderSearchInfo().lookupModule(ModuleName); + // This is the module we're building. + Module = PP->getHeaderSearchInfo().lookupModule( + ModuleName, /*AllowSearch*/ true, + /*AllowExtraModuleMapSearch*/ !IsInclusionDirective); /// FIXME: perhaps we should (a) look for a module using the module name // to file map (PrebuiltModuleFiles) and (b) diagnose if still not found? //if (Module == nullptr) { @@ -1666,7 +1668,8 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, Known = KnownModules.insert(std::make_pair(Path[0].first, Module)).first; } else { // Search for a module with the given name. - Module = PP->getHeaderSearchInfo().lookupModule(ModuleName); + Module = PP->getHeaderSearchInfo().lookupModule(ModuleName, true, + !IsInclusionDirective); HeaderSearchOptions &HSOpts = PP->getHeaderSearchInfo().getHeaderSearchOpts(); @@ -1743,7 +1746,8 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, ImportLoc, ARRFlags)) { case ASTReader::Success: { if (Source != ModuleCache && !Module) { - Module = PP->getHeaderSearchInfo().lookupModule(ModuleName); + Module = PP->getHeaderSearchInfo().lookupModule(ModuleName, true, + !IsInclusionDirective); if (!Module || !Module->getASTFile() || FileMgr->getFile(ModuleFileName) != Module->getASTFile()) { // Error out if Module does not refer to the file in the prebuilt @@ -1874,7 +1878,8 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, PrivateModule, PP->getIdentifierInfo(Module->Name)->getTokenID()); PrivPath.push_back(std::make_pair(&II, Path[0].second)); - if (PP->getHeaderSearchInfo().lookupModule(PrivateModule)) + if (PP->getHeaderSearchInfo().lookupModule(PrivateModule, true, + !IsInclusionDirective)) Sub = loadModule(ImportLoc, PrivPath, Visibility, IsInclusionDirective); if (Sub) { diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index 312bd2d..b1a2ef1 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -198,14 +198,15 @@ std::string HeaderSearch::getCachedModuleFileName(StringRef ModuleName, return Result.str().str(); } -Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch) { +Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch, + bool AllowExtraModuleMapSearch) { // Look in the module map to determine if there is a module by this name. Module *Module = ModMap.findModule(ModuleName); if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) return Module; StringRef SearchName = ModuleName; - Module = lookupModule(ModuleName, SearchName); + Module = lookupModule(ModuleName, SearchName, AllowExtraModuleMapSearch); // The facility for "private modules" -- adjacent, optional module maps named // module.private.modulemap that are supposed to define private submodules -- @@ -216,13 +217,14 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch) { // could force building unwanted dependencies into the parent module and cause // dependency cycles. if (!Module && SearchName.consume_back("_Private")) - Module = lookupModule(ModuleName, SearchName); + Module = lookupModule(ModuleName, SearchName, AllowExtraModuleMapSearch); if (!Module && SearchName.consume_back("Private")) - Module = lookupModule(ModuleName, SearchName); + Module = lookupModule(ModuleName, SearchName, AllowExtraModuleMapSearch); return Module; } -Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) { +Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName, + bool AllowExtraModuleMapSearch) { Module *Module = nullptr; // Look through the various header search paths to load any available module @@ -281,8 +283,9 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) { continue; // Load all module maps in the immediate subdirectories of this search - // directory. - loadSubdirectoryModuleMaps(SearchDirs[Idx]); + // directory if ModuleName was from @import. + if (AllowExtraModuleMapSearch) + loadSubdirectoryModuleMaps(SearchDirs[Idx]); // Look again for the module. Module = ModMap.findModule(ModuleName); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 3277243..78ca5f7 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -2626,7 +2626,9 @@ ASTReader::ReadControlBlock(ModuleFile &F, "MODULE_DIRECTORY found before MODULE_NAME"); // If we've already loaded a module map file covering this module, we may // have a better path for it (relative to the current build). - Module *M = PP.getHeaderSearchInfo().lookupModule(F.ModuleName); + Module *M = PP.getHeaderSearchInfo().lookupModule( + F.ModuleName, /*AllowSearch*/ true, + /*AllowExtraModuleMapSearch*/ true); if (M && M->Directory) { // If we're implicitly loading a module, the base directory can't // change between the build and use. diff --git a/clang/test/Modules/Inputs/autoload-subdirectory/a.h b/clang/test/Modules/Inputs/autoload-subdirectory/a.h new file mode 100644 index 0000000..8be9431 --- /dev/null +++ b/clang/test/Modules/Inputs/autoload-subdirectory/a.h @@ -0,0 +1,9 @@ +#include "b.h" + +class foo { + int x, y; + +public: + foo(){}; + ~foo(){}; +}; diff --git a/clang/test/Modules/Inputs/autoload-subdirectory/b.h b/clang/test/Modules/Inputs/autoload-subdirectory/b.h new file mode 100644 index 0000000..bfde5bf --- /dev/null +++ b/clang/test/Modules/Inputs/autoload-subdirectory/b.h @@ -0,0 +1 @@ +class bar {}; diff --git a/clang/test/Modules/Inputs/autoload-subdirectory/c.h b/clang/test/Modules/Inputs/autoload-subdirectory/c.h new file mode 100644 index 0000000..e5a4525 --- /dev/null +++ b/clang/test/Modules/Inputs/autoload-subdirectory/c.h @@ -0,0 +1,7 @@ +class nyan { + bool x, y; + +public: + nyan(){}; + ~nyan(){}; +}; diff --git a/clang/test/Modules/Inputs/autoload-subdirectory/include/module.modulemap b/clang/test/Modules/Inputs/autoload-subdirectory/include/module.modulemap new file mode 100644 index 0000000..880ae38 --- /dev/null +++ b/clang/test/Modules/Inputs/autoload-subdirectory/include/module.modulemap @@ -0,0 +1,3 @@ +module a { header "a.h" } +module b { header "b.h" } +module c { header "c.h" } diff --git a/clang/test/Modules/Inputs/autoload-subdirectory/module.modulemap b/clang/test/Modules/Inputs/autoload-subdirectory/module.modulemap new file mode 100644 index 0000000..880ae38 --- /dev/null +++ b/clang/test/Modules/Inputs/autoload-subdirectory/module.modulemap @@ -0,0 +1,3 @@ +module a { header "a.h" } +module b { header "b.h" } +module c { header "c.h" } diff --git a/clang/test/Modules/autoload-subdirectory.cpp b/clang/test/Modules/autoload-subdirectory.cpp new file mode 100644 index 0000000..e76f705 --- /dev/null +++ b/clang/test/Modules/autoload-subdirectory.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -fmodules -fmodule-name=Foo -I %S/Inputs/autoload-subdirectory/ %s -verify +// expected-no-diagnostics + +#include "a.h" +#import "c.h" + +int main() { + foo neko; + return 0; +} -- 2.7.4