diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index fd71b39a604b4..7efcbc2dad2cc 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -3489,6 +3489,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<"">, + HelpText<"Time to wait on an implicit module lock before timing out">, + MarshallingInfoInt, "90">; + defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf", LangOpts<"SkipODRCheckInGMF">, DefaultFalse, PosFlag ModulesEmbedFiles; + /// The time in seconds to wait on an implicit module lock before timing out. + unsigned ImplicitModulesLockTimeoutSeconds = 90; + /// The list of AST files to merge. std::vector ASTMergeFiles; diff --git a/clang/include/clang/Tooling/DependencyScanning/InProcessModuleCache.h b/clang/include/clang/Tooling/DependencyScanning/InProcessModuleCache.h index 213e60b39c199..84f74b621f3ab 100644 --- a/clang/include/clang/Tooling/DependencyScanning/InProcessModuleCache.h +++ b/clang/include/clang/Tooling/DependencyScanning/InProcessModuleCache.h @@ -12,14 +12,19 @@ #include "clang/Serialization/ModuleCache.h" #include "llvm/ADT/StringMap.h" +#include +#include #include -#include namespace clang { namespace tooling { namespace dependencies { struct ModuleCacheEntry { - std::shared_mutex CompilationMutex; + std::mutex Mutex; + std::condition_variable CondVar; + bool Locked = false; + unsigned Generation = 0; + std::atomic Timestamp = 0; }; diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 72363476534d8..060440ef43f36 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -4041,6 +4041,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 69d30af6eb6d1..d95be42b9bdd9 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -1763,7 +1763,9 @@ static bool compileModuleAndReadASTBehindLock( // Someone else is responsible for building the module. Wait for them to // finish. - switch (Lock->waitForUnlockFor(std::chrono::seconds(90))) { + unsigned Timeout = + ImportingInstance.getFrontendOpts().ImplicitModulesLockTimeoutSeconds; + switch (Lock->waitForUnlockFor(std::chrono::seconds(Timeout))) { case llvm::WaitForUnlockResult::Success: break; // The interesting case. case llvm::WaitForUnlockResult::OwnerDied: @@ -1771,11 +1773,11 @@ static bool compileModuleAndReadASTBehindLock( 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. + // twice. In case of timeout, try to build it ourselves again. Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout) << Module->Name; // Clear the lock file so that future invocations can make progress. - Lock->unsafeMaybeUnlock(); + Lock->unsafeUnlock(); continue; } diff --git a/clang/lib/Lex/Pragma.cpp b/clang/lib/Lex/Pragma.cpp index 87001cf17689b..3427548b3d811 100644 --- a/clang/lib/Lex/Pragma.cpp +++ b/clang/lib/Lex/Pragma.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include @@ -1069,6 +1070,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/lib/Tooling/DependencyScanning/InProcessModuleCache.cpp b/clang/lib/Tooling/DependencyScanning/InProcessModuleCache.cpp index d1e543b438225..34729bc3ce048 100644 --- a/clang/lib/Tooling/DependencyScanning/InProcessModuleCache.cpp +++ b/clang/lib/Tooling/DependencyScanning/InProcessModuleCache.cpp @@ -12,42 +12,63 @@ #include "llvm/Support/AdvisoryLock.h" #include "llvm/Support/Chrono.h" -#include - using namespace clang; using namespace tooling; 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 OwningLock; + ModuleCacheEntry &Entry; + std::optional OwnedGeneration; public: - ReaderWriterLock(std::shared_mutex &Mutex) - : OwningLock(Mutex, std::defer_lock) {} - - Expected tryLock() override { return OwningLock.try_lock(); } + ReaderWriterLock(ModuleCacheEntry &Entry) : Entry(Entry) {} + + Expected tryLock() override { + std::lock_guard Lock(Entry.Mutex); + if (Entry.Locked) + return false; + Entry.Locked = true; + OwnedGeneration = Entry.Generation; + return true; + } 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 Lock(*OwningLock.mutex()); - return llvm::WaitForUnlockResult::Success; + assert(!OwnedGeneration); + std::unique_lock Lock(Entry.Mutex); + unsigned CurrentGeneration = Entry.Generation; + bool Success = Entry.CondVar.wait_for(Lock, MaxSeconds, [&] { + // We check not only Locked, but also Generation to break the wait in case + // of unsafeUnlock() and successful tryLock(). + return !Entry.Locked || Entry.Generation != CurrentGeneration; + }); + return Success ? 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. + std::error_code unsafeUnlock() override { + { + std::lock_guard Lock(Entry.Mutex); + Entry.Generation += 1; + Entry.Locked = false; + } + Entry.CondVar.notify_all(); return {}; } - ~ReaderWriterLock() override = default; + ~ReaderWriterLock() override { + if (OwnedGeneration) { + { + std::lock_guard Lock(Entry.Mutex); + // Avoid stomping over the state managed by someone else after + // unsafeUnlock() and successful tryLock(). + if (*OwnedGeneration == Entry.Generation) + Entry.Locked = false; + } + Entry.CondVar.notify_all(); + } + } }; class InProcessModuleCache : public ModuleCache { @@ -65,14 +86,14 @@ class InProcessModuleCache : public ModuleCache { void prepareForGetLock(StringRef Filename) override {} std::unique_ptr getLock(StringRef Filename) override { - auto &CompilationMutex = [&]() -> std::shared_mutex & { + auto &Entry = [&]() -> ModuleCacheEntry & { std::lock_guard Lock(Entries.Mutex); auto &Entry = Entries.Map[Filename]; if (!Entry) Entry = std::make_unique(); - return Entry->CompilationMutex; + return *Entry; }(); - return std::make_unique(CompilationMutex); + return std::make_unique(Entry); } std::time_t getModuleTimestamp(StringRef Filename) override { 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..226c80c9b9fa2 --- /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[1:]] +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" 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 cdded51108b50..3ff00f6f6e0e1 100644 --- a/llvm/lib/Support/LockFileManager.cpp +++ b/llvm/lib/Support/LockFileManager.cpp @@ -287,6 +287,6 @@ LockFileManager::waitForUnlockFor(std::chrono::seconds MaxSeconds) { return WaitForUnlockResult::Timeout; } -std::error_code LockFileManager::unsafeMaybeUnlock() { +std::error_code LockFileManager::unsafeUnlock() { return sys::fs::remove(LockFileName); } diff --git a/llvm/lib/Support/VirtualOutputBackends.cpp b/llvm/lib/Support/VirtualOutputBackends.cpp index cff1de2072543..b2c4a1a058940 100644 --- a/llvm/lib/Support/VirtualOutputBackends.cpp +++ b/llvm/lib/Support/VirtualOutputBackends.cpp @@ -465,7 +465,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)); } @@ -477,7 +477,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. @@ -497,7 +497,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 9af812960542c..884a691076c7f 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp @@ -1577,7 +1577,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 } }