[clang][modules] Lazily load by name lookups in module maps

Instead of eagerly populating the `clang::ModuleMap` when looking up
a module by name, this patch changes `HeaderSearch` to only load the
modules that are actually used.

This introduces `ModuleMap::findOrLoadModule` which will load modules
from parsed but not loaded module maps. This cannot be used anywhere
that the module loading code calls into as it can create infinite
recursion.

This currently just reparses module maps when looking up a module by
header. This is fine as redeclarations are allowed from the same file,
but future patches will also make looking up a module by header lazy.

This patch changes the shadow.m test to use explicitly built modules
and `#import`. This test and the shadow feature are very brittle and
do not work in general. The test relied on pcm files being left behind
by prior failing clang invocations that were then reused by the last
invocation. If you clean the cache then the last invocation will
always fail. This is because the input module map and the
`-fmodule-map-file=` module map are parsed in the same module scope,
and `-fmodule-map-file=` is forwarded to implicit module builds. That
means you are guaranteed to hit a module redeclaration error if the TU
actually imports the module it is trying to shadow.

This patch changes when we load A2's module map to after the `A`
module has been loaded, which sets the `IsFromModuleFile` bit on `A`.
This means that A2's `A` is skipped entirely instead of creating a
shadow module, and we get textual inclusion. It is possible to
construct a case where this would happen before this patch too.

An upcoming patch in this series will rework shadowing to work in the
general case, but that's only possible once header -> module lookup is
lazy too.

Author: Bigcheese

clang-tools-extra/modularize/ModularizeUtilities.cpp

@@ -290,7 +290,7 @@ std::error_code ModularizeUtilities::loadModuleMap(
     Target.get(), *HeaderInfo));
 
   // Parse module.modulemap file into module map.
