From e76f92ae0afcfbb3507b1adc73574872f3e7f826 Mon Sep 17 00:00:00 2001 From: catbot Date: Mon, 1 Jun 2026 11:32:46 +0000 Subject: [PATCH] fix: scope per-build module-state reset to the config being built MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build() resets each Module/ModulePartition's per-build `compiled`/`checked` flags so a reused Configuration re-evaluates mtimes. That reset recursed into cfg.dependencies — but dependency Configurations are shared across the build DAG and each is compiled concurrently by its own Build() call. A parent/sibling's recursive reset could therefore clear a shared dependency's module `compiled` atomic *after* that dependency's module-compile thread had set it true and exited, but before an intra-config waiter (its impl, or a dependent partition) ran compiled.wait(false). The waiter then blocked forever on a flag nothing would re-signal: the build froze mid-compile, idle, with no compiler process alive — exactly the hang in issue #16. Reset only the current configuration's own modules. Every config in the tree already gets its own Build() call (the per-PcmDir builder registered in depResults), which resets its own state at the top of that call, sequenced before its compile threads spawn. Cross-config module state is consulted only via PCM file mtimes and the depResults futures, never via these flags, so the narrower reset is correct and removes the data race entirely. Adds ConcurrentDependencyReset: builds a static-lib dependency fully, then builds a consumer that depends on it while the dependency is already cached in depResults (so it is never rebuilt), and asserts the consumer build leaves the dependency's module `compiled` flag intact. Fails deterministically on the old recursive reset; passes with the fix. Resolves #16 Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Build-Clang.cpp | 43 +++--- project.cpp | 1 + .../fixture/app/main.cpp | 7 + .../fixture/deplib/DepMod-impl.cpp | 7 + .../fixture/deplib/DepMod.cppm | 2 + tests/ConcurrentDependencyReset/main.cpp | 126 ++++++++++++++++++ 6 files changed, 167 insertions(+), 19 deletions(-) create mode 100644 tests/ConcurrentDependencyReset/fixture/app/main.cpp create mode 100644 tests/ConcurrentDependencyReset/fixture/deplib/DepMod-impl.cpp create mode 100644 tests/ConcurrentDependencyReset/fixture/deplib/DepMod.cppm create mode 100644 tests/ConcurrentDependencyReset/main.cpp diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index 25f89fb..ce3ea4e 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -215,25 +215,30 @@ void Configuration::GetInterfacesAndImplementations(std::span interfac BuildResult Crafter::Build(Configuration& config, std::unordered_map>& depResults, std::mutex& depMutex) { // Reset per-build cached state on every Module/ModulePartition so that // successive Build() calls on the same Configuration re-evaluate mtimes - // (incremental-rebuild test scenarios). Walks cfg.dependencies recursively - // with a seen-set so diamond deps don't loop. - { - std::unordered_set resetSeen; - std::function reset = [&](Configuration* c) { - if (!resetSeen.insert(c).second) return; - for (auto& iface : c->interfaces) { - iface->checked = false; - iface->compiled.store(false); - for (auto& part : iface->partitions) { - part->checked = false; - part->compiled.store(false); - } - } - for (Configuration* dep : c->dependencies) { - reset(dep); - } - }; - reset(&config); + // (incremental-rebuild test scenarios). + // + // Reset ONLY this configuration's own modules — never recurse into + // dependencies. Configuration objects are shared across the dependency DAG + // (diamond deps point at the same Configuration*), and every config in the + // tree gets its own Build() call: the per-PcmDir builder registered in + // depResults resets its own state here, at the top of its own Build(), + // sequenced before that config's compile threads spawn. Recursing into + // dependencies from here would re-clear a shared dependency's `compiled` + // atomic from a parent/sibling thread *while that dependency's own Build() + // is concurrently compiling it* — after its module-compile thread set the + // flag true and exited but before its impl/partition waiter ran + // compiled.wait(false). The waiter would then block forever on a flag + // nothing re-signals: the build freezes mid-compile, idle, with no compiler + // process alive (issue #16). Cross-config module state is consulted only via + // PCM file mtimes and the depResults futures, never via these flags, so the + // narrower reset is correct. + for (auto& iface : config.interfaces) { + iface->checked = false; + iface->compiled.store(false); + for (auto& part : iface->partitions) { + part->checked = false; + part->compiled.store(false); + } } // Auto-detect the WASI sysroot before any compile step runs so BuildStdPcm diff --git a/project.cpp b/project.cpp index 749f775..c9adbac 100644 --- a/project.cpp +++ b/project.cpp @@ -113,6 +113,7 @@ extern "C" Configuration CrafterBuildProject(std::span a // for project.so. cfg.AddTest("ConcurrentCacheRace").Dependencies({ crafterBuildLib.get() }) .LinkFlag("-Wl,--export-dynamic").LinkFlag("-ldl"); + cfg.AddTest("ConcurrentDependencyReset").Dependencies({ crafterBuildLib.get() }); } return cfg; diff --git a/tests/ConcurrentDependencyReset/fixture/app/main.cpp b/tests/ConcurrentDependencyReset/fixture/app/main.cpp new file mode 100644 index 0000000..fe7b228 --- /dev/null +++ b/tests/ConcurrentDependencyReset/fixture/app/main.cpp @@ -0,0 +1,7 @@ +import std; +import DepMod; + +int main() { + std::print("{}", dep_value() * 2); + return 0; +} diff --git a/tests/ConcurrentDependencyReset/fixture/deplib/DepMod-impl.cpp b/tests/ConcurrentDependencyReset/fixture/deplib/DepMod-impl.cpp new file mode 100644 index 0000000..69e9b3b --- /dev/null +++ b/tests/ConcurrentDependencyReset/fixture/deplib/DepMod-impl.cpp @@ -0,0 +1,7 @@ +module DepMod; + +// Implementation unit of the DepMod interface. Because it declares +// `module DepMod;` it gains an intra-config module dependency on the DepMod +// interface, so its compile waits on DepMod's `compiled` flag — the exact +// flag a stale recursive reset used to clobber (issue #16). +int dep_value() { return 21; } diff --git a/tests/ConcurrentDependencyReset/fixture/deplib/DepMod.cppm b/tests/ConcurrentDependencyReset/fixture/deplib/DepMod.cppm new file mode 100644 index 0000000..17df21c --- /dev/null +++ b/tests/ConcurrentDependencyReset/fixture/deplib/DepMod.cppm @@ -0,0 +1,2 @@ +export module DepMod; +export int dep_value(); diff --git a/tests/ConcurrentDependencyReset/main.cpp b/tests/ConcurrentDependencyReset/main.cpp new file mode 100644 index 0000000..f42a4f1 --- /dev/null +++ b/tests/ConcurrentDependencyReset/main.cpp @@ -0,0 +1,126 @@ +import std; +import Crafter.Build; +namespace fs = std::filesystem; +using namespace Crafter; + +// Regression for the concurrent dependency-reset deadlock (issue #16). +// +// Build() resets each Module/ModulePartition's per-build `compiled` /`checked` +// flags so a reused Configuration re-evaluates mtimes. That reset MUST stay +// scoped to the configuration being built. It used to recurse into +// cfg.dependencies — but dependency Configurations are shared across the build +// DAG and each is compiled concurrently by its own Build() call. A parent's +// recursive reset could therefore clear a dependency's module `compiled` flag +// *after* that dependency's module-compile thread had set it true and exited, +// but before an intra-config waiter (its impl / a dependent partition) ran +// `compiled.wait(false)`. The waiter then blocked forever on a flag nothing +// re-signalled: the build froze mid-compile, idle, with no compiler process +// alive. +// +// This test pins the invariant deterministically, without depending on thread +// timing: build a static-lib dependency fully (its module ends up compiled), +// then build a consumer that depends on it while the dependency is already +// cached in depResults (so it is NOT rebuilt). Before the fix, the consumer's +// recursive reset flipped the dependency's module `compiled` flag back to false +// and, because the dependency never rebuilds, it stayed false. After the fix +// the consumer resets only its own modules, so the dependency's flag is left +// intact. We assert it is still set once the consumer build completes. +int main() { + fs::path fixtureRoot = fs::current_path() / "tests" / "ConcurrentDependencyReset" / "fixture"; + + // Cold output: a stale archive/PCM from a previous run would let the + // dependency build short-circuit and skip the module compile we rely on. + std::error_code ec; + fs::remove_all(fixtureRoot / "deplib" / "build", ec); + fs::remove_all(fixtureRoot / "deplib" / "bin", ec); + fs::remove_all(fixtureRoot / "app" / "build", ec); + fs::remove_all(fixtureRoot / "app" / "bin", ec); + + Configuration dep; + dep.path = fixtureRoot / "deplib"; + dep.name = "depmod"; + dep.outputName = "depmod"; + dep.target = HostTarget(); + dep.type = ConfigurationType::LibraryStatic; + { + std::array ifaces = { "DepMod" }; + std::array impls = { "DepMod-impl" }; + dep.GetInterfacesAndImplementations(ifaces, impls); + } + + if (dep.interfaces.size() != 1) { + std::println(std::cerr, "expected 1 dependency interface, got {}", dep.interfaces.size()); + return 1; + } + + { + std::unordered_map> depResults; + std::mutex depMutex; + BuildResult r = Build(dep, depResults, depMutex); + if (!r.result.empty()) { + std::println(std::cerr, "dependency build failed: {}", r.result); + return 1; + } + if (!dep.interfaces[0]->compiled.load()) { + std::println(std::cerr, "dependency module not marked compiled after its own build"); + return 1; + } + // Seed a consumer-facing cache so the consumer reuses this build instead + // of rebuilding the dependency — mirrors how a shared dep is built once + // and consumed by multiple parents through depResults. + std::promise promise; + promise.set_value(std::move(r)); + std::shared_future ready = promise.get_future().share(); + + std::unordered_map> consumerDeps; + consumerDeps.emplace(dep.PcmDir(), ready); + + Configuration app; + app.path = fixtureRoot / "app"; + app.name = "resetapp"; + app.outputName = "resetapp"; + app.target = HostTarget(); + app.type = ConfigurationType::Executable; + app.dependencies = { &dep }; + { + std::array appIfaces = {}; + std::array appImpls = { "main" }; + app.GetInterfacesAndImplementations(appIfaces, appImpls); + } + + BuildResult ar = Build(app, consumerDeps, depMutex); + if (!ar.result.empty()) { + std::println(std::cerr, "consumer build failed: {}", ar.result); + return 1; + } + + // The core regression assertion: building the consumer must not have + // reset the (cached, never-rebuilt) dependency's module flag. Before the + // fix this was false and an intra-config waiter would have deadlocked. + if (!dep.interfaces[0]->compiled.load()) { + std::println(std::cerr, + "FAIL: consumer build reset a cached dependency's module 'compiled' " + "flag (issue #16 regression) — this is the state that deadlocks a " + "concurrent build"); + return 1; + } + + fs::path bin = app.BinDir() / "resetapp"; + if (!fs::exists(bin)) { + std::println(std::cerr, "consumer binary not produced at {}", bin.string()); + return 1; + } + auto run = RunCommandWithTimeout(bin.string(), std::chrono::seconds(10)); + if (run.exitCode != 0 || run.timedOut || run.crashed) { + std::println(std::cerr, "consumer exe did not exit cleanly: exit={} output={}", + run.exitCode, run.output); + return 1; + } + if (run.output != "42") { + std::println(std::cerr, "expected '42', got '{}'", run.output); + return 1; + } + } + + return 0; +}