From 63f6a8256e3f9fb47cb941be63baa70e2be48d3b Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 2 Feb 2017 15:35:44 +0200 Subject: Implement target_set locking, including extension update --- build2/algorithm | 4 +- build2/algorithm.cxx | 10 ++- build2/algorithm.ixx | 15 +++- build2/bin/rule.cxx | 4 +- build2/cc/compile.cxx | 2 +- build2/cc/link.cxx | 4 +- build2/cli/rule.cxx | 12 ++-- build2/config/operation.cxx | 2 +- build2/parser.cxx | 5 +- build2/prerequisite.cxx | 8 ++- build2/search.cxx | 10 +-- build2/target | 34 ++++++--- build2/target.cxx | 172 +++++++++++++++++++++++++++++++++----------- build2/target.ixx | 21 ++++++ 14 files changed, 225 insertions(+), 78 deletions(-) diff --git a/build2/algorithm b/build2/algorithm index 742b2e9..cb84231 100644 --- a/build2/algorithm +++ b/build2/algorithm @@ -44,7 +44,7 @@ namespace build2 const dir_path& dir, const dir_path& out, const string& name, - const optional& ext, + const string* ext, // NULL means unspecified. const scope*, const optional& proj = nullopt); @@ -55,7 +55,7 @@ namespace build2 search (const dir_path& dir, const dir_path& out, const string& name, - const optional& ext, + const string* ext, const scope*); // Search for a target identified by the name. The semantics is "as if" we diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index e8e7777..734c545 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -53,7 +53,13 @@ namespace build2 // @@ OUT: for now we assume the prerequisite's out is undetermined. // Would need to pass a pair of names. // - return search (*tt, n.dir, dir_path (), n.value, ext, &s, n.proj); + return search (*tt, + n.dir, + dir_path (), + n.value, + ext ? &*ext : nullptr, + &s, + n.proj); } target* @@ -374,7 +380,7 @@ namespace build2 // Target is in the out tree, so out directory is empty. // - fsdir* r (&search (d, dir_path (), string (), nullopt, nullptr)); + fsdir* r (&search (d, dir_path (), string (), nullptr, nullptr)); match (ml, a, *r); t.prerequisite_targets.emplace_back (r); return r; diff --git a/build2/algorithm.ixx b/build2/algorithm.ixx index 365a5c4..f8e3191 100644 --- a/build2/algorithm.ixx +++ b/build2/algorithm.ixx @@ -32,12 +32,21 @@ namespace build2 const dir_path& dir, const dir_path& out, const string& name, - const optional& ext, + const string* ext, const scope* scope, const optional& proj) { return search ( - prerequisite_key {proj, {&type, &dir, &out, &name, ext}, scope}); + prerequisite_key { + proj, + { + &type, + &dir, + &out, + &name, + ext != nullptr ? optional (*ext) : nullopt + }, + scope}); } template @@ -45,7 +54,7 @@ namespace build2 search (const dir_path& dir, const dir_path& out, const string& name, - const optional& ext, + const string* ext, const scope* scope) { return static_cast ( diff --git a/build2/bin/rule.cxx b/build2/bin/rule.cxx index 648c8b6..c9e8b3e 100644 --- a/build2/bin/rule.cxx +++ b/build2/bin/rule.cxx @@ -75,7 +75,7 @@ namespace build2 if (a) { if (t.a == nullptr) - t.a = &search (t.dir, t.out, t.name, nullopt, nullptr); + t.a = &search (t.dir, t.out, t.name, nullptr, nullptr); match_only (ml, act, *t.a); } @@ -83,7 +83,7 @@ namespace build2 if (s) { if (t.s == nullptr) - t.s = &search (t.dir, t.out, t.name, nullopt, nullptr); + t.s = &search (t.dir, t.out, t.name, nullptr, nullptr); match_only (ml, act, *t.s); } diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx index 590ba7c..66ea9bd 100644 --- a/build2/cc/compile.cxx +++ b/build2/cc/compile.cxx @@ -974,7 +974,7 @@ namespace build2 // target* r; if (insert) - r = &search (*tt, d, out, n, e, nullptr); + r = &search (*tt, d, out, n, &e, nullptr); else { // Note that we skip any target type-specific searches (like for diff --git a/build2/cc/link.cxx b/build2/cc/link.cxx index 7798c95..83a7c0d 100644 --- a/build2/cc/link.cxx +++ b/build2/cc/link.cxx @@ -337,7 +337,7 @@ namespace build2 if (t.member != nullptr) // Might already be there. assert (t.member->type () == tt); else - t.member = &search (tt, t.dir, t.out, t.name, nullopt, nullptr); + t.member = &search (tt, t.dir, t.out, t.name, nullptr, nullptr); file& r (static_cast (*t.member)); r.recipe (a, group_recipe); @@ -519,7 +519,7 @@ namespace build2 // obj*{} is always in the out tree. // target& ot ( - search (tt, d, dir_path (), *cp.tk.name, nullopt, cp.scope)); + search (tt, d, dir_path (), *cp.tk.name, nullptr, cp.scope)); // If we are cleaning, check that this target is in the same or // a subdirectory of our project root. diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx index 262e89a..9f54950 100644 --- a/build2/cli/rule.cxx +++ b/build2/cli/rule.cxx @@ -88,15 +88,15 @@ namespace build2 // if (t.h == nullptr) { - t.h = &search (t.dir, t.out, t.name, nullopt, nullptr); + t.h = &search (t.dir, t.out, t.name, nullptr, nullptr); t.h->group = &t; - t.c = &search (t.dir, t.out, t.name, nullopt, nullptr); + t.c = &search (t.dir, t.out, t.name, nullptr, nullptr); t.c->group = &t; if (!find_option ("--suppress-inline", t, "cli.options")) { - t.i = &search (t.dir, t.out, t.name, nullopt, nullptr); + t.i = &search (t.dir, t.out, t.name, nullptr, nullptr); t.i->group = &t; } } @@ -211,10 +211,8 @@ namespace build2 const char* option, const char* default_extension) { - // Should have been figured out in apply(). - // - const optional& e (*t.ext_); - assert (e); + const string* e (t.ext ()); + assert (e != nullptr); // Should have been figured out in apply(). if (*e != default_extension) { diff --git a/build2/config/operation.cxx b/build2/config/operation.cxx index 75f5abc..d534dab 100644 --- a/build2/config/operation.cxx +++ b/build2/config/operation.cxx @@ -613,7 +613,7 @@ namespace build2 dir_path (), // Out tree. "", nullopt, - false, // Real (not implied). + true, // Implied. trace).first); if (!quiet) diff --git a/build2/parser.cxx b/build2/parser.cxx index e3f0c70..2ffd0cf 100644 --- a/build2/parser.cxx +++ b/build2/parser.cxx @@ -3556,13 +3556,16 @@ namespace build2 l5 ([&]{trace (t) << "creating current directory alias for " << dt;}); + // While this target is not explicitly mentioned in the buildfile, we say + // that we behave as if it were. Thus not implied. + // target& ct ( targets.insert (dir::static_type, scope_->out_path (), dir_path (), string (), nullopt, - false, // Enter as real (not implied). + false, trace).first); ct.prerequisites.emplace_back (prerequisite (dt)); diff --git a/build2/prerequisite.cxx b/build2/prerequisite.cxx index ac89456..47da950 100644 --- a/build2/prerequisite.cxx +++ b/build2/prerequisite.cxx @@ -50,6 +50,12 @@ namespace build2 // prerequisite // + static inline optional + to_ext (const string* e) + { + return e != nullptr ? optional (*e) : nullopt; + } + prerequisite:: prerequisite (target_type& t) : proj (nullopt), @@ -57,7 +63,7 @@ namespace build2 dir (t.dir), out (t.out), // @@ If it's empty, then we treat as undetermined? name (t.name), - ext (t.ext ()), + ext (to_ext (t.ext ())), scope (t.base_scope ()), target (&t) { diff --git a/build2/search.cxx b/build2/search.cxx index 8f621c2..7c658e0 100644 --- a/build2/search.cxx +++ b/build2/search.cxx @@ -171,8 +171,9 @@ namespace build2 // Find or insert. Note that we are using our updated extension. // - auto r (targets.insert ( - *tk.type, move (d), move (out), *tk.name, ext, false, trace)); + auto r ( + targets.insert ( + *tk.type, move (d), move (out), *tk.name, ext, true, trace)); // Has to be a file_target. // @@ -215,8 +216,9 @@ namespace build2 // // @@ OUT: same story as in search_existing_target() re out. // - auto r (targets.insert ( - *tk.type, move (d), *tk.out, *tk.name, tk.ext, false, trace)); + auto r ( + targets.insert ( + *tk.type, move (d), *tk.out, *tk.name, tk.ext, true, trace)); assert (r.second); target& t (r.first); diff --git a/build2/target b/build2/target index fc6e782..1a300f7 100644 --- a/build2/target +++ b/build2/target @@ -107,6 +107,8 @@ namespace build2 // class target { + optional* ext_; // Reference to value in target_key. + public: using action_type = build2::action; @@ -127,9 +129,9 @@ namespace build2 const dir_path dir; // Absolute and normalized. const dir_path out; // Empty or absolute and normalized. const string name; - optional ext () const {return *ext_;} - optional* ext_; // Reference to value in target_key. + const string* ext () const; // Return NULL if not specified. + const string& ext (string); const dir_path& out_dir () const {return out.empty () ? dir : out;} @@ -247,11 +249,11 @@ namespace build2 virtual group_view group_members (action_type) const; - // Note that the returned key "tracks" the target; that is, any updates - // to the targets's members will be reflected in the key. + // Note that the returned key "tracks" the target (except for the + // extension). // target_key - key () const {return target_key {&type (), &dir, &out, &name, ext ()};} + key () const; // Scoping. // @@ -382,7 +384,10 @@ namespace build2 // A target that is not (yet) entered as part of a real dependency // declaration (for example, that is entered as part of a target-specific - // variable assignment) is called implied. + // variable assignment, dependency extraction, etc) is called implied. + // + // The implied flag should only be cleared during the load phase via the + // target_set::insert(). // public: bool implied; @@ -1000,8 +1005,10 @@ namespace build2 T* find (const dir_path& dir, const dir_path& out, const string& name) const { - auto i (map_.find ( - target_key {&T::static_type, &dir, &out, &name, nullopt})); + slock l (mutex_); + auto i ( + map_.find ( + target_key {&T::static_type, &dir, &out, &name, nullopt})); return i != map_.end () ? static_cast (i->second.get ()) : nullptr; } @@ -1014,6 +1021,8 @@ namespace build2 bool implied, tracer&); + // Note that the following versions always enter implied targets. + // template T& insert (const target_type& tt, @@ -1029,7 +1038,7 @@ namespace build2 move (out), move (name), move (ext), - false, // Always real (not implied). + true, t).first); } @@ -1042,7 +1051,7 @@ namespace build2 tracer& t) { return static_cast ( - insert (T::static_type, dir, out, name, ext, false, t).first); + insert (T::static_type, dir, out, name, ext, true, t).first); } template @@ -1053,7 +1062,7 @@ namespace build2 tracer& t) { return static_cast ( - insert (T::static_type, dir, out, name, nullopt, false, t).first); + insert (T::static_type, dir, out, name, nullopt, true, t).first); } // Note: not MT-safe so can only be used during serial execution. @@ -1068,6 +1077,9 @@ namespace build2 clear () {map_.clear ();} private: + friend class target; // Access to mutex. + + mutable shared_mutex mutex_; map_type map_; }; diff --git a/build2/target.cxx b/build2/target.cxx index 10b1432..b250597 100644 --- a/build2/target.cxx +++ b/build2/target.cxx @@ -56,6 +56,31 @@ namespace build2 clear_data (); } + const string& target:: + ext (string v) + { + ulock l (targets.mutex_); + + // Once the extension is set, it is immutable. However, it is possible + // that someone has already "branded" this target with a different + // extension. + // + optional& e (*ext_); + + if (!e) + e = move (v); + else if (*e != v) + { + string o (*e); + l.unlock (); + + fail << "conflicting extensions '" << o << "' and '" << v << "' " + << "for target " << *this; + } + + return *e; + } + void target:: recipe (action_type a, recipe_type r) { @@ -201,34 +226,54 @@ namespace build2 target* target_set:: find (const target_key& k, tracer& trace) const { - target* t (nullptr); + slock sl (mutex_); map_type::const_iterator i (map_.find (k)); - if (i != map_.end ()) + if (i == map_.end ()) + return nullptr; + + target& t (*i->second); + optional& ext (i->first.ext); + + if (ext != k.ext) { - t = i->second.get (); - optional& ext (i->first.ext); + ulock ul; // Keep locked for trace. - if (ext != k.ext) + if (k.ext) { - l5 ([&]{ - diag_record r (trace); - r << "assuming target " << *t << " is the same as the one with "; - if (!k.ext) - r << "unspecified extension"; - else if (k.ext->empty ()) - r << "no extension"; - else - r << "extension " << *k.ext; - }); + // To update the extension we have to re-lock for exclusive access. + // Between us releasing the shared lock and acquiring unique the + // extension could change and possibly a new target that matches the + // key could be inserted. In this case we simply re-run find (). + // + sl.unlock (); + ul = ulock (mutex_); + if (ext) // Someone set the extension. + { + ul.unlock (); + return find (k, trace); + } - if (k.ext) - ext = k.ext; + ext = k.ext; } + + l5 ([&]{ + diag_record r (trace); + r << "assuming target "; + to_stream (r.os, t.key (), 0); // Don't print the extension. + r << " is the same as the one with "; + + if (!k.ext) + r << "unspecified extension"; + else if (k.ext->empty ()) + r << "no extension"; + else + r << "extension " << *k.ext; + }); } - return t; + return &t; } pair target_set:: @@ -241,35 +286,78 @@ namespace build2 tracer& trace) { target_key tk {&tt, &dir, &out, &name, move (ext)}; - target* t (find (tk, trace)); - bool r (t == nullptr); - if (r) + if (t == nullptr) { - pair> p ( + pair> te ( tt.factory ( tt, move (dir), move (out), move (name), move (tk.ext))); - t = p.first; + t = te.first; + + // Re-lock for exclusive access. In the meantime, someone could have + // inserted this target so emplace() below could return false, in which + // case we proceed pretty much like find() except already under the + // exclusive lock. + // + ulock ul (mutex_); - map_type::iterator i ( + auto p ( map_.emplace ( - target_key {&tt, &t->dir, &t->out, &t->name, move (p.second)}, - p.first).first); + target_key {&tt, &t->dir, &t->out, &t->name, te.second}, + unique_ptr (te.first))); - t->ext_ = &i->first.ext; - t->implied = implied; + map_type::iterator i (p.first); + + if (p.second) + { + t->ext_ = &i->first.ext; + t->implied = implied; + return pair (*t, true); + } + + // The "tail" of find(). + // + t = i->second.get (); + optional& ext (i->first.ext); + + if (ext != te.second) + { + if (te.second) + ext = te.second; + + l5 ([&]{ + diag_record r (trace); + r << "assuming target "; + to_stream (r.os, t->key (), 0); // Don't print the extension. + r << " is the same as the one with "; + + if (!te.second) + r << "unspecified extension"; + else if (te.second->empty ()) + r << "no extension"; + else + r << "extension " << *te.second; + }); + } + + // Fall through (continue as if the first find() returned this target). } - else if (!implied) + + if (!implied) { + // The implied flag can only be cleared during the load phase. + // + assert (phase == run_phase::load); + // Clear the implied flag. // if (t->implied) t->implied = false; } - return pair (*t, r); + return pair (*t, false); } ostream& @@ -358,31 +446,33 @@ namespace build2 // assert (de == nullptr || type ().extension != nullptr); - optional& ext (*ext_); //@@ MT lock - - if (!ext) + if (const string* p = ext ()) + // Note that returning by reference is now MT-safe since once the + // extension is specified, it is immutable. + // + return *p; + else { + optional e; + // If the target type has the extension function then try that first. // The reason for preferring it over what's been provided by the caller // is that this function will often use the 'extension' variable which // the user can use to override extensions. // if (auto f = type ().extension) - ext = f (key (), base_scope (), search); + e = f (key (), base_scope (), search); - if (!ext) + if (!e) { if (de != nullptr) - ext = de; + e = de; else fail << "no default extension for target " << *this; } - } - // Note that returning by reference is now MT-safe since once the - // extension is specified, it is immutable. - // - return *ext; + return ext (move (*e)); + } } const path& path_target:: diff --git a/build2/target.ixx b/build2/target.ixx index f3417aa..19e0dfa 100644 --- a/build2/target.ixx +++ b/build2/target.ixx @@ -4,6 +4,27 @@ namespace build2 { + // target + // + inline const string* target:: + ext () const + { + slock l (targets.mutex_); + return *ext_ ? &**ext_ : nullptr; + } + + inline target_key target:: + key () const + { + const string* e (ext ()); + return target_key { + &type (), + &dir, + &out, + &name, + e != nullptr ? optional (*e) : nullopt}; + } + // prerequisite_member // inline prerequisite prerequisite_member:: -- cgit v1.1