Add support for rule-specific variables, use to fix cc.type data race

14 files changed, 231 insertions, 68 deletions

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 <typename T>
   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<string> (l.vars[c_type]));
+      const string* t (cast_null<string> (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<strings> ("config.cc.poptions", true);
       v.insert<strings> ("config.cc.coptions", true);
       v.insert<strings> ("config.cc.loptions", true);
@@ -106,24 +108,26 @@ namespace build2
       v.insert<string> ("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<string> ("cc.type");
+      v.insert<string> ("cc.type", v_t);
 
       // If set and is true, then this (imported) library has been found in a
       // system library search directory.
       //
-      v.insert<bool> ("cc.system");
+      v.insert<bool> ("cc.system", v_t);
 
       // C++ module name. Should be set on the bmi*{} target by the matching
       // rule.
       //
-      v.insert<string> ("cc.module_name");
+      v.insert<string> ("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<action> 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<action> 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<action> 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 <build2/types.hxx>
 #include <build2/utility.hxx>
 
+#include <build2/action.hxx>
+
 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<action> = 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<lookup, size_t> scope::
   find_override (const variable& var,
                  pair<lookup, size_t> 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<lookup, size_t>
     find_override (const variable&,
                    pair<lookup, size_t> 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<lookup, size_t> target::opstate::
+  find_original (const variable& var, bool target_only) const
+  {
+    pair<lookup, size_t> 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<string> 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<target&, ulock> (*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 <operation.hxx>).
       //
-      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<lookup, size_t>
+      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<lookup, size_t>
+      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<opstate> state;
@@ -672,21 +748,23 @@ namespace build2
     static void
     combine_name (string&, const optional<string>&, 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<const name>
   reverse (const value& v, names& storage)
   {