Avoid group linkup deadlocks for dynamic and pattern-static members

3 files changed, 64 insertions, 37 deletions

diff --git a/libbuild2/adhoc-rule-regex-pattern.cxx b/libbuild2/adhoc-rule-regex-pattern.cxx
index 0f2ecc3..c4b4cab 100644
--- a/libbuild2/adhoc-rule-regex-pattern.cxx
+++ b/libbuild2/adhoc-rule-regex-pattern.cxx
@@ -400,21 +400,31 @@ namespace build2
           if (find (ms.begin (), ms.end (), &t) != ms.end ())
             continue;
 
-          // We can only update the group under lock.
+          // 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?).
           //
-          target_lock tl (lock (a, t));
-
-          if (!tl)
-            fail << "group " << *g << " member " << t << " is already matched" <<
-              info << "static group members specified by pattern rules cannot "
-                   << "be used as prerequisites directly, only via group";
-
-          if (t.group == nullptr)
-            tl.target->group = g;
-          else if (t.group != g)
+          if (t.group != g) // Note: atomic.
           {
-            fail << "group " << *g << " member " << t
-                 << " is already member of group " << *t.group;
+            // We can only update the group under lock.
+            //
+            target_lock tl (lock (a, t));
+
+            if (!tl)
+              fail << "group " << *g << " member " << t << " is already matched" <<
+                info << "static group members specified by pattern rules cannot "
+                     << "be used as prerequisites directly, only via 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;
+            }
           }
         }
 
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index 8781812..5ac9648 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -834,6 +834,9 @@ namespace build2
                             const function<dyndep_rule::group_filter_func>& fl,
                             bool skip_match)
   {
+    // NOTE: see adhoc_rule_regex_pattern::apply_group_members() for a variant
+    //       of the same code.
+
     // We expect that nobody else can insert these members (seems reasonable
     // seeing that their names are dynamically discovered).
     //
@@ -870,33 +873,44 @@ namespace build2
         return pair<const file&, bool> (t, false);
     }
 
-    // This shouldn't normally fail since we are the only ones that should
-    // know about this target (otherwise why is it dynamicaly discovered).
-    // However, nothing prevents the user from depending on such a target,
-    // however misguided.
+    // 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?).
     //
-    target_lock tl (lock (a, t));
+    if (t.group == &g && skip_match) // Note: group query is atomic.
+      t.path (move (f));
+    else
+    {
+      // This shouldn't normally fail since we are the only ones that should
+      // know about this target (otherwise why is it dynamicaly discovered).
+      // However, nothing prevents the user from depending on such a target,
+      // however misguided.
+      //
+      target_lock tl (lock (a, t));
 
-    if (!tl)
-      fail << "group " << g << " member " << t << " is already matched" <<
-        info << "dynamically extracted group members cannot be used as "
-             << "prerequisites directly, only via group";
+      if (!tl)
+        fail << "group " << g << " member " << t << " is already matched" <<
+          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 (!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;
+      }
 
-    t.path (move (f));
+      t.path (move (f));
 
-    if (!skip_match)
-    {
-      match_inc_dependents (a, g);
-      match_recipe (tl, group_recipe);
+      if (!skip_match)
+      {
+        match_inc_dependents (a, g);
+        match_recipe (tl, group_recipe);
+      }
     }
 
     return pair<const file&, bool> (t, true);
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 3f73e63..c7b1131 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -380,9 +380,12 @@ namespace build2
     //
     // Note that the group-member link-up can happen anywhere between the
     // member creation and rule matching so reading the group before the
-    // member has been matched can be racy.
+    // member has been matched can be racy. However, once the member is linked
+    // up to the group, this relationship is immutable. As a result, one can
+    // atomically query the current value to see if already linked up (can be
+    // used as an optimization, to avoid deadlocks, etc).
     //
-    const target* group = nullptr;
+    relaxed_atomic<const target*> group = nullptr;
 
     // What has been described above is an "explicit" group. That is, there is
     // a dedicated target type that explicitly serves as a group and there is