-  if (ModMap->loadModuleMapFile(ModuleMapEntry, false, Dir)) {
+  if (ModMap->parseAndLoadModuleMapFile(ModuleMapEntry, false, Dir)) {
     return std::error_code(1, std::generic_category());
   }
 

clang/include/clang/Basic/DiagnosticGroups.td

@@ -606,6 +606,7 @@ def ModuleImport : DiagGroup<"module-import">;
 def ModuleConflict : DiagGroup<"module-conflict">;
 def ModuleFileExtension : DiagGroup<"module-file-extension">;
 def ModuleIncludeDirectiveTranslation : DiagGroup<"module-include-translation">;
+def ModuleMap : DiagGroup<"module-map">;
 def RoundTripCC1Args : DiagGroup<"round-trip-cc1-args">;
 def NewlineEOF : DiagGroup<"newline-eof">;
 def Nullability : DiagGroup<"nullability">;

clang/include/clang/Basic/DiagnosticLexKinds.td

@@ -838,6 +838,12 @@ def warn_pp_date_time : Warning<
   ShowInSystemHeader, DefaultIgnore, InGroup<DiagGroup<"date-time">>;
 
 // Module map parsing
+def remark_mmap_parse : Remark<
+  "parsing modulemap '%0'">, ShowInSystemHeader, InGroup<ModuleMap>;
+def remark_mmap_load : Remark<
+  "loading modulemap '%0'">, ShowInSystemHeader, InGroup<ModuleMap>;
+def remark_mmap_load_module : Remark<
+  "loading parsed module '%0'">, ShowInSystemHeader, InGroup<ModuleMap>;
 def err_mmap_unknown_token : Error<"skipping stray token">;
 def err_mmap_expected_module : Error<"expected module declaration">;
 def err_mmap_expected_module_name : Error<"expected module name">;

clang/include/clang/Lex/HeaderSearch.h

@@ -332,13 +332,27 @@ class HeaderSearch {
   /// The mapping between modules and headers.
   mutable ModuleMap ModMap;
 
+  struct ModuleMapDirectoryState {
+    OptionalFileEntryRef ModuleMapFile;
+    enum {
+      Parsed,
+      Loaded,
+      Invalid,
+    } Status;
+  };
+
   /// Describes whether a given directory has a module map in it.
-  llvm::DenseMap<const DirectoryEntry *, bool> DirectoryHasModuleMap;
+  llvm::DenseMap<const DirectoryEntry *, ModuleMapDirectoryState>
+      DirectoryModuleMap;
 
   /// Set of module map files we've already loaded, and a flag indicating
   /// whether they were valid or not.
   llvm::DenseMap<const FileEntry *, bool> LoadedModuleMaps;
 
+  /// Set of module map files we've already parsed, and a flag indicating
+  /// whether they were valid or not.
+  llvm::DenseMap<const FileEntry *, bool> ParsedModuleMaps;
+
   // A map of discovered headers with their associated include file name.
   llvm::DenseMap<const FileEntry *, llvm::SmallString<64>> IncludeNames;
 
@@ -433,11 +447,6 @@ class HeaderSearch {
   /// Retrieve the path to the module cache.
   StringRef getModuleCachePath() const { return ModuleCachePath; }
 
-  /// Consider modules when including files from this directory.
-  void setDirectoryHasModuleMap(const DirectoryEntry* Dir) {
-    DirectoryHasModuleMap[Dir] = true;
-  }
-
   /// Forget everything we know about headers so far.
   void ClearFileInfo() {
     FileInfo.clear();
@@ -713,9 +722,10 @@ class HeaderSearch {
   ///        used to resolve paths within the module (this is required when
   ///        building the module from preprocessed source).
   /// \returns true if an error occurred, false otherwise.
-  bool loadModuleMapFile(FileEntryRef File, bool IsSystem, FileID ID = FileID(),
-                         unsigned *Offset = nullptr,
-                         StringRef OriginalModuleMapFile = StringRef());
+  bool parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem,
+                                 FileID ID = FileID(),
+                                 unsigned *Offset = nullptr,
+                                 StringRef OriginalModuleMapFile = StringRef());
 
   /// Collect the set of all known, top-level modules.
   ///
@@ -915,26 +925,31 @@ class HeaderSearch {
   size_t getTotalMemory() const;
 
 private:
-  /// Describes what happened when we tried to load a module map file.
-  enum LoadModuleMapResult {
-    /// The module map file had already been loaded.
-    LMM_AlreadyLoaded,
+  /// Describes what happened when we tried to load or parse a module map file.
+  enum ModuleMapResult {
+    /// The module map file had already been processed.
+    MMR_AlreadyProcessed,
 
-    /// The module map file was loaded by this invocation.
-    LMM_NewlyLoaded,
+    /// The module map file was processed by this invocation.
+    MMR_NewlyProcessed,
 
     /// There is was directory with the given name.
-    LMM_NoDirectory,
+    MMR_NoDirectory,
 
     /// There was either no module map file or the module map file was
     /// invalid.
-    LMM_InvalidModuleMap
+    MMR_InvalidModuleMap
   };
 
-  LoadModuleMapResult loadModuleMapFileImpl(FileEntryRef File, bool IsSystem,
-                                            DirectoryEntryRef Dir,
-                                            FileID ID = FileID(),
-                                            unsigned *Offset = nullptr);
+  ModuleMapResult parseAndLoadModuleMapFileImpl(FileEntryRef File,
+                                                bool IsSystem,
+                                                DirectoryEntryRef Dir,
+                                                FileID ID = FileID(),
+                                                unsigned *Offset = nullptr);
+
+  ModuleMapResult parseModuleMapFileImpl(FileEntryRef File, bool IsSystem,
+                                         DirectoryEntryRef Dir,
+                                         FileID ID = FileID());
 
   /// Try to load the module map file in the given directory.
   ///
@@ -945,8 +960,8 @@ class HeaderSearch {
   ///
   /// \returns The result of attempting to load the module map file from the
   /// named directory.
-  LoadModuleMapResult loadModuleMapFile(StringRef DirName, bool IsSystem,
-                                        bool IsFramework);
+  ModuleMapResult parseAndLoadModuleMapFile(StringRef DirName, bool IsSystem,
+                                            bool IsFramework);
 
   /// Try to load the module map file in the given directory.
   ///
@@ -956,8 +971,13 @@ class HeaderSearch {
   ///
   /// \returns The result of attempting to load the module map file from the
   /// named directory.
-  LoadModuleMapResult loadModuleMapFile(DirectoryEntryRef Dir, bool IsSystem,
-                                        bool IsFramework);
+  ModuleMapResult parseAndLoadModuleMapFile(DirectoryEntryRef Dir,
+                                            bool IsSystem, bool IsFramework);
+
+  ModuleMapResult parseModuleMapFile(StringRef DirName, bool IsSystem,
+                                     bool IsFramework);
+  ModuleMapResult parseModuleMapFile(DirectoryEntryRef Dir, bool IsSystem,
+                                     bool IsFramework);
 };
 
 /// Apply the header search options to get given HeaderSearch object.

clang/include/clang/Lex/ModuleMap.h

@@ -18,6 +18,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/ModuleMapFile.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
@@ -262,6 +263,18 @@ class ModuleMap {
   /// Describes whether we haved loaded a particular file as a module
   /// map.
   llvm::DenseMap<const FileEntry *, bool> LoadedModuleMap;
+  llvm::DenseMap<const FileEntry *, const modulemap::ModuleMapFile *>
+      ParsedModuleMap;
+
+  std::vector<std::unique_ptr<modulemap::ModuleMapFile>> ParsedModuleMaps;
+
+  /// Map from top level module name to a list of ModuleDecls in the order they
+  /// were discovered. This allows handling shadowing correctly and diagnosing
+  /// redefinitions.
+  llvm::StringMap<SmallVector<std::pair<const modulemap::ModuleMapFile *,
+                                        const modulemap::ModuleDecl *>,
+                              1>>
+      ParsedModules;
 
   /// Resolve the given export declaration into an actual export
   /// declaration.
@@ -478,6 +491,8 @@ class ModuleMap {
   /// \returns The named module, if known; otherwise, returns null.
   Module *findModule(StringRef Name) const;
 
+  Module *findOrLoadModule(StringRef Name);
+
   Module *findOrInferSubmodule(Module *Parent, StringRef Name);
 
   /// Retrieve a module with the given name using lexical name lookup,
@@ -693,6 +708,11 @@ class ModuleMap {
   void addHeader(Module *Mod, Module::Header Header,
                  ModuleHeaderRole Role, bool Imported = false);
 
+  /// Parse a module map without creating `clang::Module` instances.
+  bool parseModuleMapFile(FileEntryRef File, bool IsSystem,
+                          DirectoryEntryRef Dir, FileID ID = FileID(),
+                          SourceLocation ExternModuleLoc = SourceLocation());
+
   /// Load the given module map file, and record any modules we
   /// encounter.
   ///
@@ -713,10 +733,11 @@ class ModuleMap {
   ///        that caused us to load this module map file, if any.
   ///
   /// \returns true if an error occurred, false otherwise.
-  bool loadModuleMapFile(FileEntryRef File, bool IsSystem,
-                         DirectoryEntryRef HomeDir, FileID ID = FileID(),
-                         unsigned *Offset = nullptr,
-                         SourceLocation ExternModuleLoc = SourceLocation());
+  bool
+  parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem,
+                            DirectoryEntryRef HomeDir, FileID ID = FileID(),
+                            unsigned *Offset = nullptr,
+                            SourceLocation ExternModuleLoc = SourceLocation());
 
   /// Dump the contents of the module map, for debugging purposes.
   void dump();

clang/include/clang/Lex/ModuleMapFile.h

@@ -133,8 +133,17 @@ using TopLevelDecl = std::variant<ModuleDecl, ExternModuleDecl>;
 /// This holds many reference types (StringRef, SourceLocation, etc.) whose
 /// lifetimes are bound by the SourceManager and FileManager used.
 struct ModuleMapFile {
+  /// The FileID used to parse this module map. This is always a local ID.
+  FileID ID;
+
+  /// The directory in which the module map was discovered. Declarations in
+  /// the module map are relative to this directory.
+  OptionalDirectoryEntryRef Dir;
+
   /// Beginning of the file, used for moduleMapFileRead callback.
   SourceLocation Start;
+
+  bool IsSystem;
   std::vector<TopLevelDecl> Decls;
 
   void dump(llvm::raw_ostream &out) const;

clang/lib/Frontend/CompilerInstance.cpp

@@ -582,13 +582,13 @@ struct ReadModuleNames : ASTReaderListener {
     ModuleMap &MM = PP.getHeaderSearchInfo().getModuleMap();
     for (const std::string &LoadedModule : LoadedModules)
       MM.cacheModuleLoad(*PP.getIdentifierInfo(LoadedModule),
-                         MM.findModule(LoadedModule));
+                         MM.findOrLoadModule(LoadedModule));
     LoadedModules.clear();
   }
 
   void markAllUnavailable() {
     for (const std::string &LoadedModule : LoadedModules) {
-      if (Module *M = PP.getHeaderSearchInfo().getModuleMap().findModule(
+      if (Module *M = PP.getHeaderSearchInfo().getModuleMap().findOrLoadModule(
               LoadedModule)) {
         M->HasIncompatibleModuleFile = true;
 

clang/lib/Frontend/FrontendAction.cpp

@@ -621,8 +621,8 @@ static bool loadModuleMapForModuleBuild(CompilerInstance &CI, bool IsSystem,
   }
 
   // Load the module map file.
-  if (HS.loadModuleMapFile(*ModuleMap, IsSystem, ModuleMapID, &Offset,
-                           PresumedModuleMapFile))
+  if (HS.parseAndLoadModuleMapFile(*ModuleMap, IsSystem, ModuleMapID, &Offset,
+                                   PresumedModuleMapFile))
     return true;
 
   if (SrcMgr.getBufferOrFake(ModuleMapID).getBufferSize() == Offset)
@@ -1077,8 +1077,8 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
   // If we were asked to load any module map files, do so now.
   for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
     if (auto File = CI.getFileManager().getOptionalFileRef(Filename))
-      CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
-          *File, /*IsSystem*/false);
+      CI.getPreprocessor().getHeaderSearchInfo().parseAndLoadModuleMapFile(
+          *File, /*IsSystem*/ false);
     else
       CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
   }