Re: why does gccgit require pthread?

[Jonathan Wakely <jwakely.gcc@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Mon, 7 Nov 2022 at 13:51, Jonathan Wakely wrote:
>
> On Mon, 7 Nov 2022 at 13:33, LIU Hao wrote:
> >
> > 在 2022-11-07 20:57, Jonathan Wakely 写道:
> > > It would be a lot nicer if playback::context met the C++ Lockable
> > > requirements, and playback::context::compile () could just take a
> > > scoped lock on *this:
> > >
> > >
> >
> > Yeah yeah that makes a lot of sense. Would you please just commit that? I don't have write access to
> > GCC repo, and it takes a couple of hours for me to bootstrap GCC just for this tiny change.
>
> Somebody else needs to approve it first. I'll combine our patches and
> test and submit it properly for approval.

Here's a complete patch that actually builds now, although I'm seeing
a stage 2 vs stage 3 comparison error which I don't have time to look
into right now.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 7154 bytes --]

commit 5dde4bd09c4706617120a42c5953908ae39b5751
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 11 12:48:29 2022

    jit: Use std::mutex instead of pthread_mutex_t
    
    This allows JIT to be built with a different thread model from posix
    where pthread isn't available
    
    By renaming the acquire_mutex () and release_mutex () member functions
    to lock() and unlock() we make the playback::context type meet the C++
    Lockable requirements. This allows it to be used with a scoped lock
    (i.e. RAII) type as std::lock_guard. This automatically releases the
    mutex when leaving the scope.
    
    Co-authored-by: LIU Hao <lh_mouse@126.com>
    
    gcc/jit/ChangeLog:
    
            * jit-playback.cc (playback::context::scoped_lock): Define RAII
            lock type.
            (playback::context::compile): Use scoped_lock to acquire mutex
            for the active playback context.
            (jit_mutex): Change to std::mutex.
            (playback::context::acquire_mutex): Rename to ...
            (playback::context::lock): ... this.
            (playback::context::release_mutex): Rename to ...
            (playback::context::unlock): ... this.
            * jit-playback.h (playback::context): Rename members and declare
            scoped_lock.
            * jit-recording.cc (INCLUDE_PTHREAD_H): Remove unused define.
            * libgccjit.cc (version_mutex): Change to std::mutex.
            (struct jit_version_info): Use std::lock_guard to acquire and
            release mutex.
    
    gcc/ChangeLog:
    
            * system.h [INCLUDE_MUTEX]: Include header for std::mutex.

diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index d227d36283a..bf006903a44 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -19,7 +19,7 @@ along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
+#define INCLUDE_MUTEX
 #include "system.h"
 #include "coretypes.h"
 #include "target.h"
@@ -2302,6 +2302,20 @@ block (function *func,
   m_label_expr = NULL;
 }
 
+// This is basically std::lock_guard but it can call the private lock/unlock
+// members of playback::context.
+struct playback::context::scoped_lock
+{
+  scoped_lock (context &ctx) : m_ctx (&ctx) { m_ctx->lock (); }
+  ~scoped_lock () { m_ctx->unlock (); }
+
+  context *m_ctx;
+
+  // Not movable or copyable.
+  scoped_lock (scoped_lock &&) = delete;
+  scoped_lock &operator= (scoped_lock &&) = delete;
+};
+
 /* Compile a playback::context:
 
    - Use the context's options to cconstruct command-line options, and
@@ -2353,15 +2367,12 @@ compile ()
   m_recording_ctxt->get_all_requested_dumps (&requested_dumps);
 
   /* Acquire the JIT mutex and set "this" as the active playback ctxt.  */
-  acquire_mutex ();
+  scoped_lock lock(*this);
 
   auto_string_vec fake_args;
   make_fake_args (&fake_args, ctxt_progname, &requested_dumps);
   if (errors_occurred ())
