Frontend: Add -f{,no-}implicit-modules-uses-lock and -Rmodule-lock

[dexonsmith]

Summary

Add -cc1 flags -fmodules-uses-lock and -fno-modules-uses-lock to
allow the lock manager to be turned off when building implicit modules.

Add -Rmodule-lock so that we can see when it's being used.

[jansvoboda11]

Left a question in-line.

clang/test/Modules/implicit-modules-use-lock.m

22

Where is the empty remark coming from? Is it useful to us in any way?

[dexonsmith]

clang/test/Modules/implicit-modules-use-lock.m

22

This is a CHECK-NOT: <match> line (with a custom -check-prefix). It confirms that <match> does not occur at least until the next positive check matches. The FileCheck invocation that uses CHECK-NO-LOCKS has no other check lines, so this really means: "remark: does not match on any line". Note I also needed to add -allow-empty since with no diagnostics at all, FileCheck's stdin will be empty.

FYI, if you want to read more:

• -check-prefix is documented at https://www.llvm.org/docs/CommandGuide/FileCheck.html#the-filecheck-check-prefix-option
• CHECK-NOT is documented at https://www.llvm.org/docs/CommandGuide/FileCheck.html#the-check-not-directive

Often in clang tests it's easier to use -cc1 -verify for diagnostics instead of manual FileChecks (expected-no-diagnostics comment being the equivalent of -allow-empty). In the modules testcases, there's often a complicated setup that we want to run a lot of RUN: lines against where each one expects different diagnostics. Maybe we should add prefix support to -verify (or maybe it's there and no one told me...).

[jansvoboda11]

LGTM

clang/test/Modules/implicit-modules-use-lock.m

22

Ah, that makes sense, thanks!

[arichardson]

clang/test/Modules/implicit-modules-use-lock.m

22

This could use e.g. -verify=lock and -verify=lock to avoid the Filecheck usage.

[dexonsmith]

clang/test/Modules/implicit-modules-use-lock.m

22

Thanks! Fixed in d8a08fae0af9aa09c108b3e7c60f192249dd2098.

clang/include/clang/Basic/DiagnosticFrontendKinds.td

def warn_module_config_macro_undef : Warning<
   "to configure the module">,
   InGroup<ConfigMacros>;
 def note_module_def_undef_here : Note<
   "macro was %select{defined|#undef'd}0 here">;
 def remark_module_build : Remark<"building module '%0' as '%1'">,
   InGroup<ModuleBuild>;
 def remark_module_build_done : Remark<"finished building module '%0'">,
   InGroup<ModuleBuild>;
+def remark_module_lock : Remark<"locking '%0' to build module '%1'">,
+  InGroup<ModuleLock>;
 def err_modules_embed_file_not_found :
   Error<"file '%0' specified by '-fmodules-embed-file=' not found">,
   DefaultFatal;
 def err_module_header_file_not_found :
   Error<"module header file '%0' not found">, DefaultFatal;
 
 def err_test_module_file_extension_version : Error<
   "test module file extension '%0' has different version (%1.%2) than expected "

clang/include/clang/Basic/DiagnosticGroups.td

 def CXX11LongLong : DiagGroup<"c++11-long-long">;
 def LongLong : DiagGroup<"long-long", [CXX11LongLong]>;
 def ImplicitlyUnsignedLiteral : DiagGroup<"implicitly-unsigned-literal">;
 def MethodSignatures : DiagGroup<"method-signatures">;
 def MismatchedParameterTypes : DiagGroup<"mismatched-parameter-types">;
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
+def ModuleLock : DiagGroup<"module-lock">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleImport : DiagGroup<"module-import">;
 def ModuleConflict : DiagGroup<"module-conflict">;
 def ModuleFileExtension : DiagGroup<"module-file-extension">;
 def RoundTripCC1Args : DiagGroup<"round-trip-cc1-args">;
 def NewlineEOF : DiagGroup<"newline-eof">;
 def Nullability : DiagGroup<"nullability">;
 def NullabilityDeclSpec : DiagGroup<"nullability-declspec">;

clang/include/clang/Driver/Options.td

def fmodules_embed_file_EQ : Joined<["-"], "fmodules-embed-file=">,
   MetaVarName<"<file>">,
   HelpText<"Embed the contents of the specified file into the module file "
            "being compiled.">,
   MarshallingInfoStringVector<FrontendOpts<"ModulesEmbedFiles">>;
 def fmodules_embed_all_files : Joined<["-"], "fmodules-embed-all-files">,
   HelpText<"Embed the contents of all files read by this compilation into "
            "the produced module file.">,
   MarshallingInfoFlag<FrontendOpts<"ModulesEmbedAllFiles">>;
+defm fimplicit_modules_use_lock : BoolOption<"f", "implicit-modules-use-lock",
+  FrontendOpts<"BuildingImplicitModuleUsesLock">, DefaultTrue,
+  NegFlag<SetFalse>,
+  PosFlag<SetTrue, [],
+          "Use filesystem locks for implicit modules builds to avoid "
+          "duplicating work in competing clang invocations.">>;
 // FIXME: We only need this in C++ modules / Modules TS if we might textually
 // enter a different module (eg, when building a header unit).
 def fmodules_local_submodule_visibility :
   Flag<["-"], "fmodules-local-submodule-visibility">,
   HelpText<"Enforce name visibility rules across submodules of the same "
            "top-level module.">,
   MarshallingInfoFlag<LangOpts<"ModulesLocalVisibility">>,
   ImpliedByAnyOf<[fmodules_ts.KeyPath, fcxx_modules.KeyPath]>;

clang/include/clang/Frontend/FrontendOptions.h

public:
   unsigned ASTDumpLookups : 1;
 
   /// Whether we include declaration type dumps in AST dumps.
   unsigned ASTDumpDeclTypes : 1;
 
   /// Whether we are performing an implicit module build.
   unsigned BuildingImplicitModule : 1;
 
+  /// Whether to use a filesystem lock when building implicit modules.
+  unsigned BuildingImplicitModuleUsesLock : 1;
+
   /// Whether we should embed all used files into the PCM file.
   unsigned ModulesEmbedAllFiles : 1;
 
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
   /// Should a temporary file be used during compilation.
   unsigned UseTemporary : 1;
public:
   FrontendOptions()
       : DisableFree(false), RelocatablePCH(false), ShowHelp(false),
         ShowStats(false), TimeTrace(false), ShowVersion(false),
         FixWhatYouCan(false), FixOnlyWarnings(false), FixAndRecompile(false),
         FixToTemporaries(false), ARCMTMigrateEmitARCErrors(false),
         SkipFunctionBodies(false), UseGlobalModuleIndex(true),
         GenerateGlobalModuleIndex(true), ASTDumpDecls(false),
         ASTDumpLookups(false), BuildingImplicitModule(false),
-        ModulesEmbedAllFiles(false), IncludeTimestamps(true),
-        UseTemporary(true), AllowPCMWithCompilerErrors(false),
-        TimeTraceGranularity(500) {}
+        BuildingImplicitModuleUsesLock(true), ModulesEmbedAllFiles(false),
+        IncludeTimestamps(true), UseTemporary(true),
+        AllowPCMWithCompilerErrors(false), 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

 /// multiple instances will compete to create the same module.  On timeout,
 /// deletes the lock file in order to avoid deadlock from crashing processes or
 /// bugs in the lock file manager.
 static bool compileModuleAndReadASTBehindLock(
     CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
     SourceLocation ModuleNameLoc, Module *Module, StringRef ModuleFileName) {
   DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
 
+  Diags.Report(ModuleNameLoc, diag::remark_module_lock)
+      << ModuleFileName << Module->Name;
+
   // FIXME: have LockFileManager return an error_code so that we can
   // avoid the mkdir when the directory already exists.
   StringRef Dir = llvm::sys::path::parent_path(ModuleFileName);
   llvm::sys::fs::create_directories(Dir);
 
   while (1) {
     llvm::LockFileManager Locked(ModuleFileName);
     switch (Locked) {
if (!OutOfDate)
       return false;
 
     // The module may be out of date in the presence of file system races,
     // or if one of its imports depends on header search paths that are not
     // consistent with this ImportingInstance.  Try again...
   }
 }
 
+/// Compile a module in a separate compiler instance and read the AST,
+/// returning true if the module compiles without errors, potentially using a
+/// lock manager to avoid building the same module in multiple compiler
+/// instances.
+static bool compileModuleAndReadAST(CompilerInstance &ImportingInstance,
+                                    SourceLocation ImportLoc,
+                                    SourceLocation ModuleNameLoc,
+                                    Module *Module, StringRef ModuleFileName) {
+  return ImportingInstance.getInvocation()
+                 .getFrontendOpts()
+                 .BuildingImplicitModuleUsesLock
+             ? compileModuleAndReadASTBehindLock(ImportingInstance, ImportLoc,
+                                                 ModuleNameLoc, Module,
+                                                 ModuleFileName)
+             : compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
+                                           ModuleNameLoc, Module,
+                                           ModuleFileName);
+}
+
 /// Diagnose differences between the current definition of the given
 /// configuration macro and the definition provided on the command line.
 static void checkConfigMacro(Preprocessor &PP, StringRef ConfigMacro,
                              Module *Mod, SourceLocation ImportLoc) {
   IdentifierInfo *Id = PP.getIdentifierInfo(ConfigMacro);
   SourceManager &SourceMgr = PP.getSourceManager();
 
   // If this identifier has never had a macro definition, then it could
ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
   if (getPreprocessorOpts().FailedModules &&
       getPreprocessorOpts().FailedModules->hasAlreadyFailed(ModuleName)) {
     getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_built)
         << ModuleName << SourceRange(ImportLoc, ModuleNameLoc);
     return nullptr;
   }
 
   // Try to compile and then read the AST.
-  if (!compileModuleAndReadASTBehindLock(*this, ImportLoc, ModuleNameLoc, M,
-                                         ModuleFilename)) {
+  if (!compileModuleAndReadAST(*this, ImportLoc, ModuleNameLoc, M,
     assert(getDiagnostics().hasErrorOccurred() &&
            "undiagnosed error in compileModuleAndReadAST");
     if (getPreprocessorOpts().FailedModules)
       getPreprocessorOpts().FailedModules->addFailed(ModuleName);
     return nullptr;
   }
 
   // Okay, we've rebuilt and now loaded the module.

clang/test/Modules/implicit-modules-use-lock.m

+// Confirm -fimplicit-modules-use-lock and -fno-implicit-modules-use-lock control
+// whether building a module triggers -Rmodule-lock, indirectly checking whether
+// a lock manager is being used.
+//
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fimplicit-modules-use-lock -Rmodule-lock \
+// RUN:   -fmodules-cache-path=%t.cache -I%S/Inputs/system-out-of-date \
+// RUN:   -fsyntax-only %s -Wnon-modular-include-in-framework-module \
+// RUN:   -Werror=non-modular-include-in-framework-module 2>&1 \
+// RUN: | FileCheck %s -check-prefix=CHECK-LOCKS
+//
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fno-implicit-modules-use-lock -Rmodule-lock \
+// RUN:   -fmodules-cache-path=%t.cache -I%S/Inputs/system-out-of-date \
+// RUN:   -fsyntax-only %s -Wnon-modular-include-in-framework-module \
+// RUN:   -Werror=non-modular-include-in-framework-module 2>&1 \
+// RUN: | FileCheck %s -check-prefix=CHECK-NO-LOCKS -allow-empty
+
+// CHECK-NO-LOCKS-NOT: remark:
+// CHECK-LOCKS: remark: locking '{{.*}}.pcm' to build module 'X' [-Rmodule-lock]
+@import X;