From 5e2c26176cf48b65103251186a2bf321eda069a9 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 25 May 2018 09:14:48 +0200 Subject: Fix postponed group/member state race --- build2/algorithm.cxx | 22 ++++++++++++++++++---- build2/target.ixx | 4 ++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index 8ee6d47..6743907 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -1790,15 +1790,29 @@ namespace build2 // const target& g (*t.group); - if (execute (a, g) == target_state::busy) + target_state gs (execute (a, g)); + + if (gs == target_state::busy) sched.wait (target::count_executed (), g[a].task_count, scheduler::work_none); - // Indicate to execute() that this target's state comes from the group - // (which, BTW, can be failed). + // Return target_state::group to signal to execute() that this target's + // state comes from the group (which, BTW, can be failed). + // + // There is just one small problem: if the returned group state is + // postponed, then this means the group hasn't been executed yet. And if + // we return target_state::group, then this means any state queries (see + // executed_state()) will be directed to the target which might still not + // be executed or, worse, is being executed as we query. + // + // So in this case we return target_state::postponed (which will result in + // the member being treated as unchanged). This is how it is done for + // prerequisites and seeing that we've been acting as if the group is our + // prerequisite, there is no reason to deviate (see the recipe return + // value documentation for details). // - return target_state::group; + return gs != target_state::postponed ? target_state::group : gs; } target_state diff --git a/build2/target.ixx b/build2/target.ixx index ea91b6e..88bc530 100644 --- a/build2/target.ixx +++ b/build2/target.ixx @@ -94,14 +94,14 @@ namespace build2 { // We go an extra step and short-circuit to the target state even if the // raw state is not group provided the recipe is group_recipe and the - // state is not failed. + // state is unknown (see mtime() for some ideas on why we do it). // const opstate& s (state[a]); if (s.state == target_state::group) return true; - if (s.state != target_state::failed && group != nullptr) + if (s.state == target_state::unknown && group != nullptr) { if (recipe_function* const* f = s.recipe.target ()) return *f == &group_action; -- cgit v1.1