[clang][modules] Don't depend on sharing FileManager during module build

[benlangmuir]

Summary

Sharing the FileManager between the importer and the module build should only be an optimization. Add a cc1 option -fno-modules-share-filemanager to allow us to test this. Fix the path to modulemap files, which previously depended on the shared FileManager when using path mapped to an external file in a VFS.

[bnbarham]

clang/test/ClangScanDeps/modulemap-via-vfs.m

45

Nitpick: ' vs " in the rest of the file, doesn't really matter though.

clang/test/Modules/submodule-in-private-mmap-vfs.m

39

Nitpick: no new line

[benlangmuir]

• Updates for review feedback
• Attempt to fix one of the Windows path issues - I'm just guessing based on what other VFS tests are doing. I don't know what the other windows failure is about, and probably need to debug it.

[benlangmuir]

Attempt to fix test failure seen on Windows. It revealed two bugs

• Avoid reusing the FileManager at the top-level in clang-scan-deps to make the test behave as expected
• Make the FileManager return the cached redirecting entry, to match how it behaves when it's a new file, and test this case explicitly.

I think it was mostly luck of exactly which file lookups happen that we didn't hit this on other platforms.

[benlangmuir]

Split out the FileManager change into https://reviews.llvm.org/D131273

clang/include/clang/Driver/Options.td

 def fallow_pch_with_errors : Flag<["-"], "fallow-pch-with-compiler-errors">,
   HelpText<"Accept a PCH file that was created with compiler errors">,
   MarshallingInfoFlag<PreprocessorOpts<"AllowPCHWithCompilerErrors">>,
   ImpliedByAnyOf<[fallow_pcm_with_errors.KeyPath]>;
 def fallow_pch_with_different_modules_cache_path :
   Flag<["-"], "fallow-pch-with-different-modules-cache-path">,
   HelpText<"Accept a PCH file that was created with a different modules cache path">,
   MarshallingInfoFlag<PreprocessorOpts<"AllowPCHWithDifferentModulesCachePath">>;
+def fno_modules_share_filemanager : Flag<["-"], "fno-modules-share-filemanager">,
+  HelpText<"Disable sharing the FileManager when building a module implicitly">,
+  MarshallingInfoNegativeFlag<FrontendOpts<"ModulesShareFileManager">>;
 def dump_deserialized_pch_decls : Flag<["-"], "dump-deserialized-decls">,
   HelpText<"Dump declarations that are deserialized from PCH, for testing">,
   MarshallingInfoFlag<PreprocessorOpts<"DumpDeserializedPCHDecls">>;
 def error_on_deserialized_pch_decl : Separate<["-"], "error-on-deserialized-decl">,
   HelpText<"Emit error if a specific declaration is deserialized from PCH, for testing">;
 def error_on_deserialized_pch_decl_EQ : Joined<["-"], "error-on-deserialized-decl=">,
   Alias<error_on_deserialized_pch_decl>;
 def static_define : Flag<["-"], "static-define">,

clang/include/clang/Frontend/FrontendOptions.h

public:
   /// out or not. Consider either removing that record entirely if it's no
   /// longer relevant or switching the default to not write it unless an option
   /// is set to true.
   unsigned OutputPathIndependentPCM : 1;
 
   /// Output (and read) PCM files regardless of compiler errors.
   unsigned AllowPCMWithCompilerErrors : 1;
 
