[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 inline comments.

================
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 {
----------------
bruno wrote:
> manmanren wrote:
> > 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.
> +1 to Richard's suggestion here.
> 
> I wonder about the workflow on generating such modules. Suppose I want to build some modules to re-use later with -fprebuilt-module-path=. RIght now, we get a module cache with hash in the names unless we disable hash computation via cc1 option -fdisable-module-hash. Should we assume that the user will have to copy out the cache dir and rename the modules OR that it will use -fdisable-module-hash instead? Should this option also try to read out modules with a hash in the name? Should we promote -fdisable-module-hash to a driver option as well?
> 
> @Manman, do you have any workflow in mind? @Richard, do you have any feedback in this respect?
> 
> In the crash reproducer we often have to rebuild the modules when running the script in a different machine because of the paths used to hash the modules, it would be nice (depending on the resulting design) if the crash reproducer could use -fprebuilt-module-path in order to try to re-use the pre-built modules resulting from the crash. 
If the modules are built implicitly with module hash, to load the module files as prebuilt modules, the user needs to copy out the cache dir and rename the modules. The user can choose to emit the modules explicitly or disable module hash when building the modules.

================
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;
+      }
----------------
manmanren wrote:
> 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?
I assumed ReadAST only constructed Module instances for submodules, because it is inside ReadSubmoduleBlock. It turns out it also constructed Module instance for the top level module. I will update the patch.


https://reviews.llvm.org/D23125