aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2023-05-29 07:56:33 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2023-05-29 14:12:00 +0200
commit9bea2f465cc2b47e06d65d6a29cb0f0f0c37f29c (patch)
tree5eb14ac196fce453c33c06c497e25b8d8f9259a1
parent59014204d94e67d243cce45ff83ca85212237433 (diff)
Extend special match_rule() logic to all groups with dynamic targets
-rw-r--r--libbuild2/adhoc-rule-buildscript.cxx12
-rw-r--r--libbuild2/adhoc-rule-regex-pattern.cxx14
-rw-r--r--libbuild2/algorithm.cxx51
-rw-r--r--libbuild2/build/script/parser.cxx2
-rw-r--r--libbuild2/dyndep.cxx63
-rw-r--r--libbuild2/dyndep.hxx9
-rw-r--r--libbuild2/operation.cxx21
-rw-r--r--libbuild2/target-type.hxx3
-rw-r--r--libbuild2/target.cxx6
9 files changed, 89 insertions, 92 deletions
diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index cd760b4..cf45699 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -295,8 +295,7 @@ namespace build2
{
tracer trace ("adhoc_buildscript_rule::apply");
- // Handle matching explicit group members (see adhoc_rule::match() for
- // background).
+ // Handle matching group members (see adhoc_rule::match() for background).
//
if (const group* g = t.group != nullptr ? t.group->is_a<group> () : nullptr)
{
@@ -681,11 +680,10 @@ namespace build2
if (g != nullptr)
{
pair<const build2::file&, bool> r (
- dyndep::inject_group_member (
- what,
- a, bs, *g,
- move (f),
- map_ext, def_tt, filter, true /* skip_match */));
+ dyndep::inject_group_member (what,
+ a, bs, *g,
+ move (f),
+ map_ext, def_tt, filter));
if (r.second)
g->members.push_back (&r.first);
diff --git a/libbuild2/adhoc-rule-regex-pattern.cxx b/libbuild2/adhoc-rule-regex-pattern.cxx
index 3f207dc..257952f 100644
--- a/libbuild2/adhoc-rule-regex-pattern.cxx
+++ b/libbuild2/adhoc-rule-regex-pattern.cxx
@@ -370,7 +370,8 @@ namespace build2
auto& ms (g->members);
// These are conceptually static but they behave more like dynamic in
- // that we likely need to insert the target, set its group, etc.
+ // that we likely need to insert the target, set its group, etc. In a
+ // sense, they are rule-static, but group-dynamic.
//
// Note: a custom version of the dyndep_rule::inject_group_member()
// logic.
@@ -386,10 +387,6 @@ namespace build2
const target& t (l.first); // Note: non-const only if have lock.
- // Note: we don't need to match the group recipe directy due to the
- // special ad hoc recipe/rule semantics for explicit group members
- // in match_rule().
- //
if (l.second)
{
l.first.group = g;
@@ -400,13 +397,6 @@ namespace build2
if (find (ms.begin (), ms.end (), &t) != ms.end ())
continue;
- // 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?).
- //
if (t.group != g) // Note: atomic.
{
// We can only update the group under lock.
diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index cf972e7..581da2e 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -521,42 +521,45 @@ namespace build2
return dynamic_cast<const fallback_rule*> (&r.second.get ());
};
- // If this is a member of group-based target, then first try to find a
- // matching ad hoc recipe/rule by matching (to an ad hoc recipe/rule) the
- // group but applying to the member. See adhoc_rule::match() for
- // background, including for why const_cast should be safe.
- //
- // To put it another way, if a group is matched by an ad hoc recipe/rule,
- // then we want all the member to be matched to the same recipe/rule.
- //
- if (const group* g = t.group != nullptr ? t.group->is_a<group> () : nullptr)
+ if (const target* g = t.group)
{
- assert (pme == nullptr);
-
- // As an optimization, check if the group is already matched for this
- // action. Note that this can become important when matching adhoc regex
- // rules since we can potentially call match() for many members. This is
- // probably ok for static members (of which we don't expect more than a
- // handful) but can become an issue for dynamic members.
+ // If this is a group with dynamic members, then match it with the
+ // group's rule automatically. See dyndep_rule::inject_group_member()
+ // for background.
//
- if (g->matched (a, memory_order_acquire))
+ if ((g->type ().flags & target_type::flag::dyn_members) ==
+ target_type::flag::dyn_members)
{
- const rule_match* r (g->state[a].rule);
-
- if (r != nullptr && adhoc_rule_match (*r))
+ if (g->matched (a, memory_order_acquire))
+ {
+ const rule_match* r (g->state[a].rule);
+ assert (r != nullptr); // Shouldn't happen with dyn_members.
return r;
+ }
- // Fall through: if some rule matched the group then it must also deal
- // with the members.
+ // Assume static member and fall through.
}
- else
+
+ // If this is a member of group-based target, then first try to find a
+ // matching ad hoc recipe/rule by matching (to an ad hoc recipe/rule) the
+ // group but applying to the member. See adhoc_rule::match() for
+ // background, including for why const_cast should be safe.
+ //
+ // To put it another way, if a group is matched by an ad hoc
+ // recipe/rule, then we want all the member to be matched to the same
+ // recipe/rule.
+ //
+ // Note that such a group is dyn_members so we would have tried the
+ // "already matched" case above.
+ //
+ if (g->is_a<group> ())
{
// We cannot init match_extra from the target if it's unlocked so use
// a temporary (it shouldn't be modified if unlocked).
//
match_extra me (false /* locked */);
if (const rule_match* r = match_rule (
- a, const_cast<group&> (*g), skip, true /* try_match */, &me))
+ a, const_cast<target&> (*g), skip, true /* try_match */, &me))
return r;
// Fall through to normal match of the member.
diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index 85fe429..7d8a881 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -2968,7 +2968,7 @@ namespace build2
what_tgt,
a, bs, *g,
f, // Can't move since need to return dyn_targets.
- map_ext, *def_tt, filter, true /* skip_match */));
+ map_ext, *def_tt, filter));
// Note: no target_decl shenanigans since reset the members on
// each update.
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index 5ac9648..9c0f8a8 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -831,12 +831,21 @@ namespace build2
inject_group_member_impl (action a, const scope& bs, mtime_target& g,
path f, string n, string e,
const target_type& tt,
- const function<dyndep_rule::group_filter_func>& fl,
- bool skip_match)
+ const function<dyndep_rule::group_filter_func>& fl)
{
// NOTE: see adhoc_rule_regex_pattern::apply_group_members() for a variant
// of the same code.
+ // Note that we used to directly match such a member with group_recipe.
+ // But that messes up our dependency counts since we don't really know
+ // whether someone will execute such a member.
+ //
+ // So instead we now just link the member up to the group and rely on the
+ // special semantics in match_rule() for groups with the dyn_members flag.
+ //
+ assert ((g.type ().flags & target_type::flag::dyn_members) ==
+ target_type::flag::dyn_members);
+
// We expect that nobody else can insert these members (seems reasonable
// seeing that their names are dynamically discovered).
//
@@ -851,21 +860,15 @@ namespace build2
const file& t (l.first.as<file> ()); // Note: non-const only if have lock.
- bool locked (l.second);
- if (locked)
+ // We don't need to match the group recipe directy from ad hoc
+ // recipes/rules due to the special semantics for explicit group members
+ // in match_rule(). This is what skip_match is for.
+ if (l.second)
{
l.first.group = &g;
l.second.unlock ();
-
- // We don't need to match the group recipe directy from ad hoc
- // recipes/rules due to the special semantics for explicit group members
- // in match_rule(). This is what skip_match is for.
- //
- if (skip_match)
- {
- t.path (move (f));
- return pair<const file&, bool> (t, true);
- }
+ t.path (move (f));
+ return pair<const file&, bool> (t, true);
}
else
{
@@ -877,9 +880,9 @@ namespace build2
// 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?).
+ // directly... which was not quite correct and is now fixed).
//
- if (t.group == &g && skip_match) // Note: group query is atomic.
+ if (t.group == &g) // Note: atomic.
t.path (move (f));
else
{
@@ -895,22 +898,13 @@ namespace build2
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 (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));
-
- if (!skip_match)
- {
- match_inc_dependents (a, g);
- match_recipe (tl, group_recipe);
- }
}
return pair<const file&, bool> (t, true);
@@ -927,8 +921,7 @@ namespace build2
return inject_group_member_impl (a, bs, g,
move (f), move (n).string (), move (e),
tt,
- nullptr /* filter */,
- false).first;
+ nullptr /* filter */).first;
}
static const target_type&
@@ -974,8 +967,7 @@ namespace build2
path f,
const function<map_extension_func>& map_ext,
const target_type& fallback,
- const function<group_filter_func>& filter,
- bool skip_match)
+ const function<group_filter_func>& filter)
{
path n (f.leaf ());
string e (n.extension ());
@@ -989,8 +981,7 @@ namespace build2
return inject_group_member_impl (a, bs, g,
move (f), move (n).string (), move (e),
tt,
- filter,
- skip_match);
+ filter);
}
pair<const file&, bool> dyndep_rule::
diff --git a/libbuild2/dyndep.hxx b/libbuild2/dyndep.hxx
index 07511ee..ee9c1dd 100644
--- a/libbuild2/dyndep.hxx
+++ b/libbuild2/dyndep.hxx
@@ -232,7 +232,8 @@ namespace build2
// set its path, and match it with group_recipe.
//
// The file path must be absolute and normalized. Note that this function
- // assumes that this member can only be matched via this group.
+ // assumes that this member can only be matched via this group. The group
+ // type must have the target_type::flag::dyn_members flag.
//
// Note: we can split this function into {enter,match}_group_member()
// if necessary.
@@ -259,9 +260,6 @@ namespace build2
// Note that the filter is skipped if the target is newly inserted (the
// filter is meant to be used to skip duplicates).
//
- // Note that skip_match is an implementation detail and should not be
- // used.
- //
using group_filter_func = bool (mtime_target& g, const file&);
static pair<const file&, bool>
@@ -270,8 +268,7 @@ namespace build2
path,
const function<map_extension_func>&,
const target_type& fallback,
- const function<group_filter_func>& = nullptr,
- bool skip_match = false);
+ const function<group_filter_func>& = nullptr);
// Find or insert a target file path as a target, make it a member of the
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index f199827..7355f38 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -774,6 +774,21 @@ namespace build2
if (ctx.dependency_count.load (memory_order_relaxed) != 0)
{
+ auto dependents = [base = ctx.count_base ()] (action a,
+ const target& t)
+ {
+ const target::opstate& s (t.state[a]);
+
+ // Only consider targets that have been matched for this operation
+ // (since matching is what causes the dependents count reset).
+ //
+ size_t c (s.task_count.load (memory_order_relaxed) - base);
+
+ return (c >= target::offset_applied
+ ? s.dependents.load (memory_order_relaxed)
+ : 0);
+ };
+
diag_record dr;
dr << info << "detected unexecuted matched targets:";
@@ -781,14 +796,12 @@ namespace build2
{
const target& t (*pt);
- if (size_t n = t[a].dependents.load (memory_order_relaxed))
+ if (size_t n = dependents (a, t))
dr << text << t << ' ' << n;
if (a.outer ())
{
- action ia (a.inner_action ());
-
- if (size_t n = t[ia].dependents.load (memory_order_relaxed))
+ if (size_t n = dependents (a.inner_action (), t))
dr << text << t << ' ' << n;
}
}
diff --git a/libbuild2/target-type.hxx b/libbuild2/target-type.hxx
index 735532b..a0fc5a2 100644
--- a/libbuild2/target-type.hxx
+++ b/libbuild2/target-type.hxx
@@ -109,7 +109,8 @@ namespace build2
none = 0,
group = 0x01, // A (non-adhoc) group.
see_through = group | 0x02, // A group with "see through" semantics.
- member_hint = group | 0x04 // Untyped rule hint applies to members.
+ member_hint = group | 0x04, // Untyped rule hint applies to members.
+ dyn_members = group | 0x08 // A group with dynamic members.
};
flag flags;
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index c1002b9..6b2c702 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -1263,7 +1263,11 @@ namespace build2
nullptr,
nullptr,
&target_search,
- target_type::flag::group
+ //
+ // Note that the dyn_members semantics is used not only to handle
+ // depdb-dyndep --dyn-target, but also pattern rule-static members.
+ //
+ target_type::flag::group | target_type::flag::dyn_members
};
// alias