Re: why does gccgit require pthread?

Re: why does gccgit require pthread?

[LIU Hao]

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

在 2022-11-07 12:37, Andrew Pinski 写道:
> 
> The original code which used pthread was added in GCC 5 way before GCC
> moved to being written in C++11 which was only in the last 3 years.
> pthread_* functions were the best choice at the time (2014) but now
> GCC is written in C++11, I don't see any reason not to move them over
> to using C++11 threading code.
> 
>

Attached is the proposed patch.

The win32 thread model does not have `std::mutex`; but there is no `pthread_mutex_t` either, so it 
does not build either way.

Tested bootstrapping GCC on `{i686,x86_64}-w64-mingw32` with languages 
`c,lto,c++,fortran,objc,obj-c++` and with the `mcf` thread model; no errors observed. The built 
`libgccjit-0.dll` does not have imports from winpthread any more.

Please review.


-- 
Best regards,
LIU Hao


[-- Attachment #1.1.2: 9005-gcc-jit-Use-C-11-mutex-instead-of-pthread-s.patch --]
[-- Type: text/plain, Size: 4067 bytes --]

From ceb65f21b5ac23ce218efee82f40f641ebe44361 Mon Sep 17 00:00:00 2001
From: LIU Hao <lh_mouse@126.com>
Date: Mon, 7 Nov 2022 13:00:12 +0800
Subject: [PATCH] gcc/jit: Use C++11 mutex instead of pthread's

This allows JIT to be built with a different thread model from `posix`
where pthread isn't available

gcc/jit/ChangeLog:

	* jit-playback.cc: Use `std::mutex` instead of `pthread_mutex_t`
	(playback::context::acquire_mutex): Likewise
	(playback::context::release_mutex): Likewise
	* jit-recording.cc: Remove the unused `INCLUDE_PTHREAD_H`
	* libgccjit.cc: Use `std::mutex` instead of `pthread_mutex_t`
---
 gcc/jit/jit-playback.cc  | 9 +++++----
 gcc/jit/jit-recording.cc | 1 -
 gcc/jit/libgccjit.cc     | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index d227d36283a..17ff98c149b 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.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 "target.h"
@@ -51,6 +50,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "jit-w32.h"
 #endif
 
+#include <mutex>
+
 /* Compare with gcc/c-family/c-common.h: DECL_C_BIT_FIELD,
    SET_DECL_C_BIT_FIELD.
    These are redefined here to avoid depending from the C frontend.  */
@@ -2662,7 +2663,7 @@ 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.  */
 
@@ -2673,7 +2674,7 @@ playback::context::acquire_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;
 }
@@ -2687,7 +2688,7 @@ playback::context::release_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-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..a5105fbc1f9 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.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 "timevar.h"
@@ -30,6 +29,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "jit-recording.h"
 #include "jit-result.h"
 
+#include <mutex>
+
 /* The opaque types used by the public API are actually subclasses
    of the gcc::jit::recording classes.  */
 
@@ -4060,7 +4061,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 +4069,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;
-- 
2.38.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: why does gccgit require pthread?

[Andrew Pinski]

On Sun, Nov 6, 2022 at 10:51 PM LIU Hao <lh_mouse@126.com> wrote:
>
> 在 2022-11-07 12:37, Andrew Pinski 写道:
> >
> > The original code which used pthread was added in GCC 5 way before GCC
> > moved to being written in C++11 which was only in the last 3 years.
> > pthread_* functions were the best choice at the time (2014) but now
> > GCC is written in C++11, I don't see any reason not to move them over
> > to using C++11 threading code.
> >
> >
>
> Attached is the proposed patch.
>
> The win32 thread model does not have `std::mutex`; but there is no `pthread_mutex_t` either, so it
> does not build either way.
Oh, but I would assume it will later on right?

Also I think you might need to change some more than you did.
That is:
-#define INCLUDE_PTHREAD_H
 #include "system.h"

You must likely have a macro, INCLUDE_MUTEX, and define that and
include mutex in system.h like it was done for pthread.h.
GCC loves to poison identifiers while lexing to make sure those
identifiers are not used inside GCC and the include of mutex should be
done early.

