Fix group link-up race

13 files changed, 98 insertions, 33 deletions

diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx
index 1c5b6a6..38693bc 100644
--- a/build2/algorithm.cxx
+++ b/build2/algorithm.cxx
@@ -554,7 +554,7 @@ namespace build2
   }
 
   group_view
-  resolve_group_members_impl (action a, const target& g, target_lock l)
+  resolve_members_impl (action a, const target& g, target_lock l)
   {
     // Note that we will be unlocked if the target is already applied.
     //
@@ -625,6 +625,12 @@ namespace build2
     return r;
   }
 
+  void
+  resolve_group_impl (action, const target&, target_lock l)
+  {
+    match_impl (l, true /* step */, true /* try_match */);
+  }
+
   template <typename R>
   static void
   match_prerequisite_range (action a, target& t, R&& r, const scope* s)
diff --git a/build2/algorithm.hxx b/build2/algorithm.hxx
index 5287f23..1f8736f 100644
--- a/build2/algorithm.hxx
+++ b/build2/algorithm.hxx
@@ -293,12 +293,12 @@ namespace build2
     match_members (a, t, ts.data () + start, ts.size () - start);
   }
 
-  // Unless already available, match, and, if necessary, execute the group
-  // in order to obtain its members list. Note that even after that the
-  // member's list might still not be available (e.g., if some wildcard/
-  // fallback rule matched).
+  // Unless already known, match, and, if necessary, execute the group in
+  // order to resolve its members list. Note that even after that the member's
+  // list might still not be available (e.g., if some wildcard/ fallback rule
+  // matched).
   //
-  // If the action is is for an outer operation, then it is changed to inner
+  // If the action is for an outer operation, then it is changed to inner
   // which means the members are always resolved by the inner (e.g., update)
   // rule. This feels right since this is the rule that will normally do the
   // work (e.g., update) and therefore knows what it will produce (and if we
@@ -306,7 +306,19 @@ namespace build2
   // two different task_count instances for synchronization).
   //
   group_view
-  resolve_group_members (action, const target&);
+  resolve_members (action, const target&);
+
+  // Unless already known, match the target in order to resolve its group.
+  //
+  // Unlike the member case, a rule can only decide whether a target is a
+  // member of the group in its match() since otherwise it (presumably) should
+  // not match (and some other rule may).
+  //
+  // If the action is for an outer operation, then it is changed to inner, the
+  // same as for members.
+  //
+  const target*
+  resolve_group (action, const target&);
 
   // Inject dependency on the target's directory fsdir{}, unless it is in the
   // src tree or is outside of any project (say, for example, an installation
diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx
index f0330f6..9b1df35 100644
--- a/build2/algorithm.ixx
+++ b/build2/algorithm.ixx
@@ -360,10 +360,10 @@ namespace build2
   }
 
   group_view
-  resolve_group_members_impl (action, const target&, target_lock);
+  resolve_members_impl (action, const target&, target_lock);
 
   inline group_view
-  resolve_group_members (action a, const target& g)
+  resolve_members (action a, const target& g)
   {
     group_view r;
 
@@ -386,7 +386,7 @@ namespace build2
         // we can do, then unlock and return.
         //
         if (r.members == nullptr && l.offset != target::offset_executed)
-          r = resolve_group_members_impl (a, g, move (l));
+          r = resolve_members_impl (a, g, move (l));
 
         break;
       }
@@ -398,6 +398,38 @@ namespace build2
   }
 
   void
+  resolve_group_impl (action, const target&, target_lock);
+
+  inline const target*
+  resolve_group (action a, const target& t)
+  {
+    if (a.outer ())
+      a = a.inner_action ();
+
+    switch (phase)
+    {
+    case run_phase::match:
+      {
+        // Grab a target lock to make sure the group state is synchronized.
+        //
+        target_lock l (lock_impl (a, t, scheduler::work_none));
+
+        // If the group is alrealy known or there is nothing else we can do,
+        // then unlock and return.
+        //
+        if (t.group == nullptr && l.offset < target::offset_tried)
+          resolve_group_impl (a, t, move (l));
+
+        break;
+      }
+    case run_phase::execute: break;
+    case run_phase::load:    assert (false);
+    }
+
+    return t.group;
+  }
+
+  void
   match_prerequisites (action, target&, const scope*);
 
   void
