[PATCH] D23125: Modules: add command line option to support loading prebuilt modules on demand, without parsing any module map

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Lex/HeaderSearchOptions.h:96-97
@@ -95,1 +95,4 @@
 
+  /// \brief The directory used to load prebuilt module files.
+  std::string PrebuiltModulePath;
+
----------------
It would seem preferable to allow multiple of these, to support mixing (for instance) a path to prebuilt system modules and a path to some per-project modules. I'd be fine handling that as a separate patch / discussion if you prefer.

================
Comment at: lib/Frontend/CompilerInstance.cpp:1438-1450
@@ -1437,3 +1437,15 @@
     // Search for a module with the given name.
     Module = PP->getHeaderSearchInfo().lookupModule(ModuleName);
-    if (!Module) {
+    HeaderSearchOptions &HSOpts =
+        PP->getHeaderSearchInfo().getHeaderSearchOpts();
+
+    std::string ModuleFileName;
+    bool LoadFromPrebuiltModulePath = false;
+    if (Module) {
+      ModuleFileName = PP->getHeaderSearchInfo().getModuleFileName(Module);
+    } else if (!HSOpts.PrebuiltModulePath.empty()) {
+      // Try to load the module from the prebuilt module path.
+      ModuleFileName =
+          PP->getHeaderSearchInfo().getModuleFileName(ModuleName, "", true);
+      LoadFromPrebuiltModulePath = true;
+    } else {
----------------
It looks like the logic here is:

 * if we found the module in a module map, then look for it in the module cache
 * otherwise, if we have a prebuilt module path, then look for it there
 * otherwise, error

I think that may be surprising; if I explicitly load a module map, perhaps via `-fmodule-map-file=`, and supply a prebuilt form of that module in the prebuilt modules path, I'd expect my prebuilt form to still be used.

Perhaps instead we could first search the prebuilt module path for an existing .pcm file, and if that fails and we have a `Module` then look for it in the module cache?

================
Comment at: lib/Frontend/CompilerInstance.cpp:1497-1505
@@ +1496,11 @@
+      if (LoadFromPrebuiltModulePath && !Module) {
+        // Create a Module if we are using the prebuilt module path.
+        Module = PP->getHeaderSearchInfo().getModuleMap().findOrCreateModule(
+            ModuleName, nullptr, false/*IsFramework*/,
+            false/*IsExplicit*/).first;
+        // FIXME: Since we are mostly prebuilding system modules, we set
+        // IsSystem to true here. This is not always the correct choice,
+        // and IsSystem can affect diagnostics.
+        Module->IsSystem = true;
+        Module->IsFromModuleFile = true;
+      }
----------------
The module file will have provided a `Module` instance, if it is in fact a module file for the requested module. Instead of inventing a `Module` here, can you check that one exists and produce an error if not?

================
Comment at: lib/Serialization/ASTReader.cpp:2273
@@ -2271,2 +2272,3 @@
           if (DisableValidation ||
-              (AllowConfigurationMismatch && Result == ConfigurationMismatch))
+              ((AllowConfigurationMismatch || F.Kind == MK_PrebuiltModule) &&
+               Result == ConfigurationMismatch))
----------------
You're allowing configuration mismatches that we don't consider to be "compatible configuration mismatches" here. That seems really scary -- I believe this allows using a C++ module from C and similar unreasonable things, that will cause clang to crash. Did you really mean to do that?


https://reviews.llvm.org/D23125