diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index fe2b121..687b1bb 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -27,9 +27,6 @@ module; #include #include #include -#include -#include -#include #endif module Crafter.Build:Platform_impl; import std; @@ -40,57 +37,39 @@ namespace fs = std::filesystem; using namespace Crafter; namespace { - // Exclusive advisory lock over one PCM cache dir (`/-/`), - // held for the duration of a (re)build. Concurrent crafter-build invocations — - // and the worker threads inside a single invocation — that share a cache - // serialize their builds through it: the first builder to acquire the lock - // precompiles std.pcm / the Crafter.Build-*.pcm modules while everyone else - // blocks, then re-checks freshness and reuses the finished file instead of - // re-running clang and racing to overwrite the same path. Each builder must - // pass through the lock before it reaches the compile steps that *read* those - // PCMs, so a reader never observes a half-written file. - // - // The lock lives in the OS, keyed on a `.lock` file in the cache dir, and is - // released automatically when the descriptor/handle closes — including on - // process death — so an interrupted build can never leave a stale lock. The - // descriptor is close-on-exec / non-inheritable so spawned clang processes - // don't keep the lock alive past our own release. - class CacheLock { + // Used to make per-process temp filenames so concurrent crafter-build + // invocations sharing the host cache dir don't collide on the same + // partially-written .pcm. Cross-process uniqueness comes from the PID; + // intra-process uniqueness from the counter (multiple host PCMs precompile + // back-to-back, and tests can call BuildStdPcm from several threads). + unsigned long CurrentProcessId() { #ifdef CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_linux_gnu - int fd_ = -1; - public: - explicit CacheLock(const fs::path& cacheDir) { - fd_ = ::open((cacheDir / ".lock").c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644); - if (fd_ >= 0) { - while (::flock(fd_, LOCK_EX) != 0 && errno == EINTR) {} - } - } - ~CacheLock() { - if (fd_ >= 0) { - ::close(fd_); // closing the descriptor drops the flock - } - } + return static_cast(getpid()); #else - HANDLE handle_ = INVALID_HANDLE_VALUE; - public: - explicit CacheLock(const fs::path& cacheDir) { - handle_ = CreateFileW((cacheDir / ".lock").wstring().c_str(), - GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); - if (handle_ != INVALID_HANDLE_VALUE) { - OVERLAPPED ov{}; - LockFileEx(handle_, LOCKFILE_EXCLUSIVE_LOCK, 0, MAXDWORD, MAXDWORD, &ov); - } - } - ~CacheLock() { - if (handle_ != INVALID_HANDLE_VALUE) { - CloseHandle(handle_); // closing the handle drops the lock - } - } + return static_cast(GetCurrentProcessId()); #endif - CacheLock(const CacheLock&) = delete; - CacheLock& operator=(const CacheLock&) = delete; - }; + } + + fs::path MakeTempPcmPath(const fs::path& finalPath) { + static std::atomic seq{0}; + return fs::path(finalPath.string() + std::format(".tmp.{}.{}", + CurrentProcessId(), seq.fetch_add(1, std::memory_order_relaxed))); + } + + // Atomically swap `tmpPath` into `finalPath`. If a parallel crafter-build + // committed its own equivalent build first (identical .cppm + flags → + // identical .pcm), `fs::rename` may fail on some filesystems; accept the + // outcome as long as the destination now exists. + std::string CommitPcm(const fs::path& tmpPath, const fs::path& finalPath) { + std::error_code ec; + fs::rename(tmpPath, finalPath, ec); + if (!ec) return ""; + std::error_code rmEc; + fs::remove(tmpPath, rmEc); + if (fs::exists(finalPath)) return ""; + return std::format("rename {} -> {}: {}", + tmpPath.string(), finalPath.string(), ec.message()); + } } fs::path Crafter::GetCrafterBuildHome() { @@ -306,9 +285,15 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { std::string libcxx = std::getenv("LIBCXX_DIR"); std::string stdcppm = std::format("{}\\modules\\c++\\v1\\std.cppm", libcxx); - CacheLock lock(stdPcm.parent_path()); if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdcppm)) { - return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, stdPcm.string())); + fs::path tmpPcm = MakeTempPcmPath(stdPcm); + std::string out = RunCommand(std::format("clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 -nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", config.target, config.march, config.mtune, tmpPcm.string())); + if (!out.empty()) { + std::error_code ec; + fs::remove(tmpPcm, ec); + return out; + } + return CommitPcm(tmpPcm, stdPcm); } return ""; } @@ -319,7 +304,6 @@ std::string Crafter::GetBaseCommand(const Configuration& config) { namespace { void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { - CacheLock lock(cacheDir); for (std::string_view name : kCrafterBuildModules) { fs::path cppmPath = sourceDir / (std::string(name) + ".cppm"); fs::path pcmPath = cacheDir / (std::string(name) + ".pcm"); @@ -329,6 +313,7 @@ namespace { if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { continue; } + fs::path tmpPcm = MakeTempPcmPath(pcmPath); std::string cmd = std::format( "clang++ --target=x86_64-pc-windows-msvc -march=native -mtune=native " "-std=c++26 -O3 -D CRAFTER_BUILD_DLL_IMPORT " @@ -336,11 +321,16 @@ namespace { "-Wno-reserved-identifier -Wno-reserved-module-identifier " "-fprebuilt-module-path={} " "--precompile {} -o {}", - cacheDir.string(), cppmPath.string(), pcmPath.string()); + cacheDir.string(), cppmPath.string(), tmpPcm.string()); CommandResult r = Crafter::RunCommandChecked(cmd); if (r.exitCode != 0) { + std::error_code ec; + fs::remove(tmpPcm, ec); throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } + if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) { + throw std::runtime_error(std::format("Failed to commit {} pcm: {}", name, err)); + } } } } @@ -459,7 +449,6 @@ namespace { } std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { - CacheLock lock(stdPcm.parent_path()); if (config.target == "x86_64-pc-windows-msvc") { // MSVC target on mingw host: same MSVC libc++ logic as the // native-MSVC LoadProject path. User must have LIBCXX_DIR pointing @@ -472,11 +461,18 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { if (fs::exists(stdPcm) && fs::last_write_time(stdPcm) >= fs::last_write_time(stdcppm)) { return ""; } - return RunCommand(std::format( + fs::path tmpPcm = MakeTempPcmPath(stdPcm); + std::string out = RunCommand(std::format( "clang++ --target={} -march={} -mtune={} -isystem %LIBCXX_DIR%\\include\\c++\\v1 " "-nostdinc++ -nostdlib++ -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier " "--precompile %LIBCXX_DIR%\\modules\\c++\\v1\\std.cppm -o {}", - config.target, config.march, config.mtune, stdPcm.string())); + config.target, config.march, config.mtune, tmpPcm.string())); + if (!out.empty()) { + std::error_code ec; + fs::remove(tmpPcm, ec); + return out; + } + return CommitPcm(tmpPcm, stdPcm); } // Default: mingw target. Look up mingw-w64 libstdc++ via the msys2 prefix. @@ -490,15 +486,16 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { } // Copy std.cc → std.cppm in C++ rather than via cmd's `copy /Y` because // `copy` always prints "1 file(s) copied." to stdout and RunCommand - // treats any output as an error. Held under the cache lock, so a plain - // filename is safe — no other builder writes this path concurrently. - fs::path stdCppm = stdPcm.parent_path() / "std.cppm"; + // treats any output as an error. Per-PID filename so concurrent + // crafter-build invocations don't see each other's half-written copies. + fs::path stdCppm = stdPcm.parent_path() / std::format("std.cppm.{}", CurrentProcessId()); std::error_code ec; fs::copy_file(stdCc, stdCppm, fs::copy_options::overwrite_existing, ec); if (ec) { return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), ec.message()); } - return RunCommand(std::format( + fs::path tmpPcm = MakeTempPcmPath(stdPcm); + std::string out = RunCommand(std::format( "clang++ --target={} -march={} -mtune={} " "--sysroot=\"{}\" -femulated-tls " "-O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier " @@ -506,7 +503,14 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { config.target, config.march, config.mtune, prefix.string(), stdCppm.string(), - stdPcm.string())); + tmpPcm.string())); + std::error_code rmEc; + fs::remove(stdCppm, rmEc); + if (!out.empty()) { + fs::remove(tmpPcm, rmEc); + return out; + } + return CommitPcm(tmpPcm, stdPcm); } std::string Crafter::GetBaseCommand(const Configuration& config) { @@ -518,7 +522,6 @@ std::string Crafter::GetBaseCommand(const Configuration& config) { namespace { void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { - CacheLock lock(cacheDir); fs::path prefix = MingwPrefix(); for (std::string_view name : kCrafterBuildModules) { fs::path cppmPath = sourceDir / (std::string(name) + ".cppm"); @@ -531,6 +534,7 @@ namespace { if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { continue; } + fs::path tmpPcm = MakeTempPcmPath(pcmPath); std::string cmd = std::format( "clang++ --target=x86_64-w64-mingw32 -march=native -mtune=native " "--sysroot=\"{}\" -femulated-tls " @@ -538,12 +542,18 @@ namespace { "-Wno-reserved-identifier -Wno-reserved-module-identifier " "-fprebuilt-module-path={} " "--precompile {} -o {}", - prefix.string(), cacheDir.string(), cppmPath.string(), pcmPath.string()); + prefix.string(), cacheDir.string(), cppmPath.string(), tmpPcm.string()); CommandResult r = Crafter::RunCommandChecked(cmd); if (r.exitCode != 0) { + std::error_code ec; + fs::remove(tmpPcm, ec); throw std::runtime_error(std::format( "Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } + if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) { + throw std::runtime_error(std::format( + "Failed to commit {} pcm: {}", name, err)); + } } } } @@ -720,7 +730,6 @@ CommandResult Crafter::RunCommandWithTimeout(std::string_view cmd, std::chrono:: } std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { - CacheLock lock(stdPcm.parent_path()); if(config.target == "x86_64-w64-mingw32") { std::vector folders; @@ -743,15 +752,24 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdCc)) { // -femulated-tls keeps PCM TLS access matching libstdc++'s emutls // definitions; mismatch surfaces as undefined std::__once_callable. - // The std.cc → std.cppm copy uses a plain filename: held under the - // cache lock, no other builder writes this path concurrently. - fs::path stdCppm = stdPcm.parent_path() / "std.cppm"; + // Per-PID std.cppm and a tmp .pcm + atomic rename so concurrent + // crafter-build invocations sharing the cache dir don't read each + // other's half-written files. + fs::path stdCppm = stdPcm.parent_path() / std::format("std.cppm.{}", CurrentProcessId()); std::error_code copyEc; fs::copy_file(stdCc, stdCppm, fs::copy_options::overwrite_existing, copyEc); if (copyEc) { return std::format("copy {} -> {}: {}", stdCc.string(), stdCppm.string(), copyEc.message()); } - return RunCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), stdPcm.string())); + fs::path tmpPcm = MakeTempPcmPath(stdPcm); + std::string out = RunCommand(std::format("clang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, config.march, config.mtune, stdCppm.string(), tmpPcm.string())); + std::error_code rmEc; + fs::remove(stdCppm, rmEc); + if (!out.empty()) { + fs::remove(tmpPcm, rmEc); + return out; + } + return CommitPcm(tmpPcm, stdPcm); } else { return ""; } @@ -779,7 +797,14 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { ? std::string(" -fno-exceptions -msimd128 -fno-c++-static-destructors -mllvm -wasm-enable-sjlj -D_WASI_EMULATED_SIGNAL") : std::format(" -march={} -mtune={}", config.march, config.mtune); if(!fs::exists(stdPcm) || fs::last_write_time(stdPcm) < fs::last_write_time(stdCppm)) { - return RunCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, stdPcm.string())); + fs::path tmpPcm = MakeTempPcmPath(stdPcm); + std::string out = RunCommand(std::format("clang++ --target={} -std=c++26 -stdlib=libc++{}{} -O3 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {} -o {}", config.target, sysrootFlag, archFlags, stdCppm, tmpPcm.string())); + if (!out.empty()) { + std::error_code ec; + fs::remove(tmpPcm, ec); + return out; + } + return CommitPcm(tmpPcm, stdPcm); } else { return ""; } @@ -823,7 +848,6 @@ namespace { }; void EnsureCrafterBuildPcms(const fs::path& sourceDir, const fs::path& cacheDir) { - CacheLock lock(cacheDir); for (std::string_view name : kCrafterBuildModules) { fs::path cppmPath = sourceDir / (std::string(name) + ".cppm"); fs::path pcmPath = cacheDir / (std::string(name) + ".pcm"); @@ -833,17 +857,23 @@ namespace { if (fs::exists(pcmPath) && fs::last_write_time(cppmPath) < fs::last_write_time(pcmPath)) { continue; } + fs::path tmpPcm = MakeTempPcmPath(pcmPath); std::string cmd = std::format( "clang++ --target=x86_64-pc-linux-gnu -march=native -mtune=native " "-std=c++26 -stdlib=libc++ -O3 " "-Wno-reserved-identifier -Wno-reserved-module-identifier " "-fprebuilt-module-path={} " "--precompile {} -o {}", - cacheDir.string(), cppmPath.string(), pcmPath.string()); + cacheDir.string(), cppmPath.string(), tmpPcm.string()); CommandResult r = Crafter::RunCommandChecked(cmd); if (r.exitCode != 0) { + std::error_code ec; + fs::remove(tmpPcm, ec); throw std::runtime_error(std::format("Failed to precompile {} (exit {}): {}", name, r.exitCode, r.output)); } + if (std::string err = CommitPcm(tmpPcm, pcmPath); !err.empty()) { + throw std::runtime_error(std::format("Failed to commit {} pcm: {}", name, err)); + } } } } diff --git a/tests/ConcurrentCacheRace/main.cpp b/tests/ConcurrentCacheRace/main.cpp index 7caba6c..3f2f7c2 100644 --- a/tests/ConcurrentCacheRace/main.cpp +++ b/tests/ConcurrentCacheRace/main.cpp @@ -9,10 +9,9 @@ using namespace Crafter; // clobber each other's writes to `/-/std.pcm` and the // `Crafter.Build-*.pcm` host modules. The loser's clang would then read a // torn file via -fprebuilt-module-path and die with "malformed or corrupted -// precompiled file: 'can't skip to bit X from Y'". The fix serializes PCM -// (re)builds through an exclusive advisory lock on the cache dir: only one -// builder precompiles while the rest wait and then reuse the finished file, so -// no two writers ever touch the same path and a reader never sees a torn one. +// precompiled file: 'can't skip to bit X from Y'". The fix routes every PCM +// write through a per-process temp + atomic rename so a reader always sees +// either the old or the new file, never a half-written one. // // LoadProject() is the right surface to test because it runs the full host // bootstrap (BuildStdPcm + EnsureCrafterBuildPcms × the Crafter.Build-*