From eacf7f7ccd40a56d1fe761d3d30ced6c6acd58da Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Thu, 1 Nov 2018 13:00:16 +0200 Subject: Add support for rule-specific variables, use to fix cc.type data race --- build2/action.hxx | 6 +-- build2/algorithm.cxx | 10 +++-- build2/cc/common.cxx | 5 ++- build2/cc/init.cxx | 18 +++++---- build2/cc/link-rule.cxx | 4 +- build2/dump.cxx | 88 +++++++++++++++++++++++++++++------------ build2/dump.hxx | 8 +++- build2/operation.cxx | 2 +- build2/scope.cxx | 22 +++++++---- build2/scope.hxx | 3 +- build2/target.cxx | 27 +++++++++++++ build2/target.hxx | 102 ++++++++++++++++++++++++++++++++++++++++++------ build2/variable.hxx | 3 ++ build2/variable.ixx | 1 - 14 files changed, 231 insertions(+), 68 deletions(-) (limited to 'build2') diff --git a/build2/action.hxx b/build2/action.hxx index 8c9b7e5..321d2c1 100644 --- a/build2/action.hxx +++ b/build2/action.hxx @@ -114,10 +114,10 @@ namespace build2 template struct action_state { - T states[2]; // [0] -- inner, [1] -- outer. + T data[2]; // [0] -- inner, [1] -- outer. - T& operator[] (action a) {return states[a.inner () ? 0 : 1];} - const T& operator[] (action a) const {return states[a.inner () ? 0 : 1];} + T& operator[] (action a) {return data[a.inner () ? 0 : 1];} + const T& operator[] (action a) const {return data[a.inner () ? 0 : 1];} }; // Id constants for build-in and pre-defined meta/operations. diff --git a/build2/algorithm.cxx b/build2/algorithm.cxx index fbeb365..b4f2843 100644 --- a/build2/algorithm.cxx +++ b/build2/algorithm.cxx @@ -434,11 +434,12 @@ namespace build2 // Match. // - // Clear the resolved targets list and the data pad before calling - // match(). The rule is free to modify these in its match() - // (provided that it matches) in order to, for example, convey some - // information to apply(). + // Clear the rule-specific variables, resolved targets list, and the + // data pad before calling match(). The rule is free to modify these + // in its match() (provided that it matches) in order to, for + // example, convey some information to apply(). // + s.vars.clear (); t.prerequisite_targets[a].clear (); if (a.inner ()) t.clear_data (); @@ -479,6 +480,7 @@ namespace build2 // As a sanity measure clear the target data since it can be incomplete // or invalid (mark()/unmark() should give you some ideas). // + s.vars.clear (); t.prerequisite_targets[a].clear (); if (a.inner ()) t.clear_data (); diff --git a/build2/cc/common.cxx b/build2/cc/common.cxx index ae125a5..a1a17f7 100644 --- a/build2/cc/common.cxx +++ b/build2/cc/common.cxx @@ -80,9 +80,10 @@ namespace build2 // See what type of library this is (C, C++, etc). Use it do decide // which x.libs variable name to use. If it's unknown, then we only - // look into prerequisites. + // look into prerequisites. Note: lookup starting from rule-specific + // variables (target should already be matched). // - const string* t (cast_null (l.vars[c_type])); + const string* t (cast_null (l.state[a][c_type])); bool impl (proc_impl && proc_impl (l, la)); bool cc (false), same (false); diff --git a/build2/cc/init.cxx b/build2/cc/init.cxx index 2f88542..bb0269c 100644 --- a/build2/cc/init.cxx +++ b/build2/cc/init.cxx @@ -78,6 +78,8 @@ namespace build2 // auto& v (var_pool.rw (rs)); + auto v_t (variable_visibility::target); + v.insert ("config.cc.poptions", true); v.insert ("config.cc.coptions", true); v.insert ("config.cc.loptions", true); @@ -106,24 +108,26 @@ namespace build2 v.insert ("cc.stdlib"); // Target type, for example, "C library" or "C++ library". Should be set - // on the target by the matching rule to the name of the module (e.g., - // "c", "cxx"). Currenly only set for libraries and is used to decide - // which *.libs to use during static linking. + // on the target as a rule-specific variable by the matching rule to the + // name of the module (e.g., "c", "cxx"). Currenly only set for + // libraries and is used to decide which *.libs to use during static + // linking. // // It can also be the special "cc" value which means a C-common library - // but specific language is not known. Used in import installed logic. + // but specific language is not known. Used in the import installed + // logic. // - v.insert ("cc.type"); + v.insert ("cc.type", v_t); // If set and is true, then this (imported) library has been found in a // system library search directory. // - v.insert ("cc.system"); + v.insert ("cc.system", v_t); // C++ module name. Should be set on the bmi*{} target by the matching // rule. // - v.insert ("cc.module_name"); + v.insert ("cc.module_name", v_t); // Ability to disable using preprocessed output for compilation. // diff --git a/build2/cc/link-rule.cxx b/build2/cc/link-rule.cxx index 230fc6f..ba6a22d 100644 --- a/build2/cc/link-rule.cxx +++ b/build2/cc/link-rule.cxx @@ -356,10 +356,10 @@ namespace build2 otype ot (lt.type); linfo li (link_info (bs, ot)); - // Set the library type (C, C++, etc). + // Set the library type (C, C++, etc) as rule-specific variable. // if (lt.library ()) - t.vars.assign (c_type) = string (x); + t.state[a].assign (c_type) = string (x); bool binless (lt.library ()); // Binary-less until proven otherwise. diff --git a/build2/dump.cxx b/build2/dump.cxx index 0c7e839..fe120d0 100644 --- a/build2/dump.cxx +++ b/build2/dump.cxx @@ -53,7 +53,7 @@ namespace build2 } } - enum class variable_kind {scope, tt_pat, target, prerequisite}; + enum class variable_kind {scope, tt_pat, target, rule, prerequisite}; static void dump_variable (ostream& os, @@ -107,7 +107,10 @@ namespace build2 // lookup l ( s.find_override ( - var, make_pair (org, 1), k == variable_kind::target).first); + var, + make_pair (org, 1), + k == variable_kind::target || k == variable_kind::rule, + k == variable_kind::rule).first); assert (l.defined ()); // We at least have the original. @@ -190,7 +193,8 @@ namespace build2 } static void - dump_target (ostream& os, + dump_target (optional a, + ostream& os, string& ind, const target& t, const scope& s, @@ -213,26 +217,54 @@ namespace build2 os << ind << t << ':'; - // First print target-specific variables, if any. + // First print target/rule-specific variables, if any. // - if (!t.vars.empty ()) { - if (rel) - stream_verb (os, osv); // We want variable values in full. + bool tv (!t.vars.empty ()); + bool rv (a && !t.state[*a].vars.empty ()); - os << endl - << ind << '{'; - ind += " "; - dump_variables (os, ind, t.vars, s, variable_kind::target); - ind.resize (ind.size () - 2); - os << endl - << ind << '}'; + if (tv || rv) + { + if (rel) + stream_verb (os, osv); // We want variable values in full. + + os << endl + << ind << '{'; + ind += " "; - if (rel) - stream_verb (os, nsv); + if (tv) + dump_variables (os, ind, t.vars, s, variable_kind::target); - os << endl - << ind << t << ':'; + if (rv) + { + // To distinguish target and rule-specific variables, we put the + // latter into a nested block. + // + // @@ Maybe if we also print the rule name, then we could make + // the block associated with that? + + if (tv) + os << endl; + + os << endl + << ind << '{'; + ind += " "; + dump_variables (os, ind, t.state[*a].vars, s, variable_kind::rule); + ind.resize (ind.size () - 2); + os << endl + << ind << '}'; + } + + ind.resize (ind.size () - 2); + os << endl + << ind << '}'; + + if (rel) + stream_verb (os, nsv); + + os << endl + << ind << t << ':'; + } } bool used (false); // Target header has been used to display prerequisites. @@ -318,7 +350,8 @@ namespace build2 } static void - dump_scope (ostream& os, + dump_scope (optional a, + ostream& os, string& ind, scope_map::const_iterator& i, bool rel) @@ -383,7 +416,7 @@ namespace build2 os << endl; // Extra newline between scope blocks. os << endl; - dump_scope (os, ind, i, true /* relative */); + dump_scope (a, os, ind, i, true /* relative */); sb = true; } @@ -406,7 +439,7 @@ namespace build2 } os << endl; - dump_target (os, ind, t, p, true /* relative */); + dump_target (a, os, ind, t, p, true /* relative */); tb = true; } @@ -418,7 +451,7 @@ namespace build2 } void - dump () + dump (optional a) { auto i (scopes.cbegin ()); assert (&i->second == global_scope); @@ -428,7 +461,7 @@ namespace build2 // string ind; ostream& os (*diag_stream); - dump_scope (os, ind, i, false /* relative */); + dump_scope (a, os, ind, i, false /* relative */); os << endl; } @@ -441,7 +474,7 @@ namespace build2 string ind (cind); ostream& os (*diag_stream); - dump_scope (os, ind, i, false /* relative */); + dump_scope (nullopt /* action */, os, ind, i, false /* relative */); os << endl; } @@ -450,7 +483,12 @@ namespace build2 { string ind (cind); ostream& os (*diag_stream); - dump_target (os, ind, t, t.base_scope (), false /* relative */); + dump_target (nullopt /* action */, + os, + ind, + t, + t.base_scope (), + false /* relative */); os << endl; } } diff --git a/build2/dump.hxx b/build2/dump.hxx index 74ecbe7..e9a3b30 100644 --- a/build2/dump.hxx +++ b/build2/dump.hxx @@ -8,15 +8,19 @@ #include #include +#include + namespace build2 { class scope; class target; - // Dump the build state to diag_stream. + // Dump the build state to diag_stream. If action is specified, then assume + // rules have been matched for this action and dump action-specific + // information (like rule-specific variables). // void - dump (); + dump (optional = nullopt); void dump (const scope&, const char* ind = ""); diff --git a/build2/operation.cxx b/build2/operation.cxx index c42f92a..229ea4e 100644 --- a/build2/operation.cxx +++ b/build2/operation.cxx @@ -238,7 +238,7 @@ namespace build2 assert (phase == run_phase::load); if (verb >= 6) - dump (); + dump (a); } void diff --git a/build2/scope.cxx b/build2/scope.cxx index bf83830..b9dabe8 100644 --- a/build2/scope.cxx +++ b/build2/scope.cxx @@ -178,8 +178,11 @@ namespace build2 pair scope:: find_override (const variable& var, pair original, - bool target) const + bool target, + bool rule) const { + assert (!rule || target); // Rule-specific is target-specific. + // Normally there would be no overrides and if there are, there will only // be a few of them. As a result, here we concentrate on keeping the logic // as straightforward as possible without trying to optimize anything. @@ -204,14 +207,15 @@ namespace build2 const variable_map* inner_vars (nullptr); const scope* inner_proj (nullptr); - // One special case is if the original is target-specific, which is the - // most innermost. Or is it innermostest? + // One special case is if the original is target/rule-specific, which is + // the most innermost. Or is it innermostest? // bool targetspec (false); if (target) { - targetspec = orig.defined () && (orig_depth == 1 || orig_depth == 2); - + targetspec = orig.defined () && (orig_depth == 1 || + orig_depth == 2 || + (rule && orig_depth == 3)); if (targetspec) { inner_vars = orig.vars; @@ -354,7 +358,7 @@ namespace build2 size_t stem_depth (0); const scope* stem_proj (nullptr); - // Again the special case of a target-specific variable. + // Again the special case of a target/rule-specific variable. // if (targetspec) { @@ -363,7 +367,9 @@ namespace build2 stem_proj = root_scope (); } - size_t ovr_depth (target ? 2 : 0); // For implied target-specific lookup. + // For implied target/rule-specific lookup. + // + size_t ovr_depth (target ? (rule ? 3 : 2) : 0); for (s = this; s != nullptr; s = s->parent_scope ()) { @@ -462,7 +468,7 @@ namespace build2 const variable_map* vars (stem.vars); const scope* proj (stem_proj); - ovr_depth = target ? 2 : 0; + ovr_depth = target ? (rule ? 3 : 2) : 0; for (s = this; s != nullptr; s = s->parent_scope ()) { diff --git a/build2/scope.hxx b/build2/scope.hxx index d40d14a..770f320 100644 --- a/build2/scope.hxx +++ b/build2/scope.hxx @@ -135,7 +135,8 @@ namespace build2 pair find_override (const variable&, pair original, - bool target = false) const; + bool target = false, + bool rule = false) const; // Return a value suitable for assignment (or append if you only want to // append to the value from this scope). If the value does not exist in diff --git a/build2/target.cxx b/build2/target.cxx index 5d11337..20b9fd6 100644 --- a/build2/target.cxx +++ b/build2/target.cxx @@ -192,6 +192,31 @@ namespace build2 return r; } + pair target::opstate:: + find_original (const variable& var, bool target_only) const + { + pair r (lookup (), 0); + + ++r.second; + { + auto p (vars.find (var)); + if (p.first != nullptr) + r.first = lookup (*p.first, p.second, vars); + } + + // Delegate to target's find_original(). + // + if (!r.first) + { + auto p (target_->find_original (var, target_only)); + + r.first = move (p.first); + r.second = r.first ? r.second + p.second : p.second; + } + + return r; + } + optional target:: split_name (string& v, const location& loc) { @@ -366,6 +391,8 @@ namespace build2 { t->ext_ = &i->first.ext; t->implied = implied; + t->state.data[0].target_ = t; + t->state.data[1].target_ = t; return pair (*t, move (ul)); } diff --git a/build2/target.hxx b/build2/target.hxx index fb1c68c..80a5d6d 100644 --- a/build2/target.hxx +++ b/build2/target.hxx @@ -357,6 +357,8 @@ namespace build2 // Target-specific variables. // + // See also rule-specific variables below. + // public: variable_map vars; @@ -460,9 +462,10 @@ namespace build2 // Inner/outer operation state. See operation.hxx for details. // - struct opstate + class opstate { - mutable atomic_count task_count {0}; // Start offset_touched - 1. + public: + mutable atomic_count task_count = 0; // Start offset_touched - 1. // Number of direct targets that depend on this target in the current // operation. It is incremented during match and then decremented during @@ -470,7 +473,7 @@ namespace build2 // detect the last chance (i.e., last dependent) to execute the command // (see also the first/last execution modes in ). // - mutable atomic_count dependents {0}; + mutable atomic_count dependents = 0; // Matched rule (pointer to hint_rule_map element). Note that in case of // a direct recipe assignment we may not have a rule (NULL). @@ -485,6 +488,79 @@ namespace build2 // a rule is matched and recipe applied (see set_recipe()). // target_state state; + + // Rule-specific variables. + // + // The rule (for this action) has to be matched before these variables + // can be accessed and only the rule being matched can modify them (so + // no iffy modifications of the group's variables by member's rules). + // + // They are also automatically cleared before another rule is matched, + // similar to the data pad. In other words, rule-specific variables are + // only valid for this match-execute phase. + // + variable_map vars; + + // Lookup, continuing in the target-specific variables, etc. Note that + // the group's rule-specific variables are not included. If you only + // want to lookup in this target, do it on the variable map directly + // (and note that there will be no overrides). + // + lookup + operator[] (const variable& var) const + { + return find (var).first; + } + + lookup + operator[] (const variable* var) const // For cached variables. + { + assert (var != nullptr); + return operator[] (*var); + } + + lookup + operator[] (const string& name) const + { + const variable* var (var_pool.find (name)); + return var != nullptr ? operator[] (*var) : lookup (); + } + + // As above but also return the depth at which the value is found. The + // depth is calculated by adding 1 for each test performed. So a value + // that is from the rule will have depth 1. That from the target - 2, + // and so on, similar to target-specific variables. + // + pair + find (const variable& var) const + { + auto p (find_original (var)); + return var.override == nullptr + ? p + : target_->base_scope ().find_override (var, move (p), true, true); + } + + // If target_only is true, then only look in target and its target group + // without continuing in scopes. + // + pair + find_original (const variable&, bool target_only = false) const; + + // Return a value suitable for assignment. See target for details. + // + value& + assign (const variable& var) {return vars.assign (var);} + + value& + assign (const variable* var) {return vars.assign (var);} // For cached. + + public: + opstate (): vars (false /* global */) {} + + private: + friend class target_set; + + const target* target_ = nullptr; // Back-pointer, set by target_set. }; action_state state; @@ -672,21 +748,23 @@ namespace build2 static void combine_name (string&, const optional&, bool default_extension); + // Targets should be created via the targets set below. + // public: - virtual - ~target (); + target (dir_path d, dir_path o, string n) + : dir (move (d)), out (move (o)), name (move (n)), + vars (false /* global */) {} + + target (target&&) = delete; + target& operator= (target&&) = delete; target (const target&) = delete; target& operator= (const target&) = delete; - // The only way to create a target should be via the targets set below. - // - public: - friend class target_set; + virtual + ~target (); - target (dir_path d, dir_path o, string n) - : dir (move (d)), out (move (o)), name (move (n)), - vars (false /* global */) {} + friend class target_set; }; // All targets are from the targets set below. diff --git a/build2/variable.hxx b/build2/variable.hxx index 7a0e531..7dc3d1a 100644 --- a/build2/variable.hxx +++ b/build2/variable.hxx @@ -1351,6 +1351,9 @@ namespace build2 explicit variable_map (bool global = false): global_ (global) {} + void + clear () {m_.clear ();} + private: friend class variable_type_map; diff --git a/build2/variable.ixx b/build2/variable.ixx index bf5fb92..6a219ce 100644 --- a/build2/variable.ixx +++ b/build2/variable.ixx @@ -280,7 +280,6 @@ namespace build2 typify (v, t, var, memory_order_relaxed); } - inline vector_view reverse (const value& v, names& storage) { -- cgit v1.1