[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

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

[bruno]

Hi Yuka, thanks for working on this.

clang/lib/Lex/HeaderSearch.cpp

285

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

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

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

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

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

#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

clang/include/clang/Lex/ModuleMap.h

class ModuleMap {
   /// The number of modules we have created in total.
   unsigned NumCreatedModules = 0;
 
   /// In case a module has a export_as entry, it might have a pending link
   /// name to be determined if that module is imported.
   llvm::StringMap<llvm::StringSet<>> PendingLinkAsModule;
 
 public:
+  /// Get LangOpts of ModuleMap
+  const LangOptions& getLangOpts() {
+    return LangOpts;
+  }
+
   /// Use PendingLinkAsModule information to mark top level link names that
   /// are going to be replaced by export_as aliases.
   void resolveLinkAsDependencies(Module *Mod);
 
   /// Make module to use export_as as the link dependency name if enough
   /// information is available or add it to a pending list otherwise.
   void addLinkAsDependency(Module *Mod);
 

clang/lib/Lex/HeaderSearch.cpp

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.
+    if (ModMap.getLangOpts().ObjC1 || ModMap.getLangOpts().ObjC2)
       loadSubdirectoryModuleMaps(SearchDirs[Idx]);
 
     // Look again for the module.
     Module = ModMap.findModule(ModuleName);
     if (Module)
       break;
   }
 
   return Module;