Account for match options re-locking when checking if target is matched

6 files changed, 49 insertions, 15 deletions

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