From a17b6b40510b8ec5ca18dd5203e4b229aa6fee8a Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 25 Jun 2020 09:00:12 +0200 Subject: Add more instrumentation for unassigned path race --- libbuild2/algorithm.hxx | 8 +++---- libbuild2/algorithm.ixx | 10 ++++---- libbuild2/cc/compile-rule.cxx | 55 ++++++++++++++++++++++++++++++++----------- libbuild2/install/rule.cxx | 4 ++-- libbuild2/target.hxx | 12 ++++++++++ 5 files changed, 64 insertions(+), 25 deletions(-) (limited to 'libbuild2') 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 try_match (action, const target&, bool fail = true); - bool + pair 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 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 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 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 () || p.is_a (tts.bmi) || p.is_a () || 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 () || pt->is_a () || pt->is_a () - ? unmatch::safe - : unmatch::none)) + pair mr ( + build2::match ( + a, + *pt, + pt->is_a () || pt->is_a () || pt->is_a () + ? 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 ()); + const file& src (pts[src_i]->as ()); // 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 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::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) { -- cgit v1.1