-    {
-      release_mutex ();
-      return;
-    }
+    return;
 
   /* This runs the compiler.  */
   toplev toplev (get_timer (), /* external_timer */
@@ -2388,10 +2399,7 @@ compile ()
      followup activities use timevars, which are global state.  */
 
   if (errors_occurred ())
-    {
-      release_mutex ();
-      return;
-    }
+    return;
 
   if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_GENERATED_CODE))
     dump_generated_code ();
@@ -2403,8 +2411,6 @@ compile ()
      convert the .s file to the requested output format, and copy it to a
      given file (playback::compile_to_file).  */
   postprocess (ctxt_progname);
-
-  release_mutex ();
 }
 
 /* Implementation of class gcc::jit::playback::compile_to_memory,
@@ -2662,18 +2668,18 @@ playback::compile_to_file::copy_file (const char *src_path,
 /* This mutex guards gcc::jit::recording::context::compile, so that only
    one thread can be accessing the bulk of GCC's state at once.  */
 
-static pthread_mutex_t jit_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex jit_mutex;
 
 /* Acquire jit_mutex and set "this" as the active playback ctxt.  */
 
 void
-playback::context::acquire_mutex ()
+playback::context::lock ()
 {
   auto_timevar tv (get_timer (), TV_JIT_ACQUIRING_MUTEX);
 
   /* Acquire the big GCC mutex. */
   JIT_LOG_SCOPE (get_logger ());
-  pthread_mutex_lock (&jit_mutex);
+  jit_mutex.lock ();
   gcc_assert (active_playback_ctxt == NULL);
   active_playback_ctxt = this;
 }
@@ -2681,13 +2687,13 @@ playback::context::acquire_mutex ()
 /* Release jit_mutex and clear the active playback ctxt.  */
 
 void
-playback::context::release_mutex ()
+playback::context::unlock ()
 {
   /* Release the big GCC mutex. */
   JIT_LOG_SCOPE (get_logger ());
   gcc_assert (active_playback_ctxt == this);
   active_playback_ctxt = NULL;
-  pthread_mutex_unlock (&jit_mutex);
+  jit_mutex.unlock ();
 }
 
 /* Callback used by gcc::jit::playback::context::make_fake_args when
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 3ba02a0451a..1aeee2c8046 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -314,8 +314,9 @@ private:
 
   /* Functions for implementing "compile".  */
 
-  void acquire_mutex ();
-  void release_mutex ();
+  void lock ();
+  void unlock ();
+  struct scoped_lock;
 
   void
   make_fake_args (vec <char *> *argvec,
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f78daed2d71..6ae5a667e90 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -19,7 +19,6 @@ along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
 #include "system.h"
 #include "coretypes.h"
 #include "tm.h"
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index ca862662777..8884128e8d8 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -19,7 +19,7 @@ along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
+#define INCLUDE_MUTEX
 #include "system.h"
 #include "coretypes.h"
 #include "timevar.h"
@@ -4060,7 +4060,7 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context *ctxt,
    Ideally this would be within parse_basever, but the mutex is only needed
    by libgccjit.  */
 
-static pthread_mutex_t version_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex version_mutex;
 
 struct jit_version_info
 {
@@ -4068,9 +4068,8 @@ struct jit_version_info
      guarded by version_mutex.  */
   jit_version_info ()
   {
-    pthread_mutex_lock (&version_mutex);
+    std::lock_guard<std::mutex> g (version_mutex);
     parse_basever (&major, &minor, &patchlevel);
-    pthread_mutex_unlock (&version_mutex);
   }
 
   int major;
diff --git a/gcc/system.h b/gcc/system.h
index de9c5c0d2ef..4f9256efbcf 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -747,6 +747,10 @@ extern int vsnprintf (char *, size_t, const char *, va_list);
 # include <memory>
 #endif
 
+#ifdef INCLUDE_MUTEX
+# include <mutex>
+#endif
+
 #ifdef INCLUDE_MALLOC_H
 #if defined(HAVE_MALLINFO) || defined(HAVE_MALLINFO2)
 #include <malloc.h>