aboutsummaryrefslogtreecommitdiff
path: root/libbuild2
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2023-11-03 08:05:46 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2023-11-03 08:05:46 +0200
commit066980729c57e00abc3765053cf3f39e3d223a54 (patch)
tree0497ed9b08837dfc1f301851b5f2e2dcf248caad /libbuild2
parent1c6096e53a906d7821a401d91b32ca02df3d715f (diff)
Fix data race in match options logic
Diffstat (limited to 'libbuild2')
-rw-r--r--libbuild2/algorithm.cxx23
-rw-r--r--libbuild2/algorithm.ixx1
-rw-r--r--libbuild2/target.hxx22
3 files changed, 36 insertions, 10 deletions
diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 1f2d88b..0e995d5 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -291,7 +291,12 @@ namespace build2
// We don't lock already applied or executed targets unless there
// are new options.
//
- if (e >= appl && (cs.match_extra.cur_options & options) == options)
+ // Note: we don't have the lock yet so we must use atomic cur_options.
+ // We also have to re-check this once we've grabbed the lock.
+ //
+ if (e >= appl &&
+ (cs.match_extra.cur_options_.load (memory_order_relaxed) & options)
+ == options)
return target_lock {a, nullptr, e - b, false};
}
@@ -312,7 +317,21 @@ namespace build2
offset = target::offset_touched;
}
else
+ {
+ // Re-check the options if already applied or worse.
+ //
+ if (e >= appl && (s.match_extra.cur_options & options) == options)
+ {
+ // Essentially unlock_impl().
+ //
+ task_count.store (e, memory_order_release);
+ ctx.sched->resume (task_count);
+
+ return target_lock {a, nullptr, e - b, false};
+ }
+
offset = e - b;
+ }
return target_lock {a, &t, offset, first};
}
@@ -962,6 +981,7 @@ namespace build2
recipe re (ar != nullptr ? f (*ar, a, t, me) : ru.apply (a, t, me));
me.free (); // Note: cur_options are still in use.
+ me.cur_options_.store (me.cur_options, memory_order_relaxed);
return re;
}
@@ -1024,6 +1044,7 @@ namespace build2
// Note: for now no adhoc_reapply().
//
ru.reapply (a, t, me);
+ me.cur_options_.store (me.cur_options, memory_order_relaxed);
}
// If anything goes wrong, set target state to failed and return nullopt.
diff --git a/libbuild2/algorithm.ixx b/libbuild2/algorithm.ixx
index bae9151..e384433 100644
--- a/libbuild2/algorithm.ixx
+++ b/libbuild2/algorithm.ixx
@@ -622,6 +622,7 @@ namespace build2
me.reinit (false /* fallback */);
me.cur_options = options; // Already applied, so cur_, not new_options.
+ me.cur_options_.store (me.cur_options, memory_order_relaxed);
clear_target (l.action, *l.target);
set_rule (l, nullptr); // No rule.
set_recipe (l, move (r));
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 83e6994..c61f241 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -221,14 +221,16 @@ namespace build2
// rematch, if current options already include new options, then no call
// to reapply() is made. This, in particular, means that a rule that does
// not adjust cur_options in match() will never get a reapply() call
- // (because all the options are enabled from the start). If a rematch is
- // triggered after the rule has already been executed, an error is issued.
- // This means that match options are not usable for operation/target types
- // that could plausibly be executed during match. In particular, using
- // match options for update and clean operations is a bad idea (update of
- // pretty much any target can happen during match as a result of a tool
- // update while clean might have to be performed during match to provide
- // the mirror semantics).
+ // (because all the options are enabled from the start). Note that
+ // cur_options should only be modfied in apply() or reapply().
+ //
+ // If a rematch is triggered after the rule has already been executed, an
+ // error is issued. This means that match options are not usable for
+ // operation/target types that could plausibly be executed during
+ // match. In particular, using match options for update and clean
+ // operations is a bad idea (update of pretty much any target can happen
+ // during match as a result of a tool update while clean might have to be
+ // performed during match to provide the mirror semantics).
//
// Note also that with rematches the assumption that in the match phase
// after matching the target we can MT-safely examine its state (such as
@@ -264,10 +266,12 @@ namespace build2
// Note: match options are currently not exposed in Buildscript ad hoc
// recipes/rules (but are in C++).
//
+ static constexpr uint64_t all_options = ~uint64_t (0);
+
uint64_t cur_options;
uint64_t new_options;
- static constexpr uint64_t all_options = ~uint64_t (0);
+ atomic<uint64_t> cur_options_; // Implementation detail (see lock_impl()).
// The list of post hoc prerequisite targets for this target. Only not
// NULL in rule::apply_posthoc() and rule::reapply() functions and only if