From 63d1d6f8f4bb6db482b21e728245ebf9eee6b55f Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 16 Feb 2018 11:30:41 +0200 Subject: Fix group link-up race --- build2/algorithm.cxx | 8 +++++++- build2/algorithm.hxx | 24 ++++++++++++++++++------ build2/algorithm.ixx | 38 +++++++++++++++++++++++++++++++++++--- build2/cc/compile-rule.cxx | 5 +++++ build2/cc/install-rule.cxx | 18 +++++++++++++----- build2/cc/link-rule.cxx | 17 ++++++++--------- build2/cc/utility.cxx | 2 +- build2/install/rule.cxx | 2 +- build2/operation.hxx | 2 +- build2/target.hxx | 7 +++++-- build2/target.ixx | 4 ++-- build2/target.txx | 2 +- build2/test/rule.cxx | 2 +- 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 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 () || p.is_a (compile_types (ot).bmi)) + bool g (false); + if (p.is_a () || (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 () @@ -304,8 +308,12 @@ namespace build2 return pt; } - if (p.is_a () || p.is_a (compile_types (ot).bmi)) + bool g (false); + if (p.is_a () || (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 ()) 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 . + // 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&); // + resolve_members (action, const target&); // algorithm.hxx template inline auto prerequisite_members_range::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) -- cgit v1.1