[clang][modules] Cache loads of modules imported by PCH

[jansvoboda11]

Summary

During explicit modular build, PCM files are typically specified via the -fmodule-file=<path> command-line option. Early during the compilation, Clang uses the ASTReader to read their contents and caches the result so that the module isn't loaded implicitly later on. A listener is attached to the ASTReader to collect names of the modules read from the PCM files. However, if the PCM has already been loaded previously via PCH:

1. the ASTReader doesn't do anything for the second time,
2. the listener is not invoked at all,
3. the module load result is not cached,
4. the compilation fails when attempting to load the module implicitly later on.

This patch solves this problem by attaching the listener to the ASTReader for PCH reading as well.

[jansvoboda11]

Note: Another approach to fixing this might be to cache the module load results while loading the PCH too.

[dexonsmith]

In D111560#3055578, @jansvoboda11 wrote:

Note: Another approach to fixing this might be to cache the module load results while loading the PCH too.

Can you share why you chose this approach instead, and which do you think makes sense long term?

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

41

Why drop the <name>= argument? Isn't it better to keep that information, rather than forcing the reader to crack open the file to know what's inside?

[jansvoboda11]

In D111560#3056021, @dexonsmith wrote:

In D111560#3055578, @jansvoboda11 wrote:

Note: Another approach to fixing this might be to cache the module load results while loading the PCH too.

Can you share why you chose this approach instead, and which do you think makes sense long term?

I assumed there was a reason this is not being done for PCHs that I can't see.

If that idea seems workable to you, I can give it a try and see if it leads to cleaner code. Conceptually, I think it would make sense to treat both PCHs and PCMs the same in this regard.

It would be nice to have @rsmith's opinion, since I think he originally implemented this.

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

41

I think it's nice to be consistent. We already pass PCM files in the -fmodule-file=<path> form for:

• all dependencies when precompiling a module,
• non-PCH dependencies dependencies when compiling a TU.

The -fmodule-file=<name>=<path> format is handled lazily (done via header search), which might be less efficient (see 0f99d6a4413e3da6ec242c0ab715d6699045dea8). We don't need laziness, since we know we'll need to open the file anyways.

I'm open to revising this decision later as part of performance tuning, but for now, I'd like this to be consistent across the board.

Bonus points: it forces me to fix a Clang bug.

WDYT?

[dexonsmith]

In D111560#3056175, @jansvoboda11 wrote:

In D111560#3056021, @dexonsmith wrote:

In D111560#3055578, @jansvoboda11 wrote:

Note: Another approach to fixing this might be to cache the module load results while loading the PCH too.

Can you share why you chose this approach instead, and which do you think makes sense long term?

I assumed there was a reason this is not being done for PCHs that I can't see.

If that idea seems workable to you, I can give it a try and see if it leads to cleaner code. Conceptually, I think it would make sense to treat both PCHs and PCMs the same in this regard.

I can't think of a reason not to do it but maybe I'm not being imaginative enough. I tend to suspect differences between PCH and PCM are incidental, due to flagging interest in PCH once PCMs started to work.

It would be nice to have @rsmith's opinion, since I think he originally implemented this.

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

41

Okay, SGTM.

[jansvoboda11]

Implemented alternative approach: attach the caching listener to PCH reading as well.

[jansvoboda11]

I implemented the alternative approach, which now seems more straightforward than going through the ModuleManager and working with serialization::ModuleFile.
WDYT @dexonsmith?

[dexonsmith]

LGTM. It'd be nice to split out the move of ReadModuleNames into an NFC prep patch for a cleaner commit history before pushing, but up to you.

[jansvoboda11]

In D111560#3059128, @dexonsmith wrote:

LGTM. It'd be nice to split out the move of ReadModuleNames into an NFC prep patch for a cleaner commit history before pushing, but up to you.

Committed in prep patch: aae776a5341ccf90a2c0a4e2e952ae4ec5ffb422.

clang/include/clang/Serialization/ModuleFile.h

public:
   bool isDirectlyImported() const { return DirectlyImported; }
 
   /// Is this a module file for a module (rather than a PCH or similar).
   bool isModule() const {
     return Kind == MK_ImplicitModule || Kind == MK_ExplicitModule ||
            Kind == MK_PrebuiltModule;
   }
 
