[modules] Fix 37878; Autoload subdirectory modulemaps with specific LangOpts

[yamaguchi]

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.

[yamaguchi]

Update commmit message and comment, add ObjC2

[aprantl]

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

Are these flags also enabled in Objective-C++ mode?

[bruno]

Hi Yuka, thanks for working on this.

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

Looks like all this logic was introduced in r177621 to allow the names of modules to differ from the name of their subdirectory in the include path.

Instead of having this to be based on the language, it's probably better if we have it based on @import name lookup, which is the scenario where we actually currently look more aggressively, did you try that path?

This is also lacking a testcase, can you create one?

[yamaguchi]

Add a test case

[yamaguchi]

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

Are these flags also enabled in Objective-C++ mode?

I think so from FrontendOptions.h:190
bool isObjectiveC() const { return Lang == ObjC || Lang == ObjCXX; }

it's probably better if we have it based on @import name lookup

I don't think I understood what you're saying, could you explain a bit more?

This is also lacking a testcase, can you create one?

Sure!

[bruno]

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

I don't think I understood what you're saying, could you explain a bit more?

This extra call to loadSubdirectoryModuleMaps was introduced in r177621 to allow @import SomeName to work even if SomeName doesn't match a subdirectory name. See the original commit for more details.

This means that it was never supposed to be called when the module is built via #include or #import, which is what you are getting. That said, I believe it's better if the condition for calling loadSubdirectoryModuleMaps checks somehow if the lookup is coming from of an @import instead of checking the language (we too don't want it to be called for ObjC when #include or #import are used).

clang/test/Modules/autoload-subdirectory.cpp

1 ↗

