From df69d473d3ab389e915698b8c2c4bb8d22975976 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Tue, 22 Oct 2019 14:32:49 +0200
Subject: Implement loaded_modules state locking

This would be necessary if someone runs two parallel top-level contexts.
---
 libbuild2/context.cxx |  4 +++-
 libbuild2/context.hxx | 14 +++++++++++++-
 libbuild2/module.cxx  | 13 +++++++++++++
 libbuild2/module.hxx  | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

(limited to 'libbuild2')

diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx
index 841cc1d..292feed 100644
--- a/libbuild2/context.cxx
+++ b/libbuild2/context.cxx
@@ -60,7 +60,8 @@ namespace build2
            bool dr,
            bool kg,
            const strings& cmd_vars,
-           optional<context*> mc)
+           optional<context*> mc,
+           const loaded_modules_lock* ml)
       : data_ (new data (*this)),
         sched (s),
         mutexes (ms),
@@ -76,6 +77,7 @@ namespace build2
         global_target_types (data_->global_target_types),
         global_override_cache (data_->global_override_cache),
         global_var_overrides (data_->global_var_overrides),
+        modules_lock (ml),
         module_context (mc ? *mc : nullptr),
         module_context_storage (mc
                                 ? optional<unique_ptr<context>> (nullptr)
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 60f5ba8..0a2deea 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -40,6 +40,8 @@ namespace build2
 
   struct opspec;
 
+  struct loaded_modules_lock;
+
   class LIBBUILD2_SYMEXPORT run_phase_mutex
   {
   public:
@@ -129,6 +131,11 @@ namespace build2
   // future we can do save/restore but then we would need some indication that
   // we have switched to another task).
   //
+  // The loaded_modules state (module.hxx) is shared among all the contexts
+  // (there is no way to have multiple shared library loading "contexts") and
+  // is protected by loaded_modules_lock. A nested context should normally
+  // inherit this lock value from its outer context.
+  //
   // Note also that any given thread should not participate in multiple
   // schedulers at the same time (see scheduler::join/leave() for details).
   //
@@ -414,6 +421,10 @@ namespace build2
     dir_path old_src_root;
     dir_path new_src_root;
 
+    // NULL if this context hasn't already locked the loaded_modules state.
+    //
+    const loaded_modules_lock* modules_lock;
+
     // Nested context for updating build system modules.
     //
     // Note that such a context itself should normally have modules_context
@@ -434,7 +445,8 @@ namespace build2
              bool dry_run = false,
              bool keep_going = true,
              const strings& cmd_vars = {},
-             optional<context*> module_context = nullptr);
+             optional<context*> module_context = nullptr,
+             const loaded_modules_lock* inherited_mudules_lock = nullptr);
 
     // Set current meta-operation and operation.
     //
diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx
index c2fbc14..c6c6c3d 100644
--- a/libbuild2/module.cxx
+++ b/libbuild2/module.cxx
@@ -31,6 +31,8 @@ using namespace butl;
 
 namespace build2
 {
+  mutex loaded_modules_lock::mutex_;
+
   loaded_module_map loaded_modules;
 
   void
@@ -195,6 +197,10 @@ namespace build2
         ctx.module_context->current_operation (op_update);
       }
 
+      // Inherit loaded_modules lock from the outer context.
+      //
+      ctx.module_context->modules_lock = ctx.modules_lock;
+
       // "Switch" to the module context.
       //
       context& ctx (*bs.ctx.module_context);
@@ -270,6 +276,8 @@ namespace build2
 
         l5 ([&]{trace << "updated " << lib;});
       }
+
+      ctx.modules_lock = nullptr; // For good measure.
     }
     else
     {
@@ -364,6 +372,11 @@ namespace build2
   {
     tracer trace ("find_module");
 
+    // Note that we hold the lock for the entire time it takes to build a
+    // module.
+    //
+    loaded_modules_lock lock (bs.ctx);
+
     // Optional modules and submodules sure make this logic convoluted. So we
     // divide it into two parts: (1) find or insert an entry (for submodule
     // or, failed that, for the main module, the latter potentially NULL) and
diff --git a/libbuild2/module.hxx b/libbuild2/module.hxx
index 3aaa1c7..50ca7da 100644
--- a/libbuild2/module.hxx
+++ b/libbuild2/module.hxx
@@ -10,6 +10,7 @@
 #include <libbuild2/types.hxx>
 #include <libbuild2/utility.hxx>
 
+#include <libbuild2/context.hxx>
 #include <libbuild2/variable.hxx>
 #include <libbuild2/diagnostics.hxx>
 
@@ -153,6 +154,38 @@ namespace build2
   // found.
   //
   using loaded_module_map = std::map<string, const module_functions*>;
+
+  // The loaded_modules map is locked per top-level (as opposed to nested)
+  // context (see context.hxx for details).
+  //
+  // Note: should only be constructed during context-wide serial execution.
+  //
+  class LIBBUILD2_SYMEXPORT loaded_modules_lock
+  {
+  public:
+    explicit
+    loaded_modules_lock (context& c)
+      : ctx_ (c), lock_ (mutex_, defer_lock)
+    {
+      if (ctx_.modules_lock == nullptr)
+      {
+        lock_.lock ();
+        ctx_.modules_lock = this;
+      }
+    }
+
+    ~loaded_modules_lock ()
+    {
+      if (ctx_.modules_lock == this)
+        ctx_.modules_lock = nullptr;
+    }
+
+  private:
+    static mutex mutex_;
+    context& ctx_;
+    mlock lock_;
+  };
+
   LIBBUILD2_SYMEXPORT extern loaded_module_map loaded_modules;
 
   // Load a builtin module (i.e., a module linked as a static/shared library
-- 
cgit v1.1