diff --git a/build2/cc/compile-rule.cxx b/build2/cc/compile-rule.cxx
index a6d2d48..632a9bf 100644
--- a/build2/cc/compile-rule.cxx
+++ b/build2/cc/compile-rule.cxx
@@ -3537,6 +3537,7 @@ namespace build2
         // Find the mxx{} prerequisite and extract its "file name" for the
         // fuzzy match unless the user specified the module name explicitly.
         //
+        resolve_group (a, *pt);
         for (prerequisite_member p: group_prerequisite_members (a, *pt))
         {
           if (p.is_a (*x_mod))
@@ -3649,6 +3650,8 @@ namespace build2
 
           if (in != mn)
           {
+            // Note: matched, so the group should be resolved.
+            //
             for (prerequisite_member p: group_prerequisite_members (a, *bt))
             {
               if (p.is_a (*x_mod)) // Got to be there.
@@ -3865,6 +3868,8 @@ namespace build2
       // sort things out. This is pretty similar to what we do in link when
       // synthesizing dependencies for bmi{}'s.
       //
+      // Note: lt is matched and so the group is resolved.
+      //
       ps.push_back (prerequisite (lt));
       for (prerequisite_member p: group_prerequisite_members (a, lt))
       {
diff --git a/build2/cc/install-rule.cxx b/build2/cc/install-rule.cxx
index 0fdb98b..e74104c 100644
--- a/build2/cc/install-rule.cxx
+++ b/build2/cc/install-rule.cxx
@@ -107,14 +107,18 @@ namespace build2
       //
       // Note: for now we assume bmi*{} never come from see-through groups.
       //
-      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
+      bool g (false);
+      if (p.is_a<bmi> () || (g = p.is_a (compile_types (ot).bmi)))
       {
-        // This is tricky: we need to "look" inside groups for mxx{} but if
-        // found, remap to the group, not member.
-        //
+        if (g)
+          resolve_group (a, *pt);
+
         for (prerequisite_member pm:
                group_prerequisite_members (a, *pt, members_mode::maybe))
         {
+          // This is tricky: we need to "look" inside groups for mxx{} but if
+          // found, remap to the group, not member.
+          //
           if (pm.is_a (*x_mod))
           {
             pt = t.is_a<exe> ()
@@ -304,8 +308,12 @@ namespace build2
           return pt;
       }
 
-      if (p.is_a<bmi> () || p.is_a (compile_types (ot).bmi))
+      bool g (false);
+      if (p.is_a<bmi> () || (g = p.is_a (compile_types (ot).bmi)))
       {
+        if (g)
+          resolve_group (a, *pt);
+
         for (prerequisite_member pm:
                group_prerequisite_members (a, *pt, members_mode::maybe))
         {
diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx
index c59597a..bdf75c7 100644
--- a/build2/cc/link-rule.cxx
+++ b/build2/cc/link-rule.cxx
@@ -54,15 +54,10 @@ namespace build2
       // If this is a library, link-up to our group (this is the target group
       // protocol which means this can be done whether we match or not).
       //
-      if (lt.library ())
-      {
-        //@@ inner/outer race (see install-rule)?
-
-        if (t.group == nullptr)
-          t.group = &search (t,
-                             lt.utility ? libu::static_type : lib::static_type,
-                             t.dir, t.out, t.name);
-      }
+      if (lt.library () && t.group == nullptr)
+        t.group = &search (t,
+                           lt.utility ? libu::static_type : lib::static_type,
+                           t.dir, t.out, t.name);
 
       // Scan prerequisites and see if we can work with what we've got. Note
       // that X could be C. We handle this by always checking for X first.
@@ -777,6 +772,8 @@ namespace build2
                                     ? (group ? bmi::static_type : tt.bmi)
                                     : (group ? obj::static_type : tt.obj));
 
+            resolve_group (a, *pt); // Not matched yet so resolve group.
+
             bool src (false);
             for (prerequisite_member p1: group_prerequisite_members (a, *pt))
             {
@@ -904,6 +901,8 @@ namespace build2
         //
         bool mod (x_mod != nullptr && p.is_a (*x_mod));
 
+        // Note: group already resolved in the previous loop.
+
         for (prerequisite_member p1: group_prerequisite_members (a, *pt))
         {
           if (p1.is_a (mod ? *x_mod : x_src) || p1.is_a<c> ())
diff --git a/build2/cc/utility.cxx b/build2/cc/utility.cxx
index aa47085..74da443 100644
--- a/build2/cc/utility.cxx
+++ b/build2/cc/utility.cxx
@@ -63,7 +63,7 @@ namespace build2
 
         // Make sure group members are resolved.
         //
-        group_view gv (resolve_group_members (a, l));
+        group_view gv (resolve_members (a, l));
         assert (gv.members != nullptr);
 
         lorder lo (li.order);
diff --git a/build2/install/rule.cxx b/build2/install/rule.cxx
index 00166c2..18cbeae 100644
--- a/build2/install/rule.cxx
+++ b/build2/install/rule.cxx
@@ -157,7 +157,7 @@ namespace build2
       // If the rule could not resolve the group, then we ignore it.
       //
       group_view gv (a.outer ()
-                     ? resolve_group_members (a, t)
+                     ? resolve_members (a, t)
                      : t.group_members (a));
 
       if (gv.members != nullptr)
diff --git a/build2/operation.hxx b/build2/operation.hxx
index 749d65a..27f2356 100644
--- a/build2/operation.hxx
+++ b/build2/operation.hxx
@@ -58,7 +58,7 @@ namespace build2
   // While most of the relevant target state is duplicated, certain things are
   // shared among the inner/outer rules, such as the target data pad and the
   // group state. In particular, it is assumed the group state is always
-  // determined by the inner rule (see resolve_group_members()).
+  // determined by the inner rule (see resolve_members()).
   //
   // Normally, an outer rule will be responsible for any additional, outer
   // operation-specific work. Sometimes, however, the inner rule needs to
diff --git a/build2/target.hxx b/build2/target.hxx
index 23213a2..e783ee4 100644
--- a/build2/target.hxx
+++ b/build2/target.hxx
@@ -252,8 +252,8 @@ namespace build2
     }
 
   public:
-    // You should not call this function directly; rather use
-    // resolve_group_members() from <build2/algorithm.hxx>.
+    // Normally you should not call this function directly and rather use
+    // resolve_members() from algorithm.hxx.
     //
     virtual group_view
     group_members (action) const;
@@ -697,6 +697,9 @@ namespace build2
   // also be traversed in reverse, but that's what you usually want,
   // anyway.
   //
+  // Note that you either should be iterating over a locked target (e.g., in
+  // rule's match() or apply()) or you should call resolve_group().
+  //
   class group_prerequisites
   {
   public:
diff --git a/build2/target.ixx b/build2/target.ixx
index cf1eec9..3148ab4 100644
--- a/build2/target.ixx
+++ b/build2/target.ixx
@@ -201,7 +201,7 @@ namespace build2
   // prerequisite_members
   //
   group_view
-  resolve_group_members (action, const target&); // <build2/algorithm.hxx>
+  resolve_members (action, const target&); // algorithm.hxx
 
   template <typename T>
   inline auto prerequisite_members_range<T>::iterator::
@@ -248,7 +248,7 @@ namespace build2
     {
       // Otherwise assume it is a normal group.
       //
-      g_ = resolve_group_members (r_->a_, search (r_->t_, *i_));
+      g_ = resolve_members (r_->a_, search (r_->t_, *i_));
 
       if (g_.members == nullptr) // Members are not know.
       {
diff --git a/build2/target.txx b/build2/target.txx
index 065c8f2..a5d6728 100644
--- a/build2/target.txx
+++ b/build2/target.txx
@@ -20,7 +20,7 @@ namespace build2
     //
     do
     {
-      g_ = resolve_group_members (r_->a_, search (r_->t_, *i_));
+      g_ = resolve_members (r_->a_, search (r_->t_, *i_));
 
       // Group could not be resolved.
       //
diff --git a/build2/test/rule.cxx b/build2/test/rule.cxx
index 392c9d5..b21f0c3 100644
--- a/build2/test/rule.cxx
+++ b/build2/test/rule.cxx
@@ -71,7 +71,7 @@ namespace build2
         // If the rule could not resolve the group, then we ignore it.
         //
         group_view gv (a.outer ()
-                       ? resolve_group_members (a, t)
+                       ? resolve_members (a, t)
                        : t.group_members (a));
 
         if (gv.members != nullptr)