WIP: fixes for match options

3 files changed, 19 insertions, 6 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 0c90642..bc4b835 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -1241,8 +1241,8 @@ namespace build2
           // Reapply if any new options.
           //
           match_extra& me (s.match_extra);
-          assert ((me.cur_options & options) != options); // Otherwise no lock.
-          me.new_options = options;
+          me.new_options = options & ~me.cur_options; // Clear existing.
+          assert (me.new_options != 0); // Otherwise should not have locked.
 
           // Feels like this can only be a logic bug since to end up with a
           // subset of options requires a rule (see match_extra for details).
diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx
index 5ebbae2..19f7db2 100644
--- a/libbuild2/algorithm.hxx
+++ b/libbuild2/algorithm.hxx
@@ -363,7 +363,7 @@ namespace build2
   // to be unchanged after match. If it is unmatch::safe, then unmatch the
   // target if it is safe (this includes unchanged or if we know that someone
   // else will execute this target). Return true in first half of the pair if
-  // unmatch succeeded. Always throw if failed. Note that unmatching doesn't
+  // unmatch succeeded. Always throw if failed. Note that unmatching may not
   // play well with options -- if unmatch succeeds, the options that have been
   // passed to match will not be cleared.
   //
@@ -489,6 +489,12 @@ namespace build2
   // of the match_*() functions. Note that natually you cannot rematch a
   // target that you have unmatched.
   //
+  // Note also that there is no way to check if the rematch is unnecessary
+  // (i.e., because the target is already matched with this option) because
+  // that would require MT-safety considerations (since there could be a
+  // concurrent rematch). Instead, you should rematch unconditionally and if
+  // the option is already present, it will be a cheap noop.
+  //
   target_state
   rematch_sync (action, const target&,
                 uint64_t options,
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 78f3e49..f537d59 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -189,7 +189,7 @@ namespace build2
     // options are defined for performing a specific operation on a specific
     // target type and would normally be part of the target type semantics.
     // To put it another way, when a rule matches a target of certain type for
-    // certain operation, there is an expectaion of certain semantics, some
+    // certain operation, there is an expectation of certain semantics, some
     // parts of which could be made optional.
     //
     // As a concrete example, consider installing libs{}, which traditionally
@@ -228,7 +228,14 @@ namespace build2
     // match options for update and clean operations is a bad idea (update of
     // pretty much any target can happen during match as a result of a tool
     // update while clean might have to be performed during match to provide
-    // the mirro semantics).
+    // the mirror semantics).
+    //
+    // Note also that with rematches the assumption that in the match phase
+    // after matching the target we can MT-safely examine its state (such as
+    // its prerequisite_targets) no longer holds since such state could be
+    // modified during a rematch. As a result, if the target type specifies
+    // options for a certain operation, then you should not rely on this
+    // assumption for targets of this type during this operation.
     //
     // A rule that supports match options must also be prepared to handle the
     // apply() call with new_options set to 0, for example, by using a
@@ -260,7 +267,7 @@ namespace build2
     uint64_t cur_options;
     uint64_t new_options;
 
-    static const uint64_t all_options = ~uint64_t (0);
+    static constexpr uint64_t all_options = ~uint64_t (0);
 
     // Auxiliary data storage.
     //