+  /// Collect all transitive imports of this module file.
+  void collectTransitiveImports(std::vector<ModuleFile *> &Files);
+
   /// Dump debugging output for this module.
   void dump();
 };
 
 } // namespace serialization
 
 } // namespace clang
 
 #endif // LLVM_CLANG_SERIALIZATION_MODULEFILE_H

clang/lib/Frontend/CompilerInstance.cpp

struct ReadModuleNames : ASTReaderListener {
 
     ReadModuleNames(CompilerInstance &CI) : CI(CI) {}
 
     void ReadModuleName(StringRef ModuleName) override {
       LoadedModules.push_back(
           CI.getPreprocessor().getIdentifierInfo(ModuleName));
     }
 
-    void registerAll() {
-      ModuleMap &MM = CI.getPreprocessor().getHeaderSearchInfo().getModuleMap();
-      for (auto *II : LoadedModules)
-        MM.cacheModuleLoad(*II, MM.findModule(II->getName()));
-      LoadedModules.clear();
-    }
-
     void markAllUnavailable() {
       for (auto *II : LoadedModules) {
         if (Module *M = CI.getPreprocessor()
                             .getHeaderSearchInfo()
                             .getModuleMap()
                             .findModule(II->getName())) {
           M->HasIncompatibleModuleFile = true;
 
void markAllUnavailable() {
                          Current->submodule_begin(), Current->submodule_end());
           }
         }
       }
       LoadedModules.clear();
     }
   };
 