Thanks,
Andrew

>
> Tested bootstrapping GCC on `{i686,x86_64}-w64-mingw32` with languages
> `c,lto,c++,fortran,objc,obj-c++` and with the `mcf` thread model; no errors observed. The built
> `libgccjit-0.dll` does not have imports from winpthread any more.
>
> Please review.
>
>
> --
> Best regards,
> LIU Hao
>

Re: why does gccgit require pthread?

[LIU Hao]

[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]

在 2022/11/7 15:03, Andrew Pinski 写道:
>>
>> The win32 thread model does not have `std::mutex`; but there is no `pthread_mutex_t` either, so it
>> does not build either way.
> Oh, but I would assume it will later on right?
> 

There has been effort on C++11 threading support for win32 thread model, but I have a negative 
attitude on that.

Another solution is to use `__gthread_mutex_t` instead of `pthread_mutex_t`, which is also available 
in the win32 thread model. Actually I prefer this approach as it keeps code structure like what we 
have at this moment.


> Also I think you might need to change some more than you did.
> That is:
> -#define INCLUDE_PTHREAD_H
>   #include "system.h"
> 
> You must likely have a macro, INCLUDE_MUTEX, and define that and
> include mutex in system.h like it was done for pthread.h.
> GCC loves to poison identifiers while lexing to make sure those
> identifiers are not used inside GCC and the include of mutex should be
> done early.
> 

Well I am not familiar with such behavior. Feel free to amend the patch, until it looks good to you. 
I hope we can check this in before GCC 13 RC.



-- 
Best regards,
LIU Hao


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: why does gccgit require pthread?

[Jonathan Wakely]

On Mon, 7 Nov 2022 at 06:51, LIU Hao via Gcc <gcc@gcc.gnu.org> wrote:
>
> 在 2022-11-07 12:37, Andrew Pinski 写道:
> >
> > The original code which used pthread was added in GCC 5 way before GCC
> > moved to being written in C++11 which was only in the last 3 years.
> > pthread_* functions were the best choice at the time (2014) but now
> > GCC is written in C++11, I don't see any reason not to move them over
> > to using C++11 threading code.
> >
> >
>
> Attached is the proposed patch.

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:


--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -2353,15 +2353,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 ();
+  std::lock_guard<context> 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 +2385,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 +2397,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 +2654,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 +2673,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..bd2bc389f53 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -314,8 +314,8 @@ private:

  /* Functions for implementing "compile".  */

