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; +}