Merge branch 'master' of https://forgejo.catcrafts.net/Catcrafts/Crafter.Build

implementations/Crafter.Build-Platform.cpp

@@ -27,6 +27,9 @@ module;
 #include <unistd.h>
 #include <dlfcn.h>
 #include <sys/wait.h>
+#include <sys/file.h>
+#include <fcntl.h>
+#include <errno.h>
 #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 (`<cache>/<target>-<march>/`),
+    // 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<unsigned long>(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<unsigned long>(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<unsigned> 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<std::string> 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));
-            }
         }
     }
 }

tests/ConcurrentCacheRace/main.cpp

@@ -9,9 +9,10 @@ using namespace Crafter;
 // clobber each other's writes to `<cache>/<target>-<march>/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-*