Extend special match_rule() logic to all groups with dynamic targets

9 files changed, 89 insertions, 92 deletions

diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index cd760b4..cf45699 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -295,8 +295,7 @@ namespace build2
   {
     tracer trace ("adhoc_buildscript_rule::apply");
 
-    // Handle matching explicit group members (see adhoc_rule::match() for
-    // background).
+    // Handle matching group members (see adhoc_rule::match() for background).
     //
     if (const group* g = t.group != nullptr ? t.group->is_a<group> () : nullptr)
     {
@@ -681,11 +680,10 @@ namespace build2
           if (g != nullptr)
           {
             pair<const build2::file&, bool> r (
-              dyndep::inject_group_member (
-                what,
-                a, bs, *g,
-                move (f),
-                map_ext, def_tt, filter, true /* skip_match */));
+              dyndep::inject_group_member (what,
+                                           a, bs, *g,
+                                           move (f),
+                                           map_ext, def_tt, filter));
 
             if (r.second)
               g->members.push_back (&r.first);
diff --git a/libbuild2/adhoc-rule-regex-pattern.cxx b/libbuild2/adhoc-rule-regex-pattern.cxx
index 3f207dc..257952f 100644
--- a/libbuild2/adhoc-rule-regex-pattern.cxx
+++ b/libbuild2/adhoc-rule-regex-pattern.cxx
@@ -370,7 +370,8 @@ namespace build2
         auto& ms (g->members);
 
         // These are conceptually static but they behave more like dynamic in
-        // that we likely need to insert the target, set its group, etc.
+        // that we likely need to insert the target, set its group, etc. In a
+        // sense, they are rule-static, but group-dynamic.
         //
         // Note: a custom version of the dyndep_rule::inject_group_member()
         // logic.
@@ -386,10 +387,6 @@ namespace build2
 
         const target& t (l.first); // Note: non-const only if have lock.
 
-        // Note: we don't need to match the group recipe directy due to the
-        // special ad hoc recipe/rule semantics for explicit group members
-        // in match_rule().
-        //
         if (l.second)
         {
           l.first.group = g;
@@ -400,13 +397,6 @@ namespace build2
           if (find (ms.begin (), ms.end (), &t) != ms.end ())
             continue;
 
-          // Check if we already belong to this group. Note that this not a
-          // mere optimization since we may be in the member->group->member
-          // chain and trying to lock the member the second time would
-          // deadlock (this can be triggered, for example, by dist, which sort
-          // of depends on such members directly @@ maybe this should be fixed
-          // there?).
-          //
           if (t.group != g) // Note: atomic.
           {
             // We can only update the group under lock.
diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index cf972e7..581da2e 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -521,42 +521,45 @@ namespace build2
       return dynamic_cast<const fallback_rule*> (&r.second.get ());
     };
 
-    // If this is a member of group-based target, then first try to find a
-    // matching ad hoc recipe/rule by matching (to an ad hoc recipe/rule) the
-    // group but applying to the member. See adhoc_rule::match() for
-    // background, including for why const_cast should be safe.
-    //
-    // To put it another way, if a group is matched by an ad hoc recipe/rule,
-    // then we want all the member to be matched to the same recipe/rule.
-    //
-    if (const group* g = t.group != nullptr ? t.group->is_a<group> () : nullptr)
+    if (const target* g = t.group)
     {
-      assert (pme == nullptr);
-
-      // As an optimization, check if the group is already matched for this
-      // action. Note that this can become important when matching adhoc regex
-      // rules since we can potentially call match() for many members. This is
-      // probably ok for static members (of which we don't expect more than a
-      // handful) but can become an issue for dynamic members.
+      // If this is a group with dynamic members, then match it with the
+      // group's rule automatically. See dyndep_rule::inject_group_member()
+      // for background.
       //
-      if (g->matched (a, memory_order_acquire))
+      if ((g->type ().flags & target_type::flag::dyn_members) ==
+          target_type::flag::dyn_members)
       {
-        const rule_match* r (g->state[a].rule);
-
-        if (r != nullptr && adhoc_rule_match (*r))
+        if (g->matched (a, memory_order_acquire))
+        {
+          const rule_match* r (g->state[a].rule);
+          assert (r != nullptr); // Shouldn't happen with dyn_members.
           return r;
+        }
 
-        // Fall through: if some rule matched the group then it must also deal
-        // with the members.
+        // Assume static member and fall through.
       }
-      else
+
+      // If this is a member of group-based target, then first try to find a
+      // matching ad hoc recipe/rule by matching (to an ad hoc recipe/rule) the
+      // group but applying to the member. See adhoc_rule::match() for
+      // background, including for why const_cast should be safe.
+      //
+      // To put it another way, if a group is matched by an ad hoc
+      // recipe/rule, then we want all the member to be matched to the same
+      // recipe/rule.
+      //
+      // Note that such a group is dyn_members so we would have tried the
+      // "already matched" case above.
+      //
+      if (g->is_a<group> ())
       {
         // We cannot init match_extra from the target if it's unlocked so use
         // a temporary (it shouldn't be modified if unlocked).
         //
         match_extra me (false /* locked */);
         if (const rule_match* r = match_rule (
-              a, const_cast<group&> (*g), skip, true /* try_match */, &me))
+              a, const_cast<target&> (*g), skip, true /* try_match */, &me))
           return r;
 
         // Fall through to normal match of the member.
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index 4914f6e..d37fef5 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -2967,7 +2967,7 @@ namespace build2
                   what_tgt,
                   a, bs, *g,
                   f, // Can't move since need to return dyn_targets.
-                  map_ext, *def_tt, filter, true /* skip_match */));
+                  map_ext, *def_tt, filter));
 
               // Note: no target_decl shenanigans since reset the members on
               // each update.
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index 5ac9648..9c0f8a8 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -831,12 +831,21 @@ namespace build2
   inject_group_member_impl (action a, const scope& bs, mtime_target& g,
                             path f, string n, string e,
                             const target_type& tt,
-                            const function<dyndep_rule::group_filter_func>& fl,
-                            bool skip_match)
+                            const function<dyndep_rule::group_filter_func>& fl)
   {
     // NOTE: see adhoc_rule_regex_pattern::apply_group_members() for a variant
     //       of the same code.
 
+    // Note that we used to directly match such a member with group_recipe.
+    // But that messes up our dependency counts since we don't really know
+    // whether someone will execute such a member.
+    //
+    // So instead we now just link the member up to the group and rely on the
+    // special semantics in match_rule() for groups with the dyn_members flag.
+    //
+    assert ((g.type ().flags & target_type::flag::dyn_members) ==
+            target_type::flag::dyn_members);
+
     // We expect that nobody else can insert these members (seems reasonable
     // seeing that their names are dynamically discovered).
     //
@@ -851,21 +860,15 @@ namespace build2
 
     const file& t (l.first.as<file> ()); // Note: non-const only if have lock.
 
-    bool locked (l.second);
-    if (locked)
+    // We don't need to match the group recipe directy from ad hoc
+    // recipes/rules due to the special semantics for explicit group members
+    // in match_rule(). This is what skip_match is for.
+    if (l.second)
     {
       l.first.group = &g;
       l.second.unlock ();
-
-      // We don't need to match the group recipe directy from ad hoc
-      // recipes/rules due to the special semantics for explicit group members
-      // in match_rule(). This is what skip_match is for.
-      //
-      if (skip_match)
-      {
-        t.path (move (f));
-        return pair<const file&, bool> (t, true);
-      }
+      t.path (move (f));
+      return pair<const file&, bool> (t, true);
     }
     else
     {
@@ -877,9 +880,9 @@ namespace build2
     // optimization since we may be in the member->group->member chain and
     // trying to lock the member the second time would deadlock (this can be
     // triggered, for example, by dist, which sort of depends on such members
-    // directly @@ maybe this should be fixed there?).
+    // directly... which was not quite correct and is now fixed).
     //
-    if (t.group == &g && skip_match) // Note: group query is atomic.
+    if (t.group == &g) // Note: atomic.
       t.path (move (f));
     else
     {
@@ -895,22 +898,13 @@ namespace build2
           info << "dynamically extracted group members cannot be used as "
                << "prerequisites directly, only via group";
 
-      if (!locked)
-      {
-        if (t.group == nullptr)
-          tl.target->group = &g;
-        else if (t.group != &g)
-          fail << "group " << g << " member " << t
-               << " is already member of group " << *t.group;
-      }
+      if (t.group == nullptr)
+        tl.target->group = &g;
+      else if (t.group != &g)
+        fail << "group " << g << " member " << t
+             << " is already member of group " << *t.group;
 
       t.path (move (f));
-
-      if (!skip_match)
-      {
-        match_inc_dependents (a, g);
-        match_recipe (tl, group_recipe);
-      }
     }
 
     return pair<const file&, bool> (t, true);
@@ -927,8 +921,7 @@ namespace build2
     return inject_group_member_impl (a, bs, g,
                                      move (f), move (n).string (), move (e),
                                      tt,
-                                     nullptr /* filter */,
-                                     false).first;
+                                     nullptr /* filter */).first;
   }
 
   static const target_type&