+  auto RegisterAll = [&](serialization::ModuleFile *TopMF) {
+    std::vector<serialization::ModuleFile *> AllMFs{TopMF};
+    TopMF->collectTransitiveImports(AllMFs);
+    for (serialization::ModuleFile *MF : AllMFs) {
+      ModuleMap &MM = getPreprocessor().getHeaderSearchInfo().getModuleMap();
+      MM.cacheModuleLoad(*getPreprocessor().getIdentifierInfo(MF->ModuleName),
+                         MM.findModule(MF->ModuleName));
+    }
+  };
+
   // If we don't already have an ASTReader, create one now.
   if (!TheASTReader)
     createASTReader();
 
   // If -Wmodule-file-config-mismatch is mapped as an error or worse, allow the
   // ASTReader to diagnose it, since it can produce better errors that we can.
   bool ConfigMismatchIsRecoverable =
       getDiagnostics().getDiagnosticLevel(diag::warn_module_config_mismatch,
                                           SourceLocation())
         <= DiagnosticsEngine::Warning;
 
   auto Listener = std::make_unique<ReadModuleNames>(*this);
   auto &ListenerRef = *Listener;
   ASTReader::ListenerScope ReadModuleNamesListener(*TheASTReader,
                                                    std::move(Listener));
 
   // Try to load the module file.
   switch (TheASTReader->ReadAST(
       FileName, serialization::MK_ExplicitModule, SourceLocation(),
       ConfigMismatchIsRecoverable ? ASTReader::ARR_ConfigurationMismatch : 0)) {
   case ASTReader::Success:
     // We successfully loaded the module file; remember the set of provided
     // modules so that we don't try to load implicit modules for them.
-    ListenerRef.registerAll();
+    RegisterAll(TheASTReader->getModuleManager().lookupByFileName(FileName));
     return true;
 
   case ASTReader::ConfigurationMismatch:
     // Ignore unusable module files.
     getDiagnostics().Report(SourceLocation(), diag::warn_module_config_mismatch)
         << FileName;
     // All modules provided by any files we tried and failed to load are now
     // unavailable; includes of those modules should now be handled textually.

clang/lib/Serialization/ModuleFile.cpp

 using namespace reader;
 
 ModuleFile::~ModuleFile() {
   delete static_cast<ASTIdentifierLookupTable *>(IdentifierLookupTable);
   delete static_cast<HeaderFileInfoLookupTable *>(HeaderFileInfoTable);
   delete static_cast<ASTSelectorLookupTable *>(SelectorLookupTable);
 }
 
+void ModuleFile::collectTransitiveImports(std::vector<ModuleFile *> &Files) {
+  SmallVector<serialization::ModuleFile *, 4> Stack{this};
+  while (!Stack.empty()) {
+    serialization::ModuleFile *MF = Stack.pop_back_val();
+    Stack.insert(Stack.end(), MF->Imports.begin(), MF->Imports.end());
+    Files.push_back(MF);
+  }
+}
+
 template<typename Key, typename Offset, unsigned InitialCapacity>
 static void
 dumpLocalRemap(StringRef Name,
                const ContinuousRangeMap<Key, Offset, InitialCapacity> &Map) {
   if (Map.begin() == Map.end())
     return;
 
   using MapType = ContinuousRangeMap<Key, Offset, InitialCapacity>;

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

 std::vector<std::string>
 FullDependencies::getAdditionalArgsWithoutModulePaths() const {
   std::vector<std::string> Args{
       "-fno-implicit-modules",
       "-fno-implicit-module-maps",
   };
 
   for (const PrebuiltModuleDep &PMD : PrebuiltModuleDeps) {
-    Args.push_back("-fmodule-file=" + PMD.ModuleName + "=" + PMD.PCMFile);
+    Args.push_back("-fmodule-file=" + PMD.PCMFile);
     Args.push_back("-fmodule-map-file=" + PMD.ModuleMapFile);
   }
 
   return Args;
 }
 
 DependencyScanningTool::DependencyScanningTool(
     DependencyScanningService &Service)

clang/test/ClangScanDeps/modules-pch.c

 // CHECK-TU-WITH-COMMON-NEXT:         {
 // CHECK-TU-WITH-COMMON-NEXT:           "context-hash": "[[HASH_MOD_TU_WITH_COMMON]]",
 // CHECK-TU-WITH-COMMON-NEXT:           "module-name": "ModTUWithCommon"
 // CHECK-TU-WITH-COMMON-NEXT:         }
 // CHECK-TU-WITH-COMMON-NEXT:       ],
 // CHECK-TU-WITH-COMMON-NEXT:       "command-line": [
 // CHECK-TU-WITH-COMMON-NEXT:         "-fno-implicit-modules",
 // CHECK-TU-WITH-COMMON-NEXT:         "-fno-implicit-module-maps",
-// FIXME: Figure out why we need `=ModCommon2` here for Clang to pick up the PCM.
-// CHECK-TU-WITH-COMMON-NEXT:         "-fmodule-file=ModCommon2=[[PREFIX]]/build/{{.*}}/ModCommon2-{{.*}}.pcm",
+// CHECK-TU-WITH-COMMON-NEXT:         "-fmodule-file=[[PREFIX]]/build/{{.*}}/ModCommon2-{{.*}}.pcm",
 // CHECK-TU-WITH-COMMON-NEXT:         "-fmodule-map-file=[[PREFIX]]/module.modulemap"
 // CHECK-TU-WITH-COMMON-NEXT:         "-fmodule-file=[[PREFIX]]/build/[[HASH_MOD_TU_WITH_COMMON]]/ModTUWithCommon-{{.*}}.pcm",
 // CHECK-TU-WITH-COMMON-NEXT:         "-fmodule-map-file=[[PREFIX]]/module.modulemap"
 // CHECK-TU-WITH-COMMON-NEXT:       ],
 // CHECK-TU-WITH-COMMON-NEXT:       "file-deps": [
 // CHECK-TU-WITH-COMMON-NEXT:         "[[PREFIX]]/tu_with_common.c",
 // CHECK-TU-WITH-COMMON-NEXT:         "[[PREFIX]]/pch.h.gch"
 // CHECK-TU-WITH-COMMON-NEXT:       ],

clang/test/Modules/Inputs/pch-shared-module/module.modulemap

+module mod { header "mod.h" }

clang/test/Modules/Inputs/pch-shared-module/pch.h

+#include "mod.h"

clang/test/Modules/pch-shared-module.c

+// rm -rf %t && mkdir %t
+
+// RUN: %clang_cc1 -fmodules -emit-module -fmodule-name=mod %S/Inputs/pch-shared-module/module.modulemap -o %t/mod.pcm
+
+// RUN: %clang_cc1 -fmodules -emit-pch %S/Inputs/pch-shared-module/pch.h -o %t/pch.h.gch \
+// RUN:   -fmodule-file=%t/mod.pcm -fmodule-map-file=%S/Inputs/pch-shared-module/module.modulemap
+
+// Check that `mod.pcm` is loaded correctly, even though it's imported by the PCH as well.
+// RUN: %clang_cc1 -fmodules -fsyntax-only %s -include-pch %t/pch.h.gch -I %S/Inputs/pch-shared-module \
+// RUN:   -fmodule-file=%t/mod.pcm -fmodule-map-file=%S/Inputs/pch-shared-module/module.modulemap -verify
+
+#include "mod.h"
+
+// expected-no-diagnostics