[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/lib/Frontend/CompilerInstance.cpp

 
 // ExternalASTSource
 
 namespace {
 // Helper to recursively read the module names for all modules we're adding.
 // We mark these as known and redirect any attempt to load that module to
 // the files we were handed.
 struct ReadModuleNames : ASTReaderListener {
-  CompilerInstance &CI;
+  Preprocessor &PP;
   llvm::SmallVector<IdentifierInfo*, 8> LoadedModules;
 
-  ReadModuleNames(CompilerInstance &CI) : CI(CI) {}
+  ReadModuleNames(Preprocessor &PP) : PP(PP) {}
 
   void ReadModuleName(StringRef ModuleName) override {
-    LoadedModules.push_back(
-        CI.getPreprocessor().getIdentifierInfo(ModuleName));
+    LoadedModules.push_back(PP.getIdentifierInfo(ModuleName));
   }
 
   void registerAll() {
-    ModuleMap &MM = CI.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+    ModuleMap &MM = PP.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())) {
+      if (Module *M = PP.getHeaderSearchInfo().getModuleMap().findModule(
+              II->getName())) {
         M->HasIncompatibleModuleFile = true;
 
         // Mark module as available if the only reason it was unavailable
         // was missing headers.
         SmallVector<Module *, 2> Stack;
         Stack.push_back(M);
         while (!Stack.empty()) {
           Module *Current = Stack.pop_back_val();
IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(
 
   Reader->setDeserializationListener(
       static_cast<ASTDeserializationListener *>(DeserializationListener),
       /*TakeOwnership=*/OwnDeserializationListener);
 
   for (auto &Listener : DependencyCollectors)
     Listener->attachToASTReader(*Reader);
 
+  auto Listener = std::make_unique<ReadModuleNames>(PP);
+  auto &ListenerRef = *Listener;
+  ASTReader::ListenerScope ReadModuleNamesListener(*Reader,
+                                                   std::move(Listener));
+
   switch (Reader->ReadAST(Path,
                           Preamble ? serialization::MK_Preamble
                                    : serialization::MK_PCH,
                           SourceLocation(),
                           ASTReader::ARR_None)) {
   case ASTReader::Success:
     // Set the predefines buffer as suggested by the PCH reader. Typically, the
     // predefines buffer will be empty.
     PP.setPredefines(Reader->getSuggestedPredefines());
+    ListenerRef.registerAll();
     return Reader;
 
   case ASTReader::Failure:
     // Unrecoverable failure: don't even try to process the input file.
     break;
 
   case ASTReader::Missing:
   case ASTReader::OutOfDate:
   case ASTReader::VersionMismatch:
   case ASTReader::ConfigurationMismatch:
   case ASTReader::HadErrors:
     // No suitable PCH file could be found. Return an error.
     break;
   }
 
+  ListenerRef.markAllUnavailable();
   Context.setExternalSource(nullptr);
   return nullptr;
 }
 
 // Code Completion
 
 static bool EnableCodeCompletion(Preprocessor &PP,
                                  StringRef Filename,
bool CompilerInstance::loadModuleFile(StringRef FileName) {
 
   // 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 Listener = std::make_unique<ReadModuleNames>(*PP);
   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)) {

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