+  /// Whether to share the FileManager when building modules.
+  unsigned ModulesShareFileManager : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
   ASTDumpOutputFormat ASTDumpFormat = ADOF_Default;
 
   enum {
     ARCMT_None,
     ARCMT_Check,
FrontendOptions()
         FixWhatYouCan(false), FixOnlyWarnings(false), FixAndRecompile(false),
         FixToTemporaries(false), ARCMTMigrateEmitARCErrors(false),
         SkipFunctionBodies(false), UseGlobalModuleIndex(true),
         GenerateGlobalModuleIndex(true), ASTDumpDecls(false),
         ASTDumpLookups(false), BuildingImplicitModule(false),
         BuildingImplicitModuleUsesLock(true), ModulesEmbedAllFiles(false),
         IncludeTimestamps(true), UseTemporary(true),
         OutputPathIndependentPCM(false), AllowPCMWithCompilerErrors(false),
-        TimeTraceGranularity(500) {}
+        ModulesShareFileManager(true), TimeTraceGranularity(500) {}
 
   /// getInputKindForExtension - Return the appropriate input kind for a file
   /// extension. For example, "c" would return Language::C.
   ///
   /// \return The input kind for the extension, or Language::Unknown if the
   /// extension is not recognized.
   static InputKind getInputKindForExtension(StringRef Extension);
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_FRONTENDOPTIONS_H

clang/lib/Frontend/CompilerInstance.cpp

CompilerInstance Instance(ImportingInstance.getPCHContainerOperations(),
                             &ImportingInstance.getModuleCache());
   auto &Inv = *Invocation;
   Instance.setInvocation(std::move(Invocation));
 
   Instance.createDiagnostics(new ForwardingDiagnosticConsumer(
                                    ImportingInstance.getDiagnosticClient()),
                              /*ShouldOwnClient=*/true);
 
-  // Note that this module is part of the module build stack, so that we
-  // can detect cycles in the module graph.
+  if (FrontendOpts.ModulesShareFileManager) {
     Instance.setFileManager(&ImportingInstance.getFileManager());
+  } else {
+    Instance.createFileManager(&ImportingInstance.getVirtualFileSystem());
+  }
   Instance.createSourceManager(Instance.getFileManager());
   SourceManager &SourceMgr = Instance.getSourceManager();
+
+  // Note that this module is part of the module build stack, so that we
+  // can detect cycles in the module graph.
   SourceMgr.setModuleBuildStack(
     ImportingInstance.getSourceManager().getModuleBuildStack());
   SourceMgr.pushModuleBuildStack(ModuleName,
     FullSourceLoc(ImportLoc, ImportingInstance.getSourceManager()));
 
   // If we're collecting module dependencies, we need to share a collector
   // between all of the module CompilerInstances. Other than that, we don't
   // want to produce any dependency output from the module build.
if (const FileEntry *ModuleMapFile =
           ModMap.getContainingModuleMapFile(Module)) {
     // Canonicalize compilation to start with the public module map. This is
     // vital for submodules declarations in the private module maps to be
     // correctly parsed when depending on a top level module in the public one.
     if (const FileEntry *PublicMMFile = getPublicModuleMap(
             ModuleMapFile, ImportingInstance.getFileManager()))
       ModuleMapFile = PublicMMFile;
 
+    // FIXME: Update header search to keep FileEntryRef rather than rely on
+    // getLastRef().
+    StringRef ModuleMapFilePath =
+        ModuleMapFile->getLastRef().getNameAsRequested();
+
     // Use the module map where this module resides.
     Result = compileModuleImpl(
         ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
-        FrontendInputFile(ModuleMapFile->getName(), IK, +Module->IsSystem),
-        ModMap.getModuleMapFileForUniquing(Module)->getName(),
-        ModuleFileName);
+        FrontendInputFile(ModuleMapFilePath, IK, +Module->IsSystem),
+        ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
   } else {
     // FIXME: We only need to fake up an input file here as a way of
     // transporting the module's directory to the module map parser. We should
     // be able to do that more directly, and parse from a memory buffer without
     // inventing this file.
     SmallString<128> FakeModuleMapFile(Module->Directory->getName());
     llvm::sys::path::append(FakeModuleMapFile, "__inferred_module.map");
 

clang/test/ClangScanDeps/modulemap-via-vfs.m

 // RUN: rm -rf %t.dir
 // RUN: split-file %s %t.dir
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/compile-commands.json.in > %t.dir/build/compile-commands.json
 // RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml
-// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format experimental-full \
+// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json \
+// RUN:   -reuse-filemanager=0 -j 1 -format experimental-full \
 // RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t.db
 // RUN: %deps-to-rsp %t.db --module-name=A > %t.A.cc1.rsp
 // RUN: cat %t.A.cc1.rsp | sed 's:\\\\\?:/:g' | FileCheck %s
 
 // CHECK-NOT: build/module.modulemap
 // CHECK: A/module.modulemap
+// CHECK-NOT: build/module.modulemap
 
 //--- build/compile-commands.json.in
 
 [
 {
   "directory": "DIR",
   "command": "clang DIR/main.m -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
   "file": "DIR/main.m"
+},
+{
+  "directory": "DIR",
+  "command": "clang DIR/main.m -Imodules/A -fmodules -Xclang -fno-modules-share-filemanager -fmodules-cache-path=DIR/module-cache-unshared -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
+  "file": "DIR/main.m"
 }
 ]
 
 //--- build/module.modulemap
 
 module A {
   umbrella header "A.h"
 }
 
 //--- modules/A/A.h
 
 typedef int A_t;
 
 //--- build/vfs.yaml.in
 
 {
   "version": 0,
   "case-sensitive": "false",
+  "use-external-names": true,
   "roots": [
   {
      "contents": [
      {
         "external-contents": "DIR/build/module.modulemap",
         "name": "module.modulemap",
         "type": "file"
      }],

clang/test/Modules/submodule-in-private-mmap-vfs.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t -I%t/Virtual -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+  'version': 0,
+  'use-external-names': true,
+  'roots': [
+    {
+      'name': 'DIR/Virtual',
+      'type': 'directory',
+      'contents': [
+        {
+          'name': 'module.modulemap',
+          'type': 'file',
+          'external-contents': 'DIR/Dir1/module.modulemap'
+        },
+        {
+          'name': 'module.private.modulemap',
+          'type': 'file',
+          'external-contents': 'DIR/Dir2/module.private.modulemap'
+        }
+      ]
+    }
+  ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics

clang/test/VFS/module-import.m

-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
 
 @import not_real;
 
 void foo(void) {
   bar();
 }
 
 // Import a submodule that is defined in actual_module2.map, which is only
 // mapped in vfsoverlay2.yaml.
 #ifdef IMPORT2
 @import not_real.from_second_module;
 // CHECK-VFS2: error: no submodule
 #endif
 
 // Override the module map (vfsoverlay2 on top)
 // RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
 // RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
 
 // vfsoverlay2 not present
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
 
 // vfsoverlay2 on the bottom
 // RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s