Make build script variable pool local to execution environment

5 files changed, 53 insertions, 65 deletions

diff --git a/libbuild2/build/script/parser.cxx b/libbuild2/build/script/parser.cxx
index 4bdf5c7..d26b155 100644
--- a/libbuild2/build/script/parser.cxx
+++ b/libbuild2/build/script/parser.cxx
@@ -95,14 +95,12 @@ namespace build2
             // Check if we are trying to modify any of the special variables
             // ($>).
             //
-            string& n (t.value);
+            if (t.value == ">")
+              fail (t) << "attempt to set '" << t.value << "' variable";
 
-            if (n == ">")
-              fail (t) << "attempt to set '" << n << "' variable";
-
-            // Pre-enter the variable.
+            // We don't pre-enter variables.
             //
-            ln.var = &script_->var_pool.insert (move (n));
+            ln.var = nullptr;
 
             next (t, tt); // Assignment kind.
 
@@ -317,7 +315,8 @@ namespace build2
 
         exec_lines (s.lines.begin (), s.lines.end (),
                     exec_set, exec_cmd, exec_if,
-                    li);
+                    li,
+                    &environment_->var_pool);
 
         runner_->leave (*environment_, s.end_loc);
       }
diff --git a/libbuild2/build/script/script.cxx b/libbuild2/build/script/script.cxx
index 26fe006..eb5b78c 100644
--- a/libbuild2/build/script/script.cxx
+++ b/libbuild2/build/script/script.cxx
@@ -15,14 +15,6 @@ namespace build2
   {
     namespace script
     {
-      // script
-      //
-      script::
-      script ()
-          : primary_target_var (var_pool.insert<path> (">"))
-      {
-      }
-
       // environment
       //
       static const string wd_name ("current directory");
@@ -35,13 +27,13 @@ namespace build2
               work,
               wd_name),
             script (s),
-            vars (context, false /* global */),
-            primary_target (pt)
+            primary_target (pt),
+            vars (context, false /* global */)
       {
         // Set the $> variable.
         //
         {
-          value& v (assign (s.primary_target_var));
+          value& v (assign (var_pool.insert<path> (">")));
 
           if (auto* t = pt.is_a<path_target> ())
             v = t->path ();
@@ -53,19 +45,9 @@ namespace build2
       void environment::
       set_variable (string&& nm, names&& val, const string& attrs)
       {
-        // Set the variable value and attributes. Note that we need to aquire
-        // unique lock before potentially changing the script's variable
-        // pool. The obtained variable reference can safelly be used with no
-        // locking as the variable pool is an associative container
-        // (underneath) and we are only adding new variables into it.
+        // Set the variable value and attributes.
         //
-        ulock ul (script.var_pool_mutex);
-
-        const variable& var (
-          const_cast<build::script::script&> (script).var_pool.
-          insert (move (nm)));
-
-        ul.unlock ();
+        const variable& var (var_pool.insert (move (nm)));
 
         value& lhs (assign (var));
 
@@ -100,23 +82,12 @@ namespace build2
       lookup environment::
       lookup (const string& name) const
       {
-        // Every variable that is ever set in a script has been pre-entered
-        // during pre-parse or introduced with the set builtin during
-        // execution. Which means that if one is not found in the script pool
-        // then it can only possibly be set in the buildfile.
-        //
-        // Note that we need to acquire the variable pool lock. The pool can
-        // be changed from multiple threads by the set builtin. The obtained
-        // variable pointer can safelly be used with no locking as the
-        // variable pool is an associative container (underneath) and we are
-        // only adding new variables into it.
+        // Every variable that is ever set in a script has been added during
+        // variable line execution or introduced with the set builtin. Which
+        // means that if one is not found in the environment pool then it can
+        // only possibly be set in the buildfile.
         //
-        const variable* pvar (nullptr);
-
-        slock sl (script.var_pool_mutex);
-        pvar = script.var_pool.find (name);
-        sl.unlock ();
-
+        const variable* pvar (var_pool.find (name));
         return pvar != nullptr ? lookup (*pvar) : lookup_in_buildfile (name);
       }
 
diff --git a/libbuild2/build/script/script.hxx b/libbuild2/build/script/script.hxx
index 18ed4c4..6beb5a4 100644
--- a/libbuild2/build/script/script.hxx
+++ b/libbuild2/build/script/script.hxx
@@ -23,27 +23,18 @@ namespace build2
       using build2::script::command_expr;
 
       // Once parsed, the script can be executed in multiple threads with the
-      // state (variable values, etc) maintained by the environment object.
+      // state (variable values, etc) maintained in the environment object.
       //
       class script
       {
       public:
+        // Note that the variables are not pre-entered into a pool during the
+        // parsing phase, so the line variable pointers are NULL.
+        //
         build2::script::lines lines;
 
-        variable_pool var_pool;
-        mutable shared_mutex var_pool_mutex;
-
-        const variable& primary_target_var; // $>
-
         location start_loc;
         location end_loc;
-
-        script ();
-
-        script (script&&) = delete;
-        script (const script&) = delete;
-        script& operator= (script&&) = delete;
-        script& operator= (const script&) = delete;
       };
 
       class environment: public build2::script::environment
@@ -58,14 +49,17 @@ namespace build2
 
       public:
         const build::script::script& script;
+        const target& primary_target;
+
+        // Script-local variable pool.
+        //
+        variable_pool var_pool;
 
         // Note that if we pass the variable name as a string, then it will
         // be looked up in the wrong pool.
         //
         variable_map vars;
 
-        const target& primary_target;
-
         virtual void
         set_variable (string&& name, names&&, const string& attrs) override;
 
diff --git a/libbuild2/script/parser.cxx b/libbuild2/script/parser.cxx
index 5c7510b..160374d 100644
--- a/libbuild2/script/parser.cxx
+++ b/libbuild2/script/parser.cxx
@@ -3,6 +3,7 @@
 
 #include <libbuild2/script/parser.hxx>
 
+#include <libbuild2/variable.hxx>
 #include <libbuild2/script/run.hxx>   // exit
 #include <libbuild2/script/lexer.hxx>
 
@@ -1716,7 +1717,8 @@ namespace build2
                 const function<exec_set_function>& exec_set,
                 const function<exec_cmd_function>& exec_cmd,
                 const function<exec_if_function>& exec_if,
-                size_t& li)
+                size_t& li,
+                variable_pool* var_pool)
     {
       try
       {
@@ -1743,7 +1745,20 @@ namespace build2
           {
           case line_type::var:
             {
-              exec_set (*ln.var, t, tt, ll);
+              // Enter the variable into the pool if this is not done during
+              // the script parsing. Note that in this case the pool is
+              // expected to be provided.
+              //
+              const variable* var (ln.var);
+
+              if (var == nullptr)
+              {
+                assert (var_pool != nullptr);
+
+                var = &var_pool->insert (t.value);
+              }
+
+              exec_set (*var, t, tt, ll);
 
               replay_stop ();
               break;
@@ -1861,7 +1876,10 @@ namespace build2
                 // Next if-else.
                 //
                 lines::const_iterator j (next (i, false, false));
-                if (!exec_lines (i + 1, j, exec_set, exec_cmd, exec_if, li))
+                if (!exec_lines (i + 1, j,
+                                 exec_set, exec_cmd, exec_if,
+                                 li,
+                                 var_pool))
                   return false;
 
                 i = j->type == line_type::cmd_end ? j : next (j, true, true);
diff --git a/libbuild2/script/parser.hxx b/libbuild2/script/parser.hxx
index 850794e..ecd9f5a 100644
--- a/libbuild2/script/parser.hxx
+++ b/libbuild2/script/parser.hxx
@@ -146,12 +146,18 @@ namespace build2
                                       size_t li,
                                       const location&);
 
+      // If a parser implementation doesn't pre-enter variables into a pool
+      // during the pre-parsing phase, then they are entered during the
+      // execution phase and so the variable pool must be provided. Note that
+      // in this case the variable pool insertions are not MT-safe.
+      //
       bool
       exec_lines (lines::const_iterator b, lines::const_iterator e,
                   const function<exec_set_function>&,
                   const function<exec_cmd_function>&,
                   const function<exec_if_function>&,
-                  size_t& li);
+                  size_t& li,
+                  variable_pool* = nullptr);
 
       // Set lexer pointers for both the current and the base classes.
       //