From 47cd50a7d26e013e0bfeb61c8a0e847b8375204d Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Sat, 30 May 2026 19:28:06 +0200 Subject: [PATCH] fixed build error and file stdpcm lock --- implementations/Crafter.Build-Platform.cpp | 176 +++++++++------------ tests/ConcurrentCacheRace/main.cpp | 7 +- 2 files changed, 77 insertions(+), 106 deletions(-) diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index 687b1bb..fe2b121 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -27,6 +27,9 @@ module; #include #include #include +#include +#include +#include #endif module Crafter.Build:Platform_impl; import std; @@ -37,39 +40,57 @@ namespace fs = std::filesystem; using namespace Crafter; namespace { - // 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() { + // 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 { #ifdef CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_linux_gnu - return static_cast(getpid()); + 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 + } + } #else - return static_cast(GetCurrentProcessId()); + 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 + } + } #endif - } - - 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()); - } + CacheLock(const CacheLock&) = delete; + CacheLock& operator=(const CacheLock&) = delete; + }; } fs::path Crafter::GetCrafterBuildHome() { @@ -285,15 +306,9 @@ 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)) { - 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 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())); } return ""; } @@ -304,6 +319,7 @@ 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"); @@ -313,7 +329,6 @@ 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 " @@ -321,16 +336,11 @@ namespace { "-Wno-reserved-identifier -Wno-reserved-module-identifier " "-fprebuilt-module-path={} " "--precompile {} -o {}", - cacheDir.string(), cppmPath.string(), tmpPcm.string()); + cacheDir.string(), cppmPath.string(), pcmPath.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)); - } } } } @@ -449,6 +459,7 @@ 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 @@ -461,18 +472,11 @@ 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 ""; } - fs::path tmpPcm = MakeTempPcmPath(stdPcm); - std::string out = RunCommand(std::format( + 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, tmpPcm.string())); - if (!out.empty()) { - std::error_code ec; - fs::remove(tmpPcm, ec); - return out; - } - return CommitPcm(tmpPcm, stdPcm); + config.target, config.march, config.mtune, stdPcm.string())); } // Default: mingw target. Look up mingw-w64 libstdc++ via the msys2 prefix. @@ -486,16 +490,15 @@ 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. 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()); + // 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"; 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()); } - fs::path tmpPcm = MakeTempPcmPath(stdPcm); - std::string out = RunCommand(std::format( + return RunCommand(std::format( "clang++ --target={} -march={} -mtune={} " "--sysroot=\"{}\" -femulated-tls " "-O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier " @@ -503,14 +506,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { config.target, config.march, config.mtune, prefix.string(), 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); + stdPcm.string())); } std::string Crafter::GetBaseCommand(const Configuration& config) { @@ -522,6 +518,7 @@ 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"); @@ -534,7 +531,6 @@ 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 " @@ -542,18 +538,12 @@ namespace { "-Wno-reserved-identifier -Wno-reserved-module-identifier " "-fprebuilt-module-path={} " "--precompile {} -o {}", - prefix.string(), cacheDir.string(), cppmPath.string(), tmpPcm.string()); + prefix.string(), cacheDir.string(), cppmPath.string(), pcmPath.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)); - } } } } @@ -730,6 +720,7 @@ 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; @@ -752,24 +743,15 @@ 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. - // 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()); + // 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"; 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()); } - 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); + 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())); } else { return ""; } @@ -797,14 +779,7 @@ 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)) { - 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); + 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())); } else { return ""; } @@ -848,6 +823,7 @@ 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"); @@ -857,23 +833,17 @@ 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(), tmpPcm.string()); + cacheDir.string(), cppmPath.string(), pcmPath.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 3f2f7c2..7caba6c 100644 --- a/tests/ConcurrentCacheRace/main.cpp +++ b/tests/ConcurrentCacheRace/main.cpp @@ -9,9 +9,10 @@ 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 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. +// 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. // // LoadProject() is the right surface to test because it runs the full host // bootstrap (BuildStdPcm + EnsureCrafterBuildPcms × the Crafter.Build-*