aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2023-05-26 07:22:40 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2023-05-29 10:21:12 +0200
commitab91d32c1b23ea92b988d5618db2938a8c5dc63f (patch)
treefc02b89ae71aa3bcad127149718a7a4e7480f0b7
parent56d79a62e64180f639dad02f0887fef5d57bb096 (diff)
Avoid group linkup deadlocks for dynamic and pattern-static members
-rw-r--r--libbuild2/adhoc-rule-regex-pattern.cxx36
-rw-r--r--libbuild2/dyndep.cxx58
-rw-r--r--libbuild2/target.hxx7
3 files changed, 64 insertions, 37 deletions
diff --git a/libbuild2/adhoc-rule-regex-pattern.cxx b/libbuild2/adhoc-rule-regex-pattern.cxx
index 0f2ecc3..c4b4cab 100644
--- a/libbuild2/adhoc-rule-regex-pattern.cxx
+++ b/libbuild2/adhoc-rule-regex-pattern.cxx
@@ -400,21 +400,31 @@ namespace build2
if (find (ms.begin (), ms.end (), &t) != ms.end ())
continue;
- // We can only update the group under lock.
+ // 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?).
//
- target_lock tl (lock (a, t));
-
- if (!tl)
- fail << "group " << *g << " member " << t << " is already matched" <<
- info << "static group members specified by pattern rules cannot "
- << "be used as prerequisites directly, only via group";
-
- if (t.group == nullptr)
- tl.target->group = g;
- else if (t.group != g)
+ if (t.group != g) // Note: atomic.
{
- fail << "group " << *g << " member " << t
- << " is already member of group " << *t.group;
+ // We can only update the group under lock.
+ //
+ target_lock tl (lock (a, t));
+
+ if (!tl)
+ fail << "group " << *g << " member " << t << " is already matched" <<
+ info << "static group members specified by pattern rules cannot "
+ << "be used as prerequisites directly, only via 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;
+ }
}
}
diff --git a/libbuild2/dyndep.cxx b/libbuild2/dyndep.cxx
index 8781812..5ac9648 100644
--- a/libbuild2/dyndep.cxx
+++ b/libbuild2/dyndep.cxx
@@ -834,6 +834,9 @@ namespace build2
const function<dyndep_rule::group_filter_func>& fl,
bool skip_match)
{
+ // NOTE: see adhoc_rule_regex_pattern::apply_group_members() for a variant
+ // of the same code.
+
// We expect that nobody else can insert these members (seems reasonable
// seeing that their names are dynamically discovered).
//
@@ -870,33 +873,44 @@ namespace build2
return pair<const file&, bool> (t, false);
}
- // This shouldn't normally fail since we are the only ones that should
- // know about this target (otherwise why is it dynamicaly discovered).
- // However, nothing prevents the user from depending on such a target,
- // however misguided.
+ // 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?).
//
- target_lock tl (lock (a, t));
+ if (t.group == &g && skip_match) // Note: group query is atomic.
+ t.path (move (f));
+ else
+ {
+ // This shouldn't normally fail since we are the only ones that should
+ // know about this target (otherwise why is it dynamicaly discovered).
+ // However, nothing prevents the user from depending on such a target,
+ // however misguided.
+ //
+ target_lock tl (lock (a, t));
- if (!tl)
- fail << "group " << g << " member " << t << " is already matched" <<
- info << "dynamically extracted group members cannot be used as "
- << "prerequisites directly, only via group";
+ if (!tl)
+ fail << "group " << g << " member " << t << " is already matched" <<
+ 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 (!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;
+ }
- t.path (move (f));
+ t.path (move (f));
- if (!skip_match)
- {
- match_inc_dependents (a, g);
- match_recipe (tl, group_recipe);
+ if (!skip_match)
+ {
+ match_inc_dependents (a, g);
+ match_recipe (tl, group_recipe);
+ }
}
return pair<const file&, bool> (t, true);
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 3f73e63..c7b1131 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -380,9 +380,12 @@ namespace build2
//
// Note that the group-member link-up can happen anywhere between the
// member creation and rule matching so reading the group before the
- // member has been matched can be racy.
+ // member has been matched can be racy. However, once the member is linked
+ // up to the group, this relationship is immutable. As a result, one can
+ // atomically query the current value to see if already linked up (can be
+ // used as an optimization, to avoid deadlocks, etc).
//
- const target* group = nullptr;
+ relaxed_atomic<const target*> group = nullptr;
// What has been described above is an "explicit" group. That is, there is
// a dedicated target type that explicitly serves as a group and there is