@@ -974,8 +967,7 @@ namespace build2
                        path f,
                        const function<map_extension_func>& map_ext,
                        const target_type& fallback,
-                       const function<group_filter_func>& filter,
-                       bool skip_match)
+                       const function<group_filter_func>& filter)
   {
     path n (f.leaf ());
     string e (n.extension ());
@@ -989,8 +981,7 @@ namespace build2
     return inject_group_member_impl (a, bs, g,
                                      move (f), move (n).string (), move (e),
                                      tt,
-                                     filter,
-                                     skip_match);
+                                     filter);
   }
 
   pair<const file&, bool> dyndep_rule::
diff --git a/libbuild2/dyndep.hxx b/libbuild2/dyndep.hxx
index 07511ee..ee9c1dd 100644
--- a/libbuild2/dyndep.hxx
+++ b/libbuild2/dyndep.hxx
@@ -232,7 +232,8 @@ namespace build2
     // set its path, and match it with group_recipe.
     //
     // The file path must be absolute and normalized. Note that this function
-    // assumes that this member can only be matched via this group.
+    // assumes that this member can only be matched via this group. The group
+    // type must have the target_type::flag::dyn_members flag.
     //
     // Note: we can split this function into {enter,match}_group_member()
     //       if necessary.
@@ -259,9 +260,6 @@ namespace build2
     // Note that the filter is skipped if the target is newly inserted (the
     // filter is meant to be used to skip duplicates).
     //
