fix dag reverse-edge race causing stranded targets under -j (14b027eb) · Commits · tuxist / makeplus

executor.cpp

+49 −17

Changes for executor.cpp: 49 added lines, 17 removed lines.
Original line number	Diff line number	Diff line
		@@ -752,23 +752,23 @@ void Executor::add_to_dag(const std::string& target, std::set<std::string>& visi

		dag_[target] = std::move(node);

		// Add reverse edges
		for (const auto& p : dag_[target].prereqs) {
		if (dag_.count(p)) {
		dag_[p].dependents.push_back(target);
		} else {
		fprintf(stderr, "DEBUGDAG dropped-edge target=%s prereq=%s (not in dag_, unresolved_deps was still incremented)\n", target.c_str(), p.c_str());
		fflush(stderr);
		}
		}
		for (const auto& p : dag_[target].order_only) {
		if (dag_.count(p)) {
		dag_[p].dependents.push_back(target);
		} else {
		fprintf(stderr, "DEBUGDAG dropped-edge(order-only) target=%s prereq=%s (not in dag_, unresolved_deps was still incremented)\n", target.c_str(), p.c_str());
		fflush(stderr);
		}
		}
		// Reverse edges (node.dependents, used by worker_thread() to decrement a
		// dependent's unresolved_deps once this prereq finishes) are deliberately
		// NOT built here. This function recurses depth-first and marks `target`
		// in `visited` (line 719) BEFORE `target` itself lands in dag_ (line
		// 753) -- so a prereq reachable via two different paths through the
		// graph (an extremely common "diamond": many objects all depending on
		// the same shared archive/header) can have its second `add_to_dag` call
		// return early at line 718 while the FIRST call for that same target is
		// still mid-recursion, i.e. genuinely not in dag_ yet. Building reverse
		// edges right here, per-node, mid-DFS, would silently drop that edge
		// (dag_.count(p) false) while node.unresolved_deps was already
		// incremented for it unconditionally just above -- a counter that can
		// then never reach zero, permanently stranding the dependent in
		// State::Pending with no error and no process ever spawned for it. See
		// build_parallel()'s own "Phase 1.5" comment for where this is done
		// instead, once every reachable node is guaranteed to already be in
		// dag_.
		}

		void Executor::worker_thread() {
		@@ -862,6 +862,38 @@ int Executor::build_parallel(const std::vector<std::string>& targets) {
		add_to_dag(t, visited, {});
		}

		// Phase 1.5: Build reverse edges (node.dependents), now that every
		// reachable target is guaranteed to already be a key in dag_ -- see
		// add_to_dag()'s own comment for why this can't safely be done
		// per-node, mid-DFS, there. A prereq that's STILL missing from dag_ at
		// this point (all recursion long finished) is a genuine bug elsewhere
		// (e.g. the same file referenced under two spellings normalize_target()
		// doesn't unify -- an absolute path vs. a relative one, or an
		// unresolved "../" segment) rather than a DFS-ordering artifact, so it's
		// reported instead of silently stranding the dependent forever.
		for (auto& [name, node] : dag_) {
		for (const auto& p : node.prereqs) {
		auto it = dag_.find(p);
		if (it != dag_.end()) {
		it->second.dependents.push_back(name);
		} else {
		std::cerr << "makeplus: warning: '" << name
		<< "' prerequisite '" << p
		<< "' never resolved to a known target (dependency graph bug or path-spelling mismatch) -- this target may hang forever waiting for it" << std::endl;
		}
		}
		for (const auto& p : node.order_only) {
		auto it = dag_.find(p);
		if (it != dag_.end()) {
		it->second.dependents.push_back(name);
		} else {
		std::cerr << "makeplus: warning: '" << name
		<< "' order-only prerequisite '" << p
		<< "' never resolved to a known target (dependency graph bug or path-spelling mismatch) -- this target may hang forever waiting for it" << std::endl;
		}
		}
		}

		// Phase 2: Find initially ready nodes
		remaining_ = static_cast<int>(dag_.size());
		for (auto& [name, node] : dag_) {