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

[Manman Ren via cfe-commits]

manmanren added a comment.

Thanks,
Manman


================
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;
+
----------------
rsmith wrote:
> 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.
It sounds reasonable to provide prebuilt system modules and prebuilt project modules. I assume via the same command-line option -fprebuilt-module-path.

================
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 {
----------------
rsmith wrote:
> 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?
I was wondering about the priority of loading from prebuilt module path vs. loading from module cache. Your suggestion makes sense.

================
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;
+      }
----------------
rsmith wrote:
> 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?
This is nice to know. I didn't realize we construct a Module when calling ReadAST. Can you be more specific on how to check if a Module exists?

================
Comment at: lib/Serialization/ASTReader.cpp:2273
@@ -2271,2 +2272,3 @@
           if (DisableValidation ||
-              (AllowConfigurationMismatch && Result == ConfigurationMismatch))
+              ((AllowConfigurationMismatch || F.Kind == MK_PrebuiltModule) &&
+               Result == ConfigurationMismatch))
----------------
rsmith wrote:
> 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?
I originally had this in my testing patch to be really relaxing on prebuilt modules. I will remove this :]


https://reviews.llvm.org/D23125