Add more instrumentation for unassigned path race

5 files changed, 64 insertions, 25 deletions

diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx
index de09469..33bc624 100644
--- a/libbuild2/algorithm.hxx
+++ b/libbuild2/algorithm.hxx
@@ -287,8 +287,8 @@ namespace build2
   // If it is unmatch::unchanged then only unmatch the target if it is known
   // 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 if unmatch succeeded. Always
-  // throw if failed.
+  // else will execute this target). Return true in first half of the pair if
+  // unmatch succeeded. Always throw if failed.
   //
   enum class unmatch {none, unchanged, safe};
 
@@ -298,7 +298,7 @@ namespace build2
   pair<bool, target_state>
   try_match (action, const target&, bool fail = true);
 
-  bool
+  pair<bool, target_state>
   match (action, const target&, unmatch);
 
   // Start asynchronous match. Return target_state::postponed if the
@@ -337,7 +337,7 @@ namespace build2
   target_state
   match_inner (action, const target&);
 
-  bool
+  pair<bool, target_state>
   match_inner (action, const target&, unmatch);
 
   // The standard prerequisite search and match implementations. They call
diff --git a/libbuild2/algorithm.ixx b/libbuild2/algorithm.ixx
index 5f9143a..d75ad16 100644
--- a/libbuild2/algorithm.ixx
+++ b/libbuild2/algorithm.ixx
@@ -312,7 +312,7 @@ namespace build2
     return r;
   }
 
-  inline bool
+  inline pair<bool, target_state>
   match (action a, const target& t, unmatch um)
   {
     assert (t.ctx.phase == run_phase::match);
@@ -328,7 +328,7 @@ namespace build2
     case unmatch::unchanged:
       {
         if (s == target_state::unchanged)
-          return true;
+          return make_pair (true, s);
 
         break;
       }
@@ -340,14 +340,14 @@ namespace build2
         //
         if (s == target_state::unchanged                   ||
             t[a].dependents.load (memory_order_consume) != 0)
-          return true;
+          return make_pair (true, s);
 
         break;
       }
     }
 
     match_inc_dependens (a, t);
-    return false;
+    return make_pair (false, s);;
   }
 
   inline target_state
@@ -437,7 +437,7 @@ namespace build2
     return match (a.inner_action (), t);
   }
 
-  inline bool
+  inline pair<bool, target_state>
   match_inner (action a, const target& t, unmatch um)
   {
     assert (a.outer ());
diff --git a/libbuild2/cc/compile-rule.cxx b/libbuild2/cc/compile-rule.cxx
index 55bfc2c..5e757f1 100644
--- a/libbuild2/cc/compile-rule.cxx
+++ b/libbuild2/cc/compile-rule.cxx
@@ -686,6 +686,9 @@ namespace build2
       //
       wait_guard wg (ctx, ctx.count_busy (), t[a].task_count, true);
 
+      target_state src_ts1, src_ts2;
+
+      size_t src_i (~0);          // Index of src target.
       size_t start (pts.size ()); // Index of the first to be added.
       for (prerequisite_member p: group_prerequisite_members (a, t))
       {
@@ -737,7 +740,9 @@ namespace build2
         else if (pi == include_type::normal &&
                  (p.is_a<bmi> ()  || p.is_a (tts.bmi) ||
                   p.is_a<hbmi> () || p.is_a (tts.hbmi)))
+        {
           continue;
+        }
         else
         {
           pt = &p.search (t);
@@ -746,12 +751,24 @@ namespace build2
             continue;
         }
 
-        match_async (a, *pt, ctx.count_busy (), t[a].task_count);
+        target_state ts (
+          match_async (a, *pt, ctx.count_busy (), t[a].task_count));
+
+        if (p == md.src)
+        {
+          src_i = pts.size ();
+          src_ts1 = ts;
+        }
+
         pts.push_back (prerequisite_target (pt, pi));
       }
 
+      size_t src_tc1 (t[a].task_count.load (memory_order_consume));
+
       wg.wait ();
 
+      size_t src_tc2 (t[a].task_count.load (memory_order_consume));
+
       // Finish matching all the targets that we have started.
       //
       for (size_t i (start), n (pts.size ()); i != n; ++i)
@@ -765,13 +782,18 @@ namespace build2
         // match in link::apply() it will be safe unless someone is building
         // an obj?{} target directory.
         //
-        if (build2::match (
-              a,
-              *pt,
-              pt->is_a<liba> () || pt->is_a<libs> () || pt->is_a<libux> ()
-              ? unmatch::safe
-              : unmatch::none))
+        pair<bool, target_state> mr (
+          build2::match (
+            a,
+            *pt,
+            pt->is_a<liba> () || pt->is_a<libs> () || pt->is_a<libux> ()
+            ? unmatch::safe
+            : unmatch::none));
+
+        if (mr.first)
           pt = nullptr; // Ignore in execute.
+        else if (i == src_i)
+          src_ts2 = mr.second;
       }
 
       // Inject additional prerequisites. We only do it when performing update
