https://github.com/jansvoboda11 updated https://github.com/llvm/llvm-project/pull/182722
>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 1/2] [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" >From 2d9729d87d726de5a2179fd6df4654fd469baca4 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <[email protected]> Date: Sun, 22 Feb 2026 14:56:00 -0800 Subject: [PATCH 2/2] Implement `unsafeUnlock()` for in-process module cache --- .../DependencyScanning/InProcessModuleCache.h | 3 ++- .../InProcessModuleCache.cpp | 24 ++++++++++++------- clang/lib/Frontend/CompilerInstance.cpp | 11 +++------ llvm/include/llvm/Support/AdvisoryLock.h | 2 +- llvm/include/llvm/Support/LockFileManager.h | 2 +- llvm/lib/Support/LockFileManager.cpp | 2 +- llvm/lib/Support/VirtualOutputBackends.cpp | 6 ++--- llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp | 2 +- 8 files changed, 28 insertions(+), 24 deletions(-) diff --git a/clang/include/clang/DependencyScanning/InProcessModuleCache.h b/clang/include/clang/DependencyScanning/InProcessModuleCache.h index 96f74725b1e06..e2417761b2c1f 100644 --- a/clang/include/clang/DependencyScanning/InProcessModuleCache.h +++ b/clang/include/clang/DependencyScanning/InProcessModuleCache.h @@ -20,7 +20,8 @@ namespace clang { namespace dependencies { struct ModuleCacheEntry { - std::shared_timed_mutex CompilationMutex; + std::shared_ptr<std::shared_timed_mutex> CompilationMutex = + std::make_shared<std::shared_timed_mutex>(); std::atomic<std::time_t> Timestamp = 0; }; diff --git a/clang/lib/DependencyScanning/InProcessModuleCache.cpp b/clang/lib/DependencyScanning/InProcessModuleCache.cpp index add43813917fd..184cef7979e64 100644 --- a/clang/lib/DependencyScanning/InProcessModuleCache.cpp +++ b/clang/lib/DependencyScanning/InProcessModuleCache.cpp @@ -19,12 +19,21 @@ using namespace dependencies; namespace { class ReaderWriterLock : public llvm::AdvisoryLock { + /// Reference to the mutex shared by module cache clients. When unsafe unlock + /// is requested, we override it using std::atomic_store(). To obtain it, we + /// use std::atomic_load(). + std::shared_ptr<std::shared_timed_mutex> &SharedMutex; + /// The mutex we co-own with other threads. This is kept alive for the entire + /// lifetime of this class. Unsafe unlock does not affect it. + std::shared_ptr<std::shared_timed_mutex> CoOwnedMutex; + // TODO: Consider using std::atomic::{wait,notify_all} when we move to C++20. std::unique_lock<std::shared_timed_mutex> OwningLock; public: - ReaderWriterLock(std::shared_timed_mutex &Mutex) - : OwningLock(Mutex, std::defer_lock) {} + explicit ReaderWriterLock(std::shared_ptr<std::shared_timed_mutex> &M) + : SharedMutex(M), CoOwnedMutex(std::atomic_load(&SharedMutex)), + OwningLock(*CoOwnedMutex, std::defer_lock) {} Expected<bool> tryLock() override { return OwningLock.try_lock(); } @@ -37,11 +46,9 @@ class ReaderWriterLock : public llvm::AdvisoryLock { : llvm::WaitForUnlockResult::Timeout; } - std::error_code unsafeMaybeUnlock() override { - // 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. + std::error_code unsafeUnlock() override { + std::atomic_store(&SharedMutex, + std::make_shared<std::shared_timed_mutex>()); return {}; } @@ -63,7 +70,8 @@ class InProcessModuleCache : public ModuleCache { void prepareForGetLock(StringRef Filename) override {} std::unique_ptr<llvm::AdvisoryLock> getLock(StringRef Filename) override { - auto &CompilationMutex = [&]() -> std::shared_timed_mutex & { + auto &CompilationMutex = + [&]() -> std::shared_ptr<std::shared_timed_mutex> & { std::lock_guard<std::mutex> Lock(Entries.Mutex); auto &Entry = Entries.Map[Filename]; if (!Entry) diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp index 8668d10132c06..8a3315ed0ad16 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1522,16 +1522,11 @@ static bool compileModuleAndReadASTBehindLock( case llvm::WaitForUnlockResult::OwnerDied: continue; // Try again to get the lock. case llvm::WaitForUnlockResult::Timeout: - // 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; - return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc, - ModuleNameLoc, Module, ModuleFileName); + // Clear the lock file so that future invocations can make progress. + Lock->unsafeUnlock(); + continue; } // Read the module that was just written by someone else. diff --git a/llvm/include/llvm/Support/AdvisoryLock.h b/llvm/include/llvm/Support/AdvisoryLock.h index d1c3ccc187e64..03be1fcbe7243 100644 --- a/llvm/include/llvm/Support/AdvisoryLock.h +++ b/llvm/include/llvm/Support/AdvisoryLock.h @@ -48,7 +48,7 @@ class AdvisoryLock { /// For a lock owned by someone else, unlock it. A permitted side-effect is /// that another thread/process may acquire ownership of the lock before the /// existing owner unlocks it. This is an unsafe operation. - virtual std::error_code unsafeMaybeUnlock() = 0; + virtual std::error_code unsafeUnlock() = 0; /// Unlocks the lock if its ownership was previously acquired by \c tryLock(). virtual ~AdvisoryLock() = default; diff --git a/llvm/include/llvm/Support/LockFileManager.h b/llvm/include/llvm/Support/LockFileManager.h index 1c579ea866f65..69f175bd79f7c 100644 --- a/llvm/include/llvm/Support/LockFileManager.h +++ b/llvm/include/llvm/Support/LockFileManager.h @@ -61,7 +61,7 @@ class LLVM_ABI LockFileManager : public AdvisoryLock { /// Remove the lock file. This may delete a different lock file than /// the one previously read if there is a race. - std::error_code unsafeMaybeUnlock() override; + std::error_code unsafeUnlock() override; /// Unlocks the lock if previously acquired by \c tryLock(). ~LockFileManager() override; diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp index 0891ef980668b..1a98ec03486db 100644 --- a/llvm/lib/Support/LockFileManager.cpp +++ b/llvm/lib/Support/LockFileManager.cpp @@ -294,7 +294,7 @@ LockFileManager::waitForUnlockFor(std::chrono::seconds MaxSeconds) { return WaitForUnlockResult::Timeout; } -std::error_code LockFileManager::unsafeMaybeUnlock() { +std::error_code LockFileManager::unsafeUnlock() { auto BypassSandbox = sys::sandbox::scopedDisable(); return sys::fs::remove(LockFileName); diff --git a/llvm/lib/Support/VirtualOutputBackends.cpp b/llvm/lib/Support/VirtualOutputBackends.cpp index d7a91dfa1471f..0c6ce825d02d0 100644 --- a/llvm/lib/Support/VirtualOutputBackends.cpp +++ b/llvm/lib/Support/VirtualOutputBackends.cpp @@ -496,7 +496,7 @@ Error OnDiskOutputFile::keep() { if (Error Err = Lock.tryLock().moveInto(Owned)) { // If we error acquiring a lock, we cannot ensure appends // to the trace file are atomic - cannot ensure output correctness. - Lock.unsafeMaybeUnlock(); + Lock.unsafeUnlock(); return convertToOutputError( OutputPath, std::make_error_code(std::errc::no_lock_available)); } @@ -508,7 +508,7 @@ Error OnDiskOutputFile::keep() { return convertToOutputError(OutputPath, EC); Out << (*Content)->getBuffer(); Out.close(); - Lock.unsafeMaybeUnlock(); + Lock.unsafeUnlock(); if (Out.has_error()) return convertToOutputError(OutputPath, Out.error()); // Remove temp file and done. @@ -528,7 +528,7 @@ Error OnDiskOutputFile::keep() { // the lock, causing other waiting processes to time-out. Let's clear // the lock and try again right away. If we do start seeing compiler // hangs in this location, we will need to re-consider. - Lock.unsafeMaybeUnlock(); + Lock.unsafeUnlock(); continue; } } diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp index d04dc3e25f2dc..c155d85df1769 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp @@ -1575,7 +1575,7 @@ PreservedAnalyses AMDGPUSplitModulePass::run(Module &M, dbgs() << "[amdgpu-split-module] unable to acquire lockfile, debug " "output may be mangled by other processes\n"); - Lock.unsafeMaybeUnlock(); + Lock.unsafeUnlock(); break; // give up } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