(On Diff #152240)

Instead of %clang, please use %clang_cc1 and -verify here.

[yamaguchi]

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

we too don't want it to be called for ObjC when #include or #import are used

https://gist.github.com/yamaguchi1024/27caba1897eb813b297a8c4785adc11d
This is one thing I could think of, it excludes #include but #import and @import are still treated in the same way. Do you have any idea how to do this? Is Clang differenciating the module behavior based on #import or @import in ObjC?

clang/test/Modules/autoload-subdirectory.cpp

1 ↗

(On Diff #152240)

Sure

[yamaguchi]

use %clang_cc1 instead of %clang

[bruno]

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

https://gist.github.com/yamaguchi1024/27caba1897eb813b297a8c4785adc11d

Something along these lines seems fine. I'd reverse the condition of IsInclusionDirective to be true by default (which is the most common case), and you pass false when coming from @imports. Maybe also rename it to something more meaningful for the change in question, around the lines of AllowExtraModuleMapSearch.

This is one thing I could think of, it excludes #include but #import and @import are still treated in the same way.

#import and #include should be treated the same way (extra searches shouldn't happen for both), @import is the only one different.

Do you have any idea how to do this? Is Clang differenciating the module behavior based on #import or @import in ObjC?

#import works very much like #include, the difference is that #import's have implicit #pragma once like behavior.

[yamaguchi]

Add #import test and add branch AllowExtraModuleMapSearch when ModuleName was from @import

clang/lib/Lex/HeaderSearch.cpp

285 ↗

(On Diff #152086)

#import and #include should be treated the same way

I'll add a test for #import as well.

[yamaguchi]

Delete a.out)

[bruno]

Thanks for working on this, LGTM

cfe/trunk/include/clang/Lex/HeaderSearch.h

public:
   /// Lookup a module Search for a module with the given name.
   ///
   /// \param ModuleName The name of the module we're looking for.
   ///
   /// \param AllowSearch Whether we are allowed to search in the various
   /// 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.
   const FileEntry *lookupModuleMapFile(const DirectoryEntry *Dir,
                                        bool IsFramework);
   
   void IncrementFrameworkLookupCount() { ++NumFrameworkLookups; }
 
private:
   ///
   /// \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.
   ///
+  /// \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.
   ///
   /// \param Name The name of the module to retrieve.
   ///
   /// \param Dir The framework directory (e.g., ModuleName.framework).
   ///

cfe/trunk/lib/Frontend/CompilerInstance.cpp

CompilerInstance::loadModule(SourceLocation ImportLoc,
 
   // If we don't already have information on this module, load the module now.
   llvm::DenseMap<const IdentifierInfo *, clang::Module *>::iterator Known
     = KnownModules.find(Path[0].first);
   if (Known != KnownModules.end()) {
     // 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) {
     //  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
     //    << ModuleName;
     //  ModuleBuildFailed = true;
     //  return ModuleLoadResult();
     //}
     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();
 
     std::string ModuleFileName;
     enum ModuleSource {
       ModuleNotFound, ModuleCache, PrebuiltModulePath, ModuleBuildPragma
     } Source = ModuleNotFound;
 
switch (ModuleManager->ReadAST(ModuleFileName,
                                    Source == PrebuiltModulePath
                                        ? serialization::MK_PrebuiltModule
                                        : Source == ModuleBuildPragma
                                              ? serialization::MK_ExplicitModule
                                              : serialization::MK_ImplicitModule,
                                    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
           // module path.
           getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
               << ModuleName;
           ModuleBuildFailed = true;
           KnownModules[Path[0].first] = nullptr;
for (unsigned I = 1, N = Path.size(); I != N; ++I) {
         SmallString<128> PrivateModule(Module->Name);
         PrivateModule.append("_Private");
 
         SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> PrivPath;
         auto &II = PP->getIdentifierTable().get(
             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) {
           MapPrivateSubModToTopLevel = true;
           if (!getDiagnostics().isIgnored(
                   diag::warn_no_priv_submodule_use_toplevel, ImportLoc)) {
             getDiagnostics().Report(Path[I].second,
                                     diag::warn_no_priv_submodule_use_toplevel)

cfe/trunk/lib/Lex/HeaderSearch.cpp

if (HSOpts->DisableModuleHash) {
 
     SmallString<128> HashStr;
     llvm::APInt(64, size_t(Hash)).toStringUnsigned(HashStr, /*Radix*/36);
     llvm::sys::path::append(Result, ModuleName + "-" + HashStr + ".pcm");
   }
   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 --
   // may have different flavors of names: FooPrivate, Foo_Private and Foo.Private.
   //
   // Foo.Private is now deprecated in favor of Foo_Private. Users of FooPrivate
   // should also rename to Foo_Private. Representing private as submodules
   // 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
   // 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. Here we use
       // SearchName rather than ModuleName, to permit finding private modules
for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
     }
 
     // If we've already performed the exhaustive search for module maps in this
     // search directory, don't do it again.
     if (SearchDirs[Idx].haveSearchedAllModuleMaps())
       continue;
 
     // Load all module maps in the immediate subdirectories of this search
-    // directory.
+    // directory if ModuleName was from @import.
+    if (AllowExtraModuleMapSearch)
       loadSubdirectoryModuleMaps(SearchDirs[Idx]);
 
     // Look again for the module.
     Module = ModMap.findModule(ModuleName);
     if (Module)
       break;
   }
 
   return Module;

cfe/trunk/lib/Serialization/ASTReader.cpp

case MODULE_NAME:
 
       break;
 
     case MODULE_DIRECTORY: {
       assert(!F.ModuleName.empty() &&
              "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.
         if (F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) {
           const DirectoryEntry *BuildDir =
               PP.getFileManager().getDirectory(Blob);
           if (!BuildDir || BuildDir != M->Directory) {
             if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)

cfe/trunk/test/Modules/Inputs/autoload-subdirectory/a.h

+#include "b.h"
+
+class foo {
+  int x, y;
+
+public:
+  foo(){};
+  ~foo(){};
+};

cfe/trunk/test/Modules/Inputs/autoload-subdirectory/b.h

+class bar {};

cfe/trunk/test/Modules/Inputs/autoload-subdirectory/c.h

+class nyan {
+  bool x, y;
+
+public:
+  nyan(){};
+  ~nyan(){};
+};

cfe/trunk/test/Modules/Inputs/autoload-subdirectory/include/module.modulemap

+module a { header "a.h" }
+module b { header "b.h" }
+module c { header "c.h" }

cfe/trunk/test/Modules/Inputs/autoload-subdirectory/module.modulemap

+module a { header "a.h" }
+module b { header "b.h" }
+module c { header "c.h" }

cfe/trunk/test/Modules/autoload-subdirectory.cpp

+// 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;
+}