-  void acquire_mutex ();
-  void release_mutex ();
+  void lock ();
+  void unlock ();

  void
  make_fake_args (vec <char *> *argvec,

Re: why does gccgit require pthread?

[LIU Hao]

[-- Attachment #1.1: Type: text/plain, Size: 451 bytes --]

在 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.


-- 
Best regards,
LIU Hao


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: why does gccgit require pthread?

[Jonathan Wakely]

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.

Re: why does gccgit require pthread?

[Jonathan Wakely]

[-- 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>

Re: why does gccgit require pthread?

[Jonathan Wakely]

On Fri, 11 Nov 2022 at 17:16, Jonathan Wakely wrote:
>
> 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.

A clean build fixed that. This patch bootstraps and passes testing on
x86_64-pc-linux-gnu (CentOS 8 Stream).

OK for trunk?

Re: why does gccgit require pthread?

[LIU Hao]

[-- Attachment #1.1: Type: text/plain, Size: 284 bytes --]

在 2022/11/12 02:27, Jonathan Wakely 写道:
> 
> A clean build fixed that. This patch bootstraps and passes testing on
> x86_64-pc-linux-gnu (CentOS 8 Stream).
> 
> OK for trunk?

What should we do if no one has been approving this patch?


-- 
Best regards,
LIU Hao


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: why does gcc jit require pthread?

[David Malcolm]

[Fixing typo in the Subject ("git" -> "jit" ); CCing jit mailing list]

On Fri, 2022-11-11 at 17:16 +0000, Jonathan Wakely wrote:
> 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.

I confess that I'm not familiar with C++11's mutex and locking types,
but having read through the relevant entries on cppreference.com, the
patch looks correct to me.

Are these classes well-supported on the minimum compiler version we
support?  (Jonathan, I defer to your judgement here)

Jonathan: you said in your followup email that it "bootstraps and
passes testing on x86_64-pc-linux-gnu (CentOS 8 Stream)".  This is
possibly a silly question, but did this testing include the jit
testsuite?  A gotcha here is that --enable-languages=all does *not*
enable jit.

The patch is OK for trunk if you have favorable answers for the above
two questions.

Thanks!
Dave

Re: why does gcc jit require pthread?

[Jonathan Wakely]

On Tue, 15 Nov 2022 at 18:50, David Malcolm <dmalcolm@redhat.com> wrote:
>
> [Fixing typo in the Subject ("git" -> "jit" ); CCing jit mailing list]
>
> On Fri, 2022-11-11 at 17:16 +0000, Jonathan Wakely wrote:
> > 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.
>
> I confess that I'm not familiar with C++11's mutex and locking types,
> but having read through the relevant entries on cppreference.com, the
> patch looks correct to me.
>
> Are these classes well-supported on the minimum compiler version we
> support?  (Jonathan, I defer to your judgement here)

std::mutex has been supported since 4.4.0 and is very simple. The
implementation on trunk is identical to the one in gcc 4.8.5 except
for adding 'noexcept' to mutex::native_handle (), which is not
relevant to this change.

> Jonathan: you said in your followup email that it "bootstraps and
> passes testing on x86_64-pc-linux-gnu (CentOS 8 Stream)".  This is
> possibly a silly question, but did this testing include the jit
> testsuite?  A gotcha here is that --enable-languages=all does *not*
> enable jit.

Yes, I built with --enable-languages=c,c++,jit --enable-host-shared

> The patch is OK for trunk if you have favorable answers for the above
> two questions.
>
> Thanks!
> Dave
>

Re: why does gcc jit require pthread?

[Jonathan Wakely]

On Tue, 15 Nov 2022 at 19:01, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 15 Nov 2022 at 18:50, David Malcolm <dmalcolm@redhat.com> wrote:
> >
> > [Fixing typo in the Subject ("git" -> "jit" ); CCing jit mailing list]
> >
> > On Fri, 2022-11-11 at 17:16 +0000, Jonathan Wakely wrote:
> > > 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.
> >
> > I confess that I'm not familiar with C++11's mutex and locking types,
> > but having read through the relevant entries on cppreference.com, the
> > patch looks correct to me.
> >
> > Are these classes well-supported on the minimum compiler version we
> > support?  (Jonathan, I defer to your judgement here)
>
> std::mutex has been supported since 4.4.0 and is very simple. The
> implementation on trunk is identical to the one in gcc 4.8.5 except
> for adding 'noexcept' to mutex::native_handle (), which is not
> relevant to this change.
>
> > Jonathan: you said in your followup email that it "bootstraps and
> > passes testing on x86_64-pc-linux-gnu (CentOS 8 Stream)".  This is
> > possibly a silly question, but did this testing include the jit
> > testsuite?  A gotcha here is that --enable-languages=all does *not*
> > enable jit.
>
> Yes, I built with --enable-languages=c,c++,jit --enable-host-shared

I rebased the patch and re-tested with those options, and all tests
passed again:


               === jit Summary ===

# of expected passes            15081




> > The patch is OK for trunk if you have favorable answers for the above
> > two questions.

Thanks, I've pushed it to trunk now.

Re: why does gcc jit require pthread?

[LIU Hao]

[-- Attachment #1.1: Type: text/plain, Size: 453 bytes --]

在 2022-11-19 19:27, Jonathan Wakely 写道:
> I rebased the patch and re-tested with those options, and all tests
> passed again:
> 
> 
>                 === jit Summary ===
> 
> # of expected passes            15081
> 
> 
> 
> 
>>> The patch is OK for trunk if you have favorable answers for the above
>>> two questions.
> 
> Thanks, I've pushed it to trunk now.

Thank you for taking care of it!


-- 
Best regards,
LIU Hao


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]