@@ -780,11 +802,7 @@ namespace build2
       //
       if (a == perform_update_id)
       {
-        // The cached prerequisite target should be the same as what is in
-        // t.prerequisite_targets since we used standard search() and match()
-        // above.
-        //
-        const file& src (*md.src.load (memory_order_relaxed)->is_a<file> ());
+        const file& src (pts[src_i]->as<file> ());
 
         // Figure out if __symexport is used. While normally it is specified
         // on the project root (which we cached), it can be overridden with
@@ -902,12 +920,21 @@ namespace build2
           // We seem to have a race condition here but can't quite put our
           // finger on it.
           //
+          // NOTE: remember to get rid of src_ts*, etc., once done.
+          //
           // assert (!p.empty ()); // Sanity check.
           if (p.empty ())
           {
+            target_state src_ts3 (src.matched_state (a, false));
+
             info << "unassigned path for target " << src <<
-              info << "target state: " << src.matched_state (a, false) <<
-              info << "is empty_path: " << (&p == &empty_path);
+              info << "is empty_path: " << (&p == &empty_path) <<
+              info << "target state 1: " << src_ts1 <<
+              info << "target state 2: " << src_ts2 <<
+              info << "target state 3: " << src_ts3 <<
+              info << "target count 1: " << src_tc1 <<
+              info << "target count 2: " << src_tc2;
+
             assert (false);
           }
 
diff --git a/libbuild2/install/rule.cxx b/libbuild2/install/rule.cxx
index b8d6a34..901b048 100644
--- a/libbuild2/install/rule.cxx
+++ b/libbuild2/install/rule.cxx
@@ -313,7 +313,7 @@ namespace build2
       //
       optional<bool> unchanged;
       if (a.operation () == update_id)
-        unchanged = match_inner (a, t, unmatch::unchanged);
+        unchanged = match_inner (a, t, unmatch::unchanged).first;
 
       auto& pts (t.prerequisite_targets[a]);
 
@@ -367,7 +367,7 @@ namespace build2
           // when updating static installable content (headers, documentation,
           // etc).
           //
-          if (build2::match (a, *pt, unmatch::unchanged))
+          if (build2::match (a, *pt, unmatch::unchanged).first)
             pt = nullptr;
         }
         else if (!try_match (a, *pt).first)
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index c2954f8..d4b287b 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -1013,6 +1013,18 @@ namespace build2
   static_assert (std::is_trivially_destructible<prerequisite_member>::value,
                  "prerequisite_member is not trivially destructible");
 
+  inline bool
+  operator== (const prerequisite_member& x, const prerequisite_member& y)
+  {
+    return &x.prerequisite == &y.prerequisite && x.member == y.member;
+  }
+
+  inline bool
+  operator!= (const prerequisite_member& x, const prerequisite_member& y)
+  {
+    return !(x == y);
+  }
+
   inline ostream&
   operator<< (ostream& os, const prerequisite_member& pm)
   {