diff --git a/implementations/Crafter.Build-Platform.cpp b/implementations/Crafter.Build-Platform.cpp index 687b1bb..3e9bf96 100644 --- a/implementations/Crafter.Build-Platform.cpp +++ b/implementations/Crafter.Build-Platform.cpp @@ -36,42 +36,6 @@ import :Progress; 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() { -#ifdef CRAFTER_BUILD_CONFIGURATION_TARGET_x86_64_pc_linux_gnu - return static_cast(getpid()); -#else - return static_cast(GetCurrentProcessId()); -#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()); - } -} - fs::path Crafter::GetCrafterBuildHome() { if (const char* envHome = std::getenv("CRAFTER_BUILD_HOME")) { return fs::path(envHome); @@ -286,14 +250,7 @@ std::string Crafter::BuildStdPcm(const Configuration& config, fs::path stdPcm) { std::string stdcppm = std::format("{}\\modules\\c++\\v1\\std.cppm", libcxx); 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 ""; } @@ -313,7 +270,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 +277,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)); - } } } } @@ -461,18 +412,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 +430,14 @@ 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. + 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 +445,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) { @@ -534,7 +469,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 +476,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)); - } } } } @@ -752,24 +680,7 @@ 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()); - 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("cp {} {}/std.cppm\nclang++ --target={} -march={} -mtune={} -femulated-tls -O3 -std=c++26 -Wno-reserved-identifier -Wno-reserved-module-identifier --precompile {}/std.cppm -o {}", stdCc.string(), stdPcm.parent_path().string(), config.target, config.march, config.mtune, stdPcm.parent_path().string(), stdPcm.string())); } else { return ""; } @@ -797,14 +708,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 ""; } @@ -857,23 +761,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/project.cpp b/project.cpp index 749f775..89ab636 100644 --- a/project.cpp +++ b/project.cpp @@ -107,12 +107,6 @@ extern "C" Configuration CrafterBuildProject(std::span a cfg.AddTest("VariantId").Dependencies({ crafterBuildLib.get() }); cfg.AddTest("WasiBrowserRuntime").Dependencies({ crafterBuildLib.get() }); cfg.AddTest("RunSingleTestExit").Dependencies({ crafterBuildLib.get() }); - // LoadProject dlopens the synthesized project.so, which references - // Crafter:: symbols (HostTarget, Configuration ctors) that have to be - // visible from the test exe — same wiring crafter-build itself uses - // for project.so. - cfg.AddTest("ConcurrentCacheRace").Dependencies({ crafterBuildLib.get() }) - .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); } return cfg; diff --git a/tests/ConcurrentCacheRace/main.cpp b/tests/ConcurrentCacheRace/main.cpp deleted file mode 100644 index 3f2f7c2..0000000 --- a/tests/ConcurrentCacheRace/main.cpp +++ /dev/null @@ -1,117 +0,0 @@ -#include -import std; -import Crafter.Build; -namespace fs = std::filesystem; -using namespace Crafter; - -// Regression for the host-cache write race: when several crafter-build -// invocations shared the same XDG_CACHE_HOME, two LoadProject() calls would -// 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. -// -// LoadProject() is the right surface to test because it runs the full host -// bootstrap (BuildStdPcm + EnsureCrafterBuildPcms × the Crafter.Build-* -// modules), giving every concurrent thread enough writing-to-the-same-paths -// time for the race to surface. A pure BuildStdPcm test wouldn't — the std -// PCM write is too short to overlap reliably between threads. -namespace { - fs::path FindCrafterBuildHome() { - if (const char* env = std::getenv("CRAFTER_BUILD_HOME"); env && *env) { - return env; - } - for (const char* candidate : { - "/usr/local/share/crafter-build", - "/usr/share/crafter-build", - }) { - if (fs::exists(fs::path(candidate) / "Crafter.Build.cppm")) { - return candidate; - } - } - return {}; - } - - void WriteFile(const fs::path& p, std::string_view contents) { - std::ofstream out(p); - out << contents; - } -} - -int main() { - fs::path home = FindCrafterBuildHome(); - if (home.empty()) { - std::println(std::cerr, - "SKIP: no installed share/crafter-build found and CRAFTER_BUILD_HOME unset"); - return 77; - } - setenv("CRAFTER_BUILD_HOME", home.string().c_str(), 1); - - fs::path scratch = fs::temp_directory_path() / "crafter-build-concurrent-cache-race"; - std::error_code ec; - fs::remove_all(scratch, ec); - fs::create_directories(scratch); - - // Cold scratch cache. setenv before any thread starts so every thread's - // GetCacheDir() call sees the same override. - fs::path cacheDir = scratch / "cache"; - fs::create_directories(cacheDir); - setenv("XDG_CACHE_HOME", cacheDir.string().c_str(), 1); - - constexpr int N = 4; - std::vector projects; - for (int i = 0; i < N; ++i) { - fs::path dir = scratch / std::format("p{}", i); - fs::create_directories(dir); - WriteFile(dir / "project.cpp", std::format( - "import std;\n" - "import Crafter.Build;\n" - "namespace fs = std::filesystem;\n" - "using namespace Crafter;\n" - "extern \"C\" Configuration CrafterBuildProject(std::span) {{\n" - " Configuration cfg;\n" - " cfg.path = \"./\";\n" - " cfg.name = \"p{}\";\n" - " cfg.outputName = \"p{}\";\n" - " cfg.target = HostTarget();\n" - " cfg.type = ConfigurationType::Executable;\n" - " return cfg;\n" - "}}\n", i, i)); - projects.push_back(dir / "project.cpp"); - } - - std::array errors; - std::vector threads; - threads.reserve(N); - for (int i = 0; i < N; ++i) { - threads.emplace_back([&, i]() { - try { - std::array args = {}; - (void)LoadProject(projects[i], args); - } catch (const std::exception& e) { - errors[i] = e.what(); - } - }); - } - for (std::thread& t : threads) t.join(); - - int failures = 0; - for (int i = 0; i < N; ++i) { - if (!errors[i].empty()) { - std::println(std::cerr, "FAIL p{}: {}", i, errors[i]); - ++failures; - } - } - - fs::remove_all(scratch, ec); - - if (failures > 0) { - std::println(std::cerr, - "{}/{} concurrent LoadProject() invocations failed (host-cache race)", - failures, N); - return 1; - } - return 0; -}