https://github.com/jansvoboda11 created https://github.com/llvm/llvm-project/pull/182722
When there's a dependency cycle between modules, the dependency scanner may encounter a deadlock. This was caused by not respecting the lock timeout. Even with the timeout implemented, the in-process module cache lock cannot possibly implement `unsafeMaybeUnlock()` (to avoid UB), which means the thread still won't make any progress. This was fixed in by just compiling the module after timing out on a lock instead of trying (and failing) to acquire it again. This PR also implements `-fimplicit-modules-lock-timeout=<seconds>` that allows tweaking the default 90-second lock timeout, and adds `#pragma clang __debug sleep` that makes it easier to achieve desired execution ordering. >From be001ad7223c795514f2cd138649714467c6d767 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <[email protected]> Date: Sat, 21 Feb 2026 20:09:57 -0800 Subject: [PATCH] [clang][modules] Prevent deadlock in module cache --- .../DependencyScanning/InProcessModuleCache.h | 2 +- .../include/clang/Frontend/FrontendOptions.h | 7 +++- clang/include/clang/Options/Options.td | 6 +++ .../InProcessModuleCache.cpp | 23 ++++++------ clang/lib/Driver/ToolChains/Clang.cpp | 2 + clang/lib/Frontend/CompilerInstance.cpp | 26 +++++++------ clang/lib/Lex/Pragma.cpp | 3 ++ .../ClangScanDeps/modules-cycle-deadlock.c | 37 +++++++++++++++++++ clang/test/Modules/implicit-cycle-deadlock.c | 36 ++++++++++++++++++ 9 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 clang/test/ClangScanDeps/modules-cycle-deadlock.c create mode 100644 clang/test/Modules/implicit-cycle-deadlock.c diff --git a/clang/include/clang/DependencyScanning/InProcessModuleCache.h b/clang/include/clang/DependencyScanning/InProcessModuleCache.h index 4c95171a2e21e..96f74725b1e06 100644 --- a/clang/include/clang/DependencyScanning/InProcessModuleCache.h +++ b/clang/include/clang/DependencyScanning/InProcessModuleCache.h @@ -20,7 +20,7 @@ namespace clang { namespace dependencies { struct ModuleCacheEntry { - std::shared_mutex CompilationMutex; + std::shared_timed_mutex CompilationMutex; std::atomic<std::time_t> Timestamp = 0; }; diff --git a/clang/include/clang/Frontend/FrontendOptions.h b/clang/include/clang/Frontend/FrontendOptions.h index ba7da56cb9fce..48e1db4e812be 100644 --- a/clang/include/clang/Frontend/FrontendOptions.h +++ b/clang/include/clang/Frontend/FrontendOptions.h @@ -491,6 +491,9 @@ class FrontendOptions { /// The list of files to embed into the compiled module file. std::vector<std::string> ModulesEmbedFiles; + /// The time in seconds to wait on an implicit module lock before timing out. + unsigned ImplicitModulesLockTimeoutSeconds; + /// The list of AST files to merge. std::vector<std::string> ASTMergeFiles; @@ -552,8 +555,8 @@ class FrontendOptions { EmitSymbolGraphSymbolLabelsForTesting(false), EmitPrettySymbolGraphs(false), GenReducedBMI(false), UseClangIRPipeline(false), ClangIRDisablePasses(false), - ClangIRDisableCIRVerifier(false), TimeTraceGranularity(500), - TimeTraceVerbose(false) {} + ClangIRDisableCIRVerifier(false), ImplicitModulesLockTimeoutSeconds(90), + TimeTraceGranularity(500), TimeTraceVerbose(false) {} /// getInputKindForExtension - Return the appropriate input kind for a file /// extension. For example, "c" would return Language::C. diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td index 4ac812e92e2cb..3d8f70f6b5ccb 100644 --- a/clang/include/clang/Options/Options.td +++ b/clang/include/clang/Options/Options.td @@ -3488,6 +3488,12 @@ def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption]>, Visibility<[ClangOption, CLOption, CC1Option]>, HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">; +def fimplicit_modules_lock_timeout_EQ : Joined<["-"], "fimplicit-modules-lock-timeout=">, + Flags<[NoXarchOption]>, Visibility<[ClangOption, CLOption, CC1Option]>, + MetaVarName<"<seconds>">, + HelpText<"Time to wait on an implicit module lock before timing out">, + MarshallingInfoInt<FrontendOpts<"ImplicitModulesLockTimeoutSeconds">>; + defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf", LangOpts<"SkipODRCheckInGMF">, DefaultFalse, PosFlag<SetTrue, [], [CC1Option], diff --git a/clang/lib/DependencyScanning/InProcessModuleCache.cpp b/clang/lib/DependencyScanning/InProcessModuleCache.cpp index 4d5dd0c43112c..add43813917fd 100644 --- a/clang/lib/DependencyScanning/InProcessModuleCache.cpp +++ b/clang/lib/DependencyScanning/InProcessModuleCache.cpp @@ -20,10 +20,10 @@ using namespace dependencies; namespace { class ReaderWriterLock : public llvm::AdvisoryLock { // TODO: Consider using std::atomic::{wait,notify_all} when we move to C++20. - std::unique_lock<std::shared_mutex> OwningLock; + std::unique_lock<std::shared_timed_mutex> OwningLock; public: - ReaderWriterLock(std::shared_mutex &Mutex) + ReaderWriterLock(std::shared_timed_mutex &Mutex) : OwningLock(Mutex, std::defer_lock) {} Expected<bool> tryLock() override { return OwningLock.try_lock(); } @@ -31,18 +31,17 @@ class ReaderWriterLock : public llvm::AdvisoryLock { llvm::WaitForUnlockResult waitForUnlockFor(std::chrono::seconds MaxSeconds) override { assert(!OwningLock); - // We do not respect the timeout here. It's very generous for implicit - // modules, so we'd typically only reach it if the owner crashed (but so did - // we, since we run in the same process), or encountered deadlock. - (void)MaxSeconds; - std::shared_lock<std::shared_mutex> Lock(*OwningLock.mutex()); - return llvm::WaitForUnlockResult::Success; + std::shared_lock<std::shared_timed_mutex> Lock(*OwningLock.mutex(), + MaxSeconds); + return Lock ? llvm::WaitForUnlockResult::Success + : llvm::WaitForUnlockResult::Timeout; } std::error_code unsafeMaybeUnlock() override { - // Unlocking the mutex here would trigger UB and we don't expect this to be - // actually called when compiling scanning modules due to the no-timeout - // guarantee above. + // Only the thread that locked a mutex can unlock it without triggering UB. + // We're forced to ignore the request with the understanding that we will + // not unblock other threads that are currently waiting, and they will have + // to time out themselves. return {}; } @@ -64,7 +63,7 @@ class InProcessModuleCache : public ModuleCache { void prepareForGetLock(StringRef Filename) override {} std::unique_ptr<llvm::AdvisoryLock> getLock(StringRef Filename) override { - auto &CompilationMutex = [&]() -> std::shared_mutex & { + auto &CompilationMutex = [&]() -> std::shared_timed_mutex & { std::lock_guard<std::mutex> Lock(Entries.Mutex); auto &Entry = Entries.Map[Filename]; if (!Entry) diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 195f05d61464f..3fcc7a83b0070 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -3911,6 +3911,8 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D, Path.insert(Path.begin(), Arg, Arg + strlen(Arg)); CmdArgs.push_back(Args.MakeArgString(Path)); } + + Args.AddLastArg(CmdArgs, options::OPT_fimplicit_modules_lock_timeout_EQ); } if (HaveModules) { diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp index 9f1a3c56feec1..8668d10132c06 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1498,9 +1498,9 @@ static bool compileModuleAndReadASTBehindLock( auto Lock = ModuleCache.getLock(ModuleFileName); bool Owned; if (llvm::Error Err = Lock->tryLock().moveInto(Owned)) { - // ModuleCache takes care of correctness and locks are only necessary for - // performance. Fallback to building the module in case of any lock - // related errors. + // InMemoryModuleCache takes care of correctness and locks are only + // necessary for performance. Fallback to building the module in case of + // any lock related errors. Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure) << Module->Name << toString(std::move(Err)); return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc, @@ -1514,20 +1514,24 @@ static bool compileModuleAndReadASTBehindLock( // Someone else is responsible for building the module. Wait for them to // finish. - switch (Lock->waitForUnlockFor(std::chrono::seconds(90))) { + std::chrono::seconds Timeout( + ImportingInstance.getFrontendOpts().ImplicitModulesLockTimeoutSeconds); + switch (Lock->waitForUnlockFor(Timeout)) { case llvm::WaitForUnlockResult::Success: break; // The interesting case. case llvm::WaitForUnlockResult::OwnerDied: - continue; // try again to get the lock. + continue; // Try again to get the lock. case llvm::WaitForUnlockResult::Timeout: - // Since the InMemoryModuleCache takes care of correctness, we try waiting - // for someone else to complete the build so that it does not happen - // twice. In case of timeout, build it ourselves. + // Attempt to clear the lock file so that future invocations can make + // progress without waiting for the timeout. + Lock->unsafeMaybeUnlock(); + // InMemoryModuleCache takes care of correctness and locks are only + // necessary for performance. Fallback to building the module in case of + // lock timeout. Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout) << Module->Name; - // Clear the lock file so that future invocations can make progress. - Lock->unsafeMaybeUnlock(); - continue; + return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc, + ModuleNameLoc, Module, ModuleFileName); } // Read the module that was just written by someone else. diff --git a/clang/lib/Lex/Pragma.cpp b/clang/lib/Lex/Pragma.cpp index bba3c89bed38f..1a5a6ce61ecf1 100644 --- a/clang/lib/Lex/Pragma.cpp +++ b/clang/lib/Lex/Pragma.cpp @@ -46,6 +46,7 @@ #include <cstdint> #include <optional> #include <string> +#include <thread> #include <utility> #include <vector> @@ -1079,6 +1080,8 @@ struct PragmaDebugHandler : public PragmaHandler { Crasher.setAnnotationRange(SourceRange(Tok.getLocation())); PP.EnterToken(Crasher, /*IsReinject*/ false); } + } else if (II->isStr("sleep")) { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); } else if (II->isStr("dump")) { Token DumpAnnot; DumpAnnot.startToken(); diff --git a/clang/test/ClangScanDeps/modules-cycle-deadlock.c b/clang/test/ClangScanDeps/modules-cycle-deadlock.c new file mode 100644 index 0000000000000..2a800ff7f2f20 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-cycle-deadlock.c @@ -0,0 +1,37 @@ +// This test checks that implicit modules do not encounter a deadlock on a dependency cycle. + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: sed "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json +// RUN: not clang-scan-deps -mode preprocess -format experimental-full \ +// RUN: -compilation-database %t/cdb.json -j 2 -o %t/out 2> %t/err +// RUN: FileCheck %s --input-file %t/err +// CHECK-DAG: fatal error: cyclic dependency in module 'M': M -> N -> M +// CHECK-DAG: fatal error: cyclic dependency in module 'N': N -> M -> N + +//--- cdb.json.in +[ + { + "file": "DIR/tu1.c", + "directory": "DIR", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fimplicit-modules-lock-timeout=3 DIR/tu1.c -o DIR/tu1.o" + }, + { + "file": "DIR/tu2.c", + "directory": "DIR", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fimplicit-modules-lock-timeout=3 DIR/tu2.c -o DIR/tu2.o" + } +] +//--- tu1.c +#include "m.h" +//--- tu2.c +#include "n.h" +//--- module.modulemap +module M { header "m.h" } +module N { header "n.h" } +//--- m.h +#pragma clang __debug sleep // Give enough time for tu2.c to start building N. +#include "n.h" +//--- n.h +#pragma clang __debug sleep // Give enough time for tu1.c to start building M. +#include "m.h" diff --git a/clang/test/Modules/implicit-cycle-deadlock.c b/clang/test/Modules/implicit-cycle-deadlock.c new file mode 100644 index 0000000000000..dff1af9be11ae --- /dev/null +++ b/clang/test/Modules/implicit-cycle-deadlock.c @@ -0,0 +1,36 @@ +// This test checks that implicit modules do not encounter a deadlock on a dependency cycle. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: %python %t/run_concurrently.py \ +// RUN: "not %clang -fsyntax-only -fmodules -fmodules-cache-path=%t/cache -fimplicit-modules-lock-timeout=3 %t/tu1.c 2> %t/err1" \ +// RUN: "not %clang -fsyntax-only -fmodules -fmodules-cache-path=%t/cache -fimplicit-modules-lock-timeout=3 %t/tu2.c 2> %t/err2" + +// RUN: FileCheck %s --input-file %t/err1 --check-prefix=CHECK1 +// RUN: FileCheck %s --input-file %t/err2 --check-prefix=CHECK2 +// CHECK1: fatal error: cyclic dependency in module 'M': M -> N -> M +// CHECK2: fatal error: cyclic dependency in module 'N': N -> M -> N + +//--- run_concurrently.py +import subprocess, sys, threading + +def run(cmd): + subprocess.run(cmd, shell=True) + +threads = [threading.Thread(target=run, args=(cmd,)) for cmd in sys.argv] +for t in threads: t.start() +for t in threads: t.join() +//--- tu1.c +#include "m.h" +//--- tu2.c +#include "n.h" +//--- module.modulemap +module M { header "m.h" } +module N { header "n.h" } +//--- m.h +#pragma clang __debug sleep // Give enough time for tu2.c to start building N. +#include "n.h" +//--- n.h +#pragma clang __debug sleep // Give enough time for tu1.c to start building M. +#include "m.h" _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