-    // Note that skip_match is an implementation detail and should not be
-    // used.
-    //
     using group_filter_func = bool (mtime_target& g, const file&);
 
     static pair<const file&, bool>
@@ -270,8 +268,7 @@ namespace build2
                          path,
                          const function<map_extension_func>&,
                          const target_type& fallback,
-                         const function<group_filter_func>& = nullptr,
-                         bool skip_match = false);
+                         const function<group_filter_func>& = nullptr);
 
 
     // Find or insert a target file path as a target, make it a member of the
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index f199827..7355f38 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -774,6 +774,21 @@ namespace build2
 
       if (ctx.dependency_count.load (memory_order_relaxed) != 0)
       {
+        auto dependents = [base = ctx.count_base ()] (action a,
+                                                      const target& t)
+        {
+          const target::opstate& s (t.state[a]);
+
+          // Only consider targets that have been matched for this operation
+          // (since matching is what causes the dependents count reset).
+          //
+          size_t c (s.task_count.load (memory_order_relaxed) - base);
+
+          return (c >= target::offset_applied
+                  ? s.dependents.load (memory_order_relaxed)
+                  : 0);
+        };
+
         diag_record dr;
         dr << info << "detected unexecuted matched targets:";
 
@@ -781,14 +796,12 @@ namespace build2
         {
           const target& t (*pt);
 
-          if (size_t n = t[a].dependents.load (memory_order_relaxed))
+          if (size_t n = dependents (a, t))
             dr << text << t << ' ' << n;
 
           if (a.outer ())
           {
-            action ia (a.inner_action ());
-
-            if (size_t n = t[ia].dependents.load (memory_order_relaxed))
+            if (size_t n = dependents (a.inner_action (), t))
               dr << text << t << ' ' << n;
           }
         }
diff --git a/libbuild2/target-type.hxx b/libbuild2/target-type.hxx
index 735532b..a0fc5a2 100644
--- a/libbuild2/target-type.hxx
+++ b/libbuild2/target-type.hxx
@@ -109,7 +109,8 @@ namespace build2
       none        = 0,
       group       = 0x01,         // A (non-adhoc) group.
       see_through = group | 0x02, // A group with "see through" semantics.
-      member_hint = group | 0x04  // Untyped rule hint applies to members.
+      member_hint = group | 0x04, // Untyped rule hint applies to members.
+      dyn_members = group | 0x08  // A group with dynamic members.
     };
 
     flag flags;
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index c1002b9..6b2c702 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -1263,7 +1263,11 @@ namespace build2
     nullptr,
     nullptr,
     &target_search,
-    target_type::flag::group
+    //
+    // Note that the dyn_members semantics is used not only to handle
+    // depdb-dyndep --dyn-target, but also pattern rule-static members.
+    //
+    target_type::flag::group | target_type::flag::dyn_members
   };
 
   // alias