Implement target_set locking, including extension update

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<string>& ext,
+          const string* ext,  // NULL means unspecified.
           const scope*,
           const optional<string>& proj = nullopt);
 
@@ -55,7 +55,7 @@ namespace build2
   search (const dir_path& dir,
           const dir_path& out,
           const string& name,
-          const optional<string>& 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<fsdir> (d, dir_path (), string (), nullopt, nullptr));
+    fsdir* r (&search<fsdir> (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<string>& ext,
+          const string* ext,
           const scope* scope,
           const optional<string>& proj)
   {
     return search (
-      prerequisite_key {proj, {&type, &dir, &out, &name, ext}, scope});
+      prerequisite_key {
+        proj,
+        {
+          &type,
+          &dir,
+          &out,
+          &name,
+          ext != nullptr ? optional<string> (*ext) : nullopt
+        },
+        scope});
   }
 
   template <typename T>
@@ -45,7 +54,7 @@ namespace build2
   search (const dir_path& dir,
           const dir_path& out,
           const string& name,
-          const optional<string>& ext,
+          const string* ext,
           const scope* scope)
   {
     return static_cast<T&> (
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<liba> (t.dir, t.out, t.name, nullopt, nullptr);
+          t.a = &search<liba> (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<libs> (t.dir, t.out, t.name, nullopt, nullptr);
+          t.s = &search<libs> (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<file&> (*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<cxx::hxx> (t.dir, t.out, t.name, nullopt, nullptr);
+          t.h = &search<cxx::hxx> (t.dir, t.out, t.name, nullptr, nullptr);
           t.h->group = &t;
 
-          t.c = &search<cxx::cxx> (t.dir, t.out, t.name, nullopt, nullptr);
+          t.c = &search<cxx::cxx> (t.dir, t.out, t.name, nullptr, nullptr);
           t.c->group = &t;
 
           if (!find_option ("--suppress-inline", t, "cli.options"))
           {
-            t.i = &search<cxx::ixx> (t.dir, t.out, t.name, nullopt, nullptr);
+            t.i = &search<cxx::ixx> (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<string>& 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<string>
+  to_ext (const string* e)
+  {
+    return e != nullptr ? optional<string> (*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<string>* 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<string> ext () const {return *ext_;}
 
-    optional<string>* 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<T*> (i->second.get ()) : nullptr;
     }
 
@@ -1014,6 +1021,8 @@ namespace build2
             bool implied,
             tracer&);
 
+    // Note that the following versions always enter implied targets.
+    //
     template <typename T>
     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<T&> (
-        insert (T::static_type, dir, out, name, ext, false, t).first);
+        insert (T::static_type, dir, out, name, ext, true, t).first);
     }
 
     template <typename T>
@@ -1053,7 +1062,7 @@ namespace build2
             tracer& t)
     {
       return static_cast<T&> (
-        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<string>& 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<string>& ext (i->first.ext);
+
+    if (ext != k.ext)
     {
-      t = i->second.get ();
-      optional<string>& 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&, bool> 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<target*, optional<string>> p (
+      pair<target*, optional<string>> 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<target> (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<target&, bool> (*t, true);
+      }
+
+      // The "tail" of find().
+      //
+      t = i->second.get ();
+      optional<string>& 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<target&, bool> (*t, r);
+    return pair<target&, bool> (*t, false);
   }
 
   ostream&
@@ -358,31 +446,33 @@ namespace build2
     //
     assert (de == nullptr || type ().extension != nullptr);
 
-    optional<string>& 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<string> 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<string> (*e) : nullopt};
+  }
+
   // prerequisite_member
   //
   inline prerequisite prerequisite_member::