From 98c7247ff06decba8130e8aea67ea839d084d2dd Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Tue, 7 Nov 2023 07:21:06 +0200 Subject: Account for match options re-locking when checking if target is matched --- libbuild2/algorithm.cxx | 15 +++++++++++++++ libbuild2/algorithm.hxx | 2 +- libbuild2/algorithm.ixx | 3 ++- libbuild2/target.cxx | 12 ++++++++---- libbuild2/target.hxx | 2 ++ libbuild2/target.ixx | 30 +++++++++++++++++++++--------- 6 files changed, 49 insertions(+), 15 deletions(-) (limited to 'libbuild2') diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx index 0e995d5..1bb46d8 100644 --- a/libbuild2/algorithm.cxx +++ b/libbuild2/algorithm.cxx @@ -311,8 +311,15 @@ namespace build2 { // First lock for this operation. // + // Note that we use 0 match_extra::cur_options_ as an indication of not + // being applied yet. In particular, in the match phase, this is used to + // distinguish between the "busy because not applied yet" and "busy + // because relocked to reapply match options" cases. See + // target::matched() for details. + // s.rule = nullptr; s.dependents.store (0, memory_order_release); + s.match_extra.cur_options_.store (0, memory_order_relaxed); offset = target::offset_touched; } @@ -981,6 +988,7 @@ namespace build2 recipe re (ar != nullptr ? f (*ar, a, t, me) : ru.apply (a, t, me)); me.free (); // Note: cur_options are still in use. + assert (me.cur_options != 0); // Match options cannot be 0 after apply(). me.cur_options_.store (me.cur_options, memory_order_relaxed); return re; } @@ -1044,6 +1052,7 @@ namespace build2 // Note: for now no adhoc_reapply(). // ru.reapply (a, t, me); + assert (me.cur_options != 0); // Match options cannot be 0 after reapply(). me.cur_options_.store (me.cur_options, memory_order_relaxed); } @@ -1352,6 +1361,12 @@ namespace build2 { s.state = target_state::failed; l.offset = target::offset_applied; + + // Make sure we don't relock a failed target. + // + match_extra& me (s.match_extra); + me.cur_options = match_extra::all_options; + me.cur_options_.store (me.cur_options, memory_order_relaxed); } if (s.state == target_state::failed) diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx index 11e655b..a0f8cd6 100644 --- a/libbuild2/algorithm.hxx +++ b/libbuild2/algorithm.hxx @@ -440,7 +440,7 @@ namespace build2 // // Note that there will be no way to rematch on options change (since there // is no rule), so passing anything other than all_options is most likely a - // bad idea. + // bad idea. Passing 0 for options is illegal. // void match_recipe (target_lock&, diff --git a/libbuild2/algorithm.ixx b/libbuild2/algorithm.ixx index e384433..c7b4b7e 100644 --- a/libbuild2/algorithm.ixx +++ b/libbuild2/algorithm.ixx @@ -614,7 +614,8 @@ namespace build2 inline void match_recipe (target_lock& l, recipe r, uint64_t options) { - assert (l.target != nullptr && + assert (options != 0 && + l.target != nullptr && l.offset < target::offset_matched && l.target->ctx.phase == run_phase::match); diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx index fb47b6d..b4c3968 100644 --- a/libbuild2/target.cxx +++ b/libbuild2/target.cxx @@ -1004,15 +1004,19 @@ namespace build2 case run_phase::load: break; case run_phase::match: { - // Similar logic to matched_state_impl(). + // Similar logic to target::matched(). // const opstate& s (state[action () /* inner */]); - // Note: already synchronized. - size_t c (s.task_count.load (memory_order_relaxed)); + // Note: use acquire for group_state(). + // + size_t c (s.task_count.load (memory_order_acquire)); size_t b (ctx.count_base ()); // Note: cannot do (c - b)! - if (c != (b + offset_applied) && c != (b + offset_executed)) + if (!(c == (b + offset_applied) || + c == (b + offset_executed) || + (c >= (b + offset_busy) && + s.match_extra.cur_options_.load (memory_order_relaxed) != 0))) break; } // Fall through. diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx index c61f241..76b7158 100644 --- a/libbuild2/target.hxx +++ b/libbuild2/target.hxx @@ -249,6 +249,8 @@ namespace build2 // - match to resolve members (resolve_members()) // - match of ad hoc group via one of its ad hoc members // + // Note that the 0 cur_options value is illegal. + // // When it comes to match options specified for group members, the // semantics differs between explicit and ad hoc groups. For explicit // groups, the standard semantics described above applies and the group's diff --git a/libbuild2/target.ixx b/libbuild2/target.ixx index ad09cd4..8136fe9 100644 --- a/libbuild2/target.ixx +++ b/libbuild2/target.ixx @@ -243,17 +243,25 @@ namespace build2 assert (ctx.phase == run_phase::match || ctx.phase == run_phase::execute); - size_t c (state[a].task_count.load (mo)); + const opstate& s (state[a]); + size_t c (s.task_count.load (mo)); size_t b (ctx.count_base ()); // Note: cannot do (c - b)! if (ctx.phase == run_phase::match) { - // While it will normally be applied, it could also be already executed. + // While it will normally be applied, it could also be already executed + // or being relocked to reapply match options (see lock_impl() for + // background). + // + // Note that we can't just do >= offset_applied since offset_busy can + // also mean it is being matched. // - // Note that we can't do >= offset_applied since offset_busy means it is - // being matched. + // See also matched_state_impl(), mtime() for similar logic. // - return c == (b + offset_applied) || c == (b + offset_executed); + return (c == (b + offset_applied) || + c == (b + offset_executed) || + (c >= (b + offset_busy) && + s.match_extra.cur_options_.load (memory_order_relaxed) != 0)); } else { @@ -308,11 +316,15 @@ namespace build2 return make_pair (false, target_state::unknown); else { - // Normally applied but can also be already executed. Note that in the - // latter case we are guaranteed to be synchronized since we are in the - // match phase. + // The same semantics as in target::matched(). Note that in the executed + // case we are guaranteed to be synchronized since we are in the match + // phase. // - assert (c == (b + offset_applied) || c == (b + offset_executed)); + assert (c == (b + offset_applied) || + c == (b + offset_executed) || + (c >= (b + offset_busy) && + s.match_extra.cur_options_.load (memory_order_relaxed) != 0)); + return make_pair (true, (group_state (a) ? group->state[a] : s).state); } } -- cgit v1.1