[review] Use run_on_main_thread in gdb.post_event

[review] Use run_on_main_thread in gdb.post_event

[Tom Tromey (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................

Use run_on_main_thread in gdb.post_event

This changes gdb.post_event to use the new run_on_main_thread
function.  This is somewhat tricky because the Python GIL must be held
while manipulating reference counts.

gdb/ChangeLog
2019-10-19  Tom Tromey  <tom@tromey.com>

	* python/python.c (class gdbpy_gil): New.
	(struct gdbpy_event): Add constructor, destructor, operator().
	(gdbpy_post_event): Use run_on_main_thread.
	(gdbpy_initialize_events): Remove.
	(do_start_initialization): Update.

Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
---
M gdb/ChangeLog
M gdb/python/python.c
2 files changed, 76 insertions(+), 75 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7be0051..ec8f644 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-10-19  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (class gdbpy_gil): New.
+	(struct gdbpy_event): Add constructor, destructor, operator().
+	(gdbpy_post_event): Use run_on_main_thread.
+	(gdbpy_initialize_events): Remove.
+	(do_start_initialization): Update.
+
+2019-10-19  Tom Tromey  <tom@tromey.com>
+
 	* NEWS: Add entry.
 	* maint.c (_initialize_maint_cmds): Add "worker-threads" maint
 	commands.  Call update_thread_pool_size.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index ddf0e72..1e05457 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -28,7 +28,6 @@
 #include "value.h"
 #include "language.h"
 #include "event-loop.h"
-#include "serial.h"
 #include "readline/tilde.h"
 #include "python.h"
 #include "extension-priv.h"
@@ -940,63 +939,81 @@
 
 /* Posting and handling events.  */
 
+/* A helper class to save and restore the GIL, but without touching
+   the other globals that are handled by gdbpy_enter.  */
+
+class gdbpy_gil
+{
+public:
+
+  gdbpy_gil ()
+    : m_state (PyGILState_Ensure ())
+  {
+  }
+
+  ~gdbpy_gil ()
+  {
+    PyGILState_Release (m_state);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (gdbpy_gil);
+
+private:
+
+  PyGILState_STATE m_state;
+};
+
 /* A single event.  */
 struct gdbpy_event
 {
-  /* The Python event.  This is just a callable object.  */
-  PyObject *event;
-  /* The next event.  */
-  struct gdbpy_event *next;
+  gdbpy_event (gdbpy_ref<> &&func)
+    : m_func (func.release ())
+  {
+  }
+
+  gdbpy_event (gdbpy_event &&other)
+    : m_func (other.m_func)
+  {
+    other.m_func = nullptr;
+  }
+
+  gdbpy_event (const gdbpy_event &other)
+    : m_func (other.m_func)
+  {
+    gdbpy_gil gil;
+    Py_XINCREF (m_func);
+  }
+
+  ~gdbpy_event ()
+  {
+    gdbpy_gil gil;
+    Py_XDECREF (m_func);
+  }
+
+  gdbpy_event &operator= (const gdbpy_event &other) = delete;
+
+  void operator() ()
+  {
+    gdbpy_enter enter_py (get_current_arch (), current_language);
+
+    gdbpy_ref<> call_result (PyObject_CallObject (m_func, NULL));
+    if (call_result == NULL)
+      gdbpy_print_stack ();
+  }
+
+private:
+
+  /* The Python event.  This is just a callable object.  Note that
+     this is not a gdbpy_ref<>, because we have to take particular
+     care to only destroy the reference when holding the GIL. */
+  PyObject *m_func;
 };
 
-/* All pending events.  */
-static struct gdbpy_event *gdbpy_event_list;
-/* The final link of the event list.  */
-static struct gdbpy_event **gdbpy_event_list_end;
-
-/* So that we can wake up the main thread even when it is blocked in
-   poll().  */
-static struct serial_event *gdbpy_serial_event;
-
-/* The file handler callback.  This reads from the internal pipe, and
-   then processes the Python event queue.  This will always be run in
-   the main gdb thread.  */
-
-static void
-gdbpy_run_events (int error, gdb_client_data client_data)
-{
-  gdbpy_enter enter_py (get_current_arch (), current_language);
-
-  /* Clear the event fd.  Do this before flushing the events list, so
-     that any new event post afterwards is sure to re-awake the event
-     loop.  */
-  serial_event_clear (gdbpy_serial_event);
-
-  while (gdbpy_event_list)
-    {
-      /* Dispatching the event might push a new element onto the event
-	 loop, so we update here "atomically enough".  */
-      struct gdbpy_event *item = gdbpy_event_list;
-      gdbpy_event_list = gdbpy_event_list->next;
-      if (gdbpy_event_list == NULL)
-	gdbpy_event_list_end = &gdbpy_event_list;
-
-      gdbpy_ref<> call_result (PyObject_CallObject (item->event, NULL));
-      if (call_result == NULL)
-	gdbpy_print_stack ();
-
-      Py_DECREF (item->event);
-      xfree (item);
-    }
-}
-
 /* Submit an event to the gdb thread.  */
 static PyObject *
 gdbpy_post_event (PyObject *self, PyObject *args)
 {
-  struct gdbpy_event *event;
   PyObject *func;
-  int wakeup;
 
   if (!PyArg_ParseTuple (args, "O", &func))
     return NULL;
@@ -1010,36 +1027,13 @@
 
   Py_INCREF (func);
 
-  /* From here until the end of the function, we have the GIL, so we
-     can operate on our global data structures without worrying.  */
-  wakeup = gdbpy_event_list == NULL;
-
-  event = XNEW (struct gdbpy_event);
-  event->event = func;
-  event->next = NULL;
-  *gdbpy_event_list_end = event;
-  gdbpy_event_list_end = &event->next;
-
-  /* Wake up gdb when needed.  */
-  if (wakeup)
-    serial_event_set (gdbpy_serial_event);
+  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);
+  gdbpy_event event (std::move (func_ref));
+  run_on_main_thread (event);
 
   Py_RETURN_NONE;
 }
 
-/* Initialize the Python event handler.  */
-static int
-gdbpy_initialize_events (void)
-{
-  gdbpy_event_list_end = &gdbpy_event_list;
-
-  gdbpy_serial_event = make_serial_event ();
-  add_file_handler (serial_event_fd (gdbpy_serial_event),
-		    gdbpy_run_events, NULL);
-
-  return 0;
-}
-
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -1704,7 +1698,6 @@
       || gdbpy_initialize_linetable () < 0
       || gdbpy_initialize_thread () < 0
       || gdbpy_initialize_inferior () < 0
-      || gdbpy_initialize_events () < 0
       || gdbpy_initialize_eventregistry () < 0
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0

[review v2] Use run_on_main_thread in gdb.post_event

[Tom Tromey (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................

Use run_on_main_thread in gdb.post_event

This changes gdb.post_event to use the new run_on_main_thread
function.  This is somewhat tricky because the Python GIL must be held
while manipulating reference counts.

2019-10-19  Tom Tromey  <tom@tromey.com>

	* python/python.c (class gdbpy_gil): New.
	(struct gdbpy_event): Add constructor, destructor, operator().
	(gdbpy_post_event): Use run_on_main_thread.
	(gdbpy_initialize_events): Remove.
	(do_start_initialization): Update.

Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
---
M gdb/ChangeLog
M gdb/python/python.c
2 files changed, 76 insertions(+), 75 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d7b40d7..3eeee1e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-10-19  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (class gdbpy_gil): New.
+	(struct gdbpy_event): Add constructor, destructor, operator().
+	(gdbpy_post_event): Use run_on_main_thread.
+	(gdbpy_initialize_events): Remove.
+	(do_start_initialization): Update.
+
+2019-10-19  Tom Tromey  <tom@tromey.com>
+
 	* NEWS: Add entry.
 	* maint.c (_initialize_maint_cmds): Add "worker-threads" maint
 	commands.  Call update_thread_pool_size.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index ddf0e72..1e05457 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -28,7 +28,6 @@
 #include "value.h"
 #include "language.h"
 #include "event-loop.h"
-#include "serial.h"
 #include "readline/tilde.h"
 #include "python.h"
 #include "extension-priv.h"
@@ -940,63 +939,81 @@
 
 /* Posting and handling events.  */
 
+/* A helper class to save and restore the GIL, but without touching
+   the other globals that are handled by gdbpy_enter.  */
+
+class gdbpy_gil
+{
+public:
+
+  gdbpy_gil ()
+    : m_state (PyGILState_Ensure ())
+  {
+  }
+
+  ~gdbpy_gil ()
+  {
+    PyGILState_Release (m_state);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (gdbpy_gil);
+
+private:
+
+  PyGILState_STATE m_state;
+};
+
 /* A single event.  */
 struct gdbpy_event
 {
-  /* The Python event.  This is just a callable object.  */
-  PyObject *event;
-  /* The next event.  */
-  struct gdbpy_event *next;
+  gdbpy_event (gdbpy_ref<> &&func)
+    : m_func (func.release ())
+  {
+  }
+
+  gdbpy_event (gdbpy_event &&other)
+    : m_func (other.m_func)
+  {
+    other.m_func = nullptr;
+  }
+
+  gdbpy_event (const gdbpy_event &other)
+    : m_func (other.m_func)
+  {
+    gdbpy_gil gil;
+    Py_XINCREF (m_func);
+  }
+
+  ~gdbpy_event ()
+  {
+    gdbpy_gil gil;
+    Py_XDECREF (m_func);
+  }
+
+  gdbpy_event &operator= (const gdbpy_event &other) = delete;
+
+  void operator() ()
+  {
+    gdbpy_enter enter_py (get_current_arch (), current_language);
+
+    gdbpy_ref<> call_result (PyObject_CallObject (m_func, NULL));
+    if (call_result == NULL)
+      gdbpy_print_stack ();
+  }
+
+private:
+
+  /* The Python event.  This is just a callable object.  Note that
+     this is not a gdbpy_ref<>, because we have to take particular
+     care to only destroy the reference when holding the GIL. */
+  PyObject *m_func;
 };
 
-/* All pending events.  */
-static struct gdbpy_event *gdbpy_event_list;
-/* The final link of the event list.  */
-static struct gdbpy_event **gdbpy_event_list_end;
-
-/* So that we can wake up the main thread even when it is blocked in
-   poll().  */
-static struct serial_event *gdbpy_serial_event;
-
-/* The file handler callback.  This reads from the internal pipe, and
-   then processes the Python event queue.  This will always be run in
-   the main gdb thread.  */
-
-static void
-gdbpy_run_events (int error, gdb_client_data client_data)
-{
-  gdbpy_enter enter_py (get_current_arch (), current_language);
-
-  /* Clear the event fd.  Do this before flushing the events list, so
-     that any new event post afterwards is sure to re-awake the event
-     loop.  */
-  serial_event_clear (gdbpy_serial_event);
-
-  while (gdbpy_event_list)
-    {
-      /* Dispatching the event might push a new element onto the event
-	 loop, so we update here "atomically enough".  */
-      struct gdbpy_event *item = gdbpy_event_list;
-      gdbpy_event_list = gdbpy_event_list->next;
-      if (gdbpy_event_list == NULL)
-	gdbpy_event_list_end = &gdbpy_event_list;
-
-      gdbpy_ref<> call_result (PyObject_CallObject (item->event, NULL));
-      if (call_result == NULL)
-	gdbpy_print_stack ();
-
-      Py_DECREF (item->event);
-      xfree (item);
-    }
-}
-
 /* Submit an event to the gdb thread.  */
 static PyObject *
 gdbpy_post_event (PyObject *self, PyObject *args)
 {
-  struct gdbpy_event *event;
   PyObject *func;
-  int wakeup;
 
   if (!PyArg_ParseTuple (args, "O", &func))
     return NULL;
@@ -1010,36 +1027,13 @@
 
   Py_INCREF (func);
 
-  /* From here until the end of the function, we have the GIL, so we
-     can operate on our global data structures without worrying.  */
-  wakeup = gdbpy_event_list == NULL;
-
-  event = XNEW (struct gdbpy_event);
-  event->event = func;
-  event->next = NULL;
-  *gdbpy_event_list_end = event;
-  gdbpy_event_list_end = &event->next;
-
-  /* Wake up gdb when needed.  */
-  if (wakeup)
-    serial_event_set (gdbpy_serial_event);
+  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);
+  gdbpy_event event (std::move (func_ref));
+  run_on_main_thread (event);
 
   Py_RETURN_NONE;
 }
 
-/* Initialize the Python event handler.  */
-static int
-gdbpy_initialize_events (void)
-{
-  gdbpy_event_list_end = &gdbpy_event_list;
-
-  gdbpy_serial_event = make_serial_event ();
-  add_file_handler (serial_event_fd (gdbpy_serial_event),
-		    gdbpy_run_events, NULL);
-
-  return 0;
-}
-
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -1704,7 +1698,6 @@
       || gdbpy_initialize_linetable () < 0
       || gdbpy_initialize_thread () < 0
       || gdbpy_initialize_inferior () < 0
-      || gdbpy_initialize_events () < 0
       || gdbpy_initialize_eventregistry () < 0
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0

[review v4] Use run_on_main_thread in gdb.post_event

[Tom Tromey (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................

Use run_on_main_thread in gdb.post_event

This changes gdb.post_event to use the new run_on_main_thread
function.  This is somewhat tricky because the Python GIL must be held
while manipulating reference counts.

2019-10-19  Tom Tromey  <tom@tromey.com>

	* python/python.c (class gdbpy_gil): New.
	(struct gdbpy_event): Add constructor, destructor, operator().
	(gdbpy_post_event): Use run_on_main_thread.
	(gdbpy_initialize_events): Remove.
	(do_start_initialization): Update.

Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
---
M gdb/ChangeLog
M gdb/python/python.c
2 files changed, 77 insertions(+), 76 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1d627af..97245ff 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-10-19  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (class gdbpy_gil): New.
+	(struct gdbpy_event): Add constructor, destructor, operator().
+	(gdbpy_post_event): Use run_on_main_thread.
+	(gdbpy_initialize_events): Remove.
+	(do_start_initialization): Update.
+
+2019-10-19  Tom Tromey  <tom@tromey.com>
+
 	* NEWS: Add entry.
 	* maint.c (_initialize_maint_cmds): Add "worker-threads" maint
 	commands.  Call update_thread_pool_size.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 7b561a1..47bfae2 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -28,14 +28,13 @@
 #include "value.h"
 #include "language.h"
 #include "event-loop.h"
-#include "serial.h"
 #include "readline/tilde.h"
 #include "python.h"
 #include "extension-priv.h"
 #include "cli/cli-utils.h"
 #include <ctype.h>
 #include "location.h"
-#include "ser-event.h"
+#include "run-on-main-thread.h"
 
 /* Declared constants and enum for python stack printing.  */
 static const char python_excp_none[] = "none";
@@ -940,63 +939,81 @@
 
 /* Posting and handling events.  */
 
+/* A helper class to save and restore the GIL, but without touching
+   the other globals that are handled by gdbpy_enter.  */
+
+class gdbpy_gil
+{
+public:
+
+  gdbpy_gil ()
+    : m_state (PyGILState_Ensure ())
+  {
+  }
+
+  ~gdbpy_gil ()
+  {
+    PyGILState_Release (m_state);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (gdbpy_gil);
+
+private:
+
+  PyGILState_STATE m_state;
+};
+
 /* A single event.  */
 struct gdbpy_event
 {
-  /* The Python event.  This is just a callable object.  */
-  PyObject *event;
-  /* The next event.  */
-  struct gdbpy_event *next;
+  gdbpy_event (gdbpy_ref<> &&func)
+    : m_func (func.release ())
+  {
+  }
+
+  gdbpy_event (gdbpy_event &&other)
+    : m_func (other.m_func)
+  {
+    other.m_func = nullptr;
+  }
+
+  gdbpy_event (const gdbpy_event &other)
+    : m_func (other.m_func)
+  {
+    gdbpy_gil gil;
+    Py_XINCREF (m_func);
+  }
+
+  ~gdbpy_event ()
+  {
+    gdbpy_gil gil;
+    Py_XDECREF (m_func);
+  }
+
+  gdbpy_event &operator= (const gdbpy_event &other) = delete;
+
+  void operator() ()
+  {
+    gdbpy_enter enter_py (get_current_arch (), current_language);
+
+    gdbpy_ref<> call_result (PyObject_CallObject (m_func, NULL));
+    if (call_result == NULL)
+      gdbpy_print_stack ();
+  }
+
+private:
+
+  /* The Python event.  This is just a callable object.  Note that
+     this is not a gdbpy_ref<>, because we have to take particular
+     care to only destroy the reference when holding the GIL. */
+  PyObject *m_func;
 };
 
-/* All pending events.  */
-static struct gdbpy_event *gdbpy_event_list;
-/* The final link of the event list.  */
-static struct gdbpy_event **gdbpy_event_list_end;
-
-/* So that we can wake up the main thread even when it is blocked in
-   poll().  */
-static struct serial_event *gdbpy_serial_event;
-
-/* The file handler callback.  This reads from the internal pipe, and
-   then processes the Python event queue.  This will always be run in
-   the main gdb thread.  */
-
-static void
-gdbpy_run_events (int error, gdb_client_data client_data)
-{
-  gdbpy_enter enter_py (get_current_arch (), current_language);
-
-  /* Clear the event fd.  Do this before flushing the events list, so
-     that any new event post afterwards is sure to re-awake the event
-     loop.  */
-  serial_event_clear (gdbpy_serial_event);
-
-  while (gdbpy_event_list)
-    {
-      /* Dispatching the event might push a new element onto the event
-	 loop, so we update here "atomically enough".  */
-      struct gdbpy_event *item = gdbpy_event_list;
-      gdbpy_event_list = gdbpy_event_list->next;
-      if (gdbpy_event_list == NULL)
-	gdbpy_event_list_end = &gdbpy_event_list;
-
-      gdbpy_ref<> call_result (PyObject_CallObject (item->event, NULL));
-      if (call_result == NULL)
-	gdbpy_print_stack ();
-
-      Py_DECREF (item->event);
-      xfree (item);
-    }
-}
-
 /* Submit an event to the gdb thread.  */
 static PyObject *
 gdbpy_post_event (PyObject *self, PyObject *args)
 {
-  struct gdbpy_event *event;
   PyObject *func;
-  int wakeup;
 
   if (!PyArg_ParseTuple (args, "O", &func))
     return NULL;
@@ -1010,36 +1027,13 @@
 
   Py_INCREF (func);
 
-  /* From here until the end of the function, we have the GIL, so we
-     can operate on our global data structures without worrying.  */
-  wakeup = gdbpy_event_list == NULL;
-
-  event = XNEW (struct gdbpy_event);
-  event->event = func;
-  event->next = NULL;
-  *gdbpy_event_list_end = event;
-  gdbpy_event_list_end = &event->next;
-
-  /* Wake up gdb when needed.  */
-  if (wakeup)
-    serial_event_set (gdbpy_serial_event);
+  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);
+  gdbpy_event event (std::move (func_ref));
+  run_on_main_thread (event);
 
   Py_RETURN_NONE;
 }
 
-/* Initialize the Python event handler.  */
-static int
-gdbpy_initialize_events (void)
-{
-  gdbpy_event_list_end = &gdbpy_event_list;
-
-  gdbpy_serial_event = make_serial_event ();
-  add_file_handler (serial_event_fd (gdbpy_serial_event),
-		    gdbpy_run_events, NULL);
-
-  return 0;
-}
-
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -1704,7 +1698,6 @@
       || gdbpy_initialize_linetable () < 0
       || gdbpy_initialize_thread () < 0
       || gdbpy_initialize_inferior () < 0
-      || gdbpy_initialize_events () < 0
       || gdbpy_initialize_eventregistry () < 0
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
Gerrit-Change-Number: 175
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v4] Use run_on_main_thread in gdb.post_event

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................


Patch Set 4: Code-Review+2

(1 comment)

Thanks for following through with doing this.

One comment about refcount handling.  Otherwise LGTM.

| --- gdb/python/python.c
| +++ gdb/python/python.c
| @@ -1017,16 +1030,7 @@ gdbpy_post_event (PyObject *self, PyObject *args)
| -  event = XNEW (struct gdbpy_event);
| -  event->event = func;
| -  event->next = NULL;
| -  *gdbpy_event_list_end = event;
| -  gdbpy_event_list_end = &event->next;
| -
| -  /* Wake up gdb when needed.  */
| -  if (wakeup)
| -    serial_event_set (gdbpy_serial_event);
| +  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);

PS4, Line 1030:

gdbpy_ref<>::new_reference does incref inside, and I see there's an
Py_INCREF above.

I don't know whether it's correct or not, but it caught my eye.  Could
you double check?

| +  gdbpy_event event (std::move (func_ref));
| +  run_on_main_thread (event);
|  
|    Py_RETURN_NONE;
|  }
|  
| -/* Initialize the Python event handler.  */
| -static int
| -gdbpy_initialize_events (void)

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
Gerrit-Change-Number: 175
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Tue, 26 Nov 2019 19:25:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v4] Use run_on_main_thread in gdb.post_event

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................


Patch Set 4:

(1 comment)

| --- gdb/python/python.c
| +++ gdb/python/python.c
| @@ -1017,16 +1030,7 @@ gdbpy_post_event (PyObject *self, PyObject *args)
| -  event = XNEW (struct gdbpy_event);
| -  event->event = func;
| -  event->next = NULL;
| -  *gdbpy_event_list_end = event;
| -  gdbpy_event_list_end = &event->next;
| -
| -  /* Wake up gdb when needed.  */
| -  if (wakeup)
| -    serial_event_set (gdbpy_serial_event);
| +  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);

PS4, Line 1030:

> gdbpy_ref<>::new_reference does incref inside, and I see there's an Py_INCREF above.
> 
> I don't know whether it's correct or not, but it caught my eye.  Could you double check?

Thanks for noticing this.  I removed the incref here.

| +  gdbpy_event event (std::move (func_ref));
| +  run_on_main_thread (event);
|  
|    Py_RETURN_NONE;
|  }
|  
| -/* Initialize the Python event handler.  */
| -static int
| -gdbpy_initialize_events (void)

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
Gerrit-Change-Number: 175
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 19:52:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

[pushed] Use run_on_main_thread in gdb.post_event

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................

Use run_on_main_thread in gdb.post_event

This changes gdb.post_event to use the new run_on_main_thread
function.  This is somewhat tricky because the Python GIL must be held
while manipulating reference counts.

2019-11-26  Tom Tromey  <tom@tromey.com>

	* python/python.c (class gdbpy_gil): New.
	(struct gdbpy_event): Add constructor, destructor, operator().
	(gdbpy_post_event): Use run_on_main_thread.
	(gdbpy_initialize_events): Remove.
	(do_start_initialization): Update.

Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
---
M gdb/ChangeLog
M gdb/python/python.c
2 files changed, 77 insertions(+), 78 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 289cbce..0c5aab2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-11-26  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (class gdbpy_gil): New.
+	(struct gdbpy_event): Add constructor, destructor, operator().
+	(gdbpy_post_event): Use run_on_main_thread.
+	(gdbpy_initialize_events): Remove.
+	(do_start_initialization): Update.
+
+2019-11-26  Tom Tromey  <tom@tromey.com>
+
 	* NEWS: Add entry.
 	* maint.c (_initialize_maint_cmds): Add "worker-threads" maint
 	commands.  Call update_thread_pool_size.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 1cc5ebb..2f4fb0f 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -28,14 +28,13 @@
 #include "value.h"
 #include "language.h"
 #include "event-loop.h"
-#include "serial.h"
 #include "readline/tilde.h"
 #include "python.h"
 #include "extension-priv.h"
 #include "cli/cli-utils.h"
 #include <ctype.h>
 #include "location.h"
-#include "ser-event.h"
+#include "run-on-main-thread.h"
 
 /* Declared constants and enum for python stack printing.  */
 static const char python_excp_none[] = "none";
@@ -940,63 +939,81 @@
 
 /* Posting and handling events.  */
 
+/* A helper class to save and restore the GIL, but without touching
+   the other globals that are handled by gdbpy_enter.  */
+
+class gdbpy_gil
+{
+public:
+
+  gdbpy_gil ()
+    : m_state (PyGILState_Ensure ())
+  {
+  }
+
+  ~gdbpy_gil ()
+  {
+    PyGILState_Release (m_state);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (gdbpy_gil);
+
+private:
+
+  PyGILState_STATE m_state;
+};
+
 /* A single event.  */
 struct gdbpy_event
 {
-  /* The Python event.  This is just a callable object.  */
-  PyObject *event;
-  /* The next event.  */
-  struct gdbpy_event *next;
+  gdbpy_event (gdbpy_ref<> &&func)
+    : m_func (func.release ())
+  {
+  }
+
+  gdbpy_event (gdbpy_event &&other)
+    : m_func (other.m_func)
+  {
+    other.m_func = nullptr;
+  }
+
+  gdbpy_event (const gdbpy_event &other)
+    : m_func (other.m_func)
+  {
+    gdbpy_gil gil;
+    Py_XINCREF (m_func);
+  }
+
+  ~gdbpy_event ()
+  {
+    gdbpy_gil gil;
+    Py_XDECREF (m_func);
+  }
+
+  gdbpy_event &operator= (const gdbpy_event &other) = delete;
+
+  void operator() ()
+  {
+    gdbpy_enter enter_py (get_current_arch (), current_language);
+
+    gdbpy_ref<> call_result (PyObject_CallObject (m_func, NULL));
+    if (call_result == NULL)
+      gdbpy_print_stack ();
+  }
+
+private:
+
+  /* The Python event.  This is just a callable object.  Note that
+     this is not a gdbpy_ref<>, because we have to take particular
+     care to only destroy the reference when holding the GIL. */
+  PyObject *m_func;
 };
 
-/* All pending events.  */
-static struct gdbpy_event *gdbpy_event_list;
-/* The final link of the event list.  */
-static struct gdbpy_event **gdbpy_event_list_end;
-
-/* So that we can wake up the main thread even when it is blocked in
-   poll().  */
-static struct serial_event *gdbpy_serial_event;
-
-/* The file handler callback.  This reads from the internal pipe, and
-   then processes the Python event queue.  This will always be run in
-   the main gdb thread.  */
-
-static void
-gdbpy_run_events (int error, gdb_client_data client_data)
-{
-  gdbpy_enter enter_py (get_current_arch (), current_language);
-
-  /* Clear the event fd.  Do this before flushing the events list, so
-     that any new event post afterwards is sure to re-awake the event
-     loop.  */
-  serial_event_clear (gdbpy_serial_event);
-
-  while (gdbpy_event_list)
-    {
-      /* Dispatching the event might push a new element onto the event
-	 loop, so we update here "atomically enough".  */
-      struct gdbpy_event *item = gdbpy_event_list;
-      gdbpy_event_list = gdbpy_event_list->next;
-      if (gdbpy_event_list == NULL)
-	gdbpy_event_list_end = &gdbpy_event_list;
-
-      gdbpy_ref<> call_result (PyObject_CallObject (item->event, NULL));
-      if (call_result == NULL)
-	gdbpy_print_stack ();
-
-      Py_DECREF (item->event);
-      xfree (item);
-    }
-}
-
 /* Submit an event to the gdb thread.  */
 static PyObject *
 gdbpy_post_event (PyObject *self, PyObject *args)
 {
-  struct gdbpy_event *event;
   PyObject *func;
-  int wakeup;
 
   if (!PyArg_ParseTuple (args, "O", &func))
     return NULL;
@@ -1008,38 +1025,13 @@
       return NULL;
     }
 
-  Py_INCREF (func);
-
-  /* From here until the end of the function, we have the GIL, so we
-     can operate on our global data structures without worrying.  */
-  wakeup = gdbpy_event_list == NULL;
-
-  event = XNEW (struct gdbpy_event);
-  event->event = func;
-  event->next = NULL;
-  *gdbpy_event_list_end = event;
-  gdbpy_event_list_end = &event->next;
-
-  /* Wake up gdb when needed.  */
-  if (wakeup)
-    serial_event_set (gdbpy_serial_event);
+  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);
+  gdbpy_event event (std::move (func_ref));
+  run_on_main_thread (event);
 
   Py_RETURN_NONE;
 }
 
-/* Initialize the Python event handler.  */
-static int
-gdbpy_initialize_events (void)
-{
-  gdbpy_event_list_end = &gdbpy_event_list;
-
-  gdbpy_serial_event = make_serial_event ();
-  add_file_handler (serial_event_fd (gdbpy_serial_event),
-		    gdbpy_run_events, NULL);
-
-  return 0;
-}
-
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -1704,7 +1696,6 @@
       || gdbpy_initialize_linetable () < 0
       || gdbpy_initialize_thread () < 0
       || gdbpy_initialize_inferior () < 0
-      || gdbpy_initialize_events () < 0
       || gdbpy_initialize_eventregistry () < 0
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
Gerrit-Change-Number: 175
Gerrit-PatchSet: 5
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged

[pushed] Use run_on_main_thread in gdb.post_event

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Tom Tromey.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/175
......................................................................

Use run_on_main_thread in gdb.post_event

This changes gdb.post_event to use the new run_on_main_thread
function.  This is somewhat tricky because the Python GIL must be held
while manipulating reference counts.

2019-11-26  Tom Tromey  <tom@tromey.com>

	* python/python.c (class gdbpy_gil): New.
	(struct gdbpy_event): Add constructor, destructor, operator().
	(gdbpy_post_event): Use run_on_main_thread.
	(gdbpy_initialize_events): Remove.
	(do_start_initialization): Update.

Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
---
M gdb/ChangeLog
M gdb/python/python.c
2 files changed, 77 insertions(+), 78 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 289cbce..0c5aab2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-11-26  Tom Tromey  <tom@tromey.com>
 
+	* python/python.c (class gdbpy_gil): New.
+	(struct gdbpy_event): Add constructor, destructor, operator().
+	(gdbpy_post_event): Use run_on_main_thread.
+	(gdbpy_initialize_events): Remove.
+	(do_start_initialization): Update.
+
+2019-11-26  Tom Tromey  <tom@tromey.com>
+
 	* NEWS: Add entry.
 	* maint.c (_initialize_maint_cmds): Add "worker-threads" maint
 	commands.  Call update_thread_pool_size.
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 1cc5ebb..2f4fb0f 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -28,14 +28,13 @@
 #include "value.h"
 #include "language.h"
 #include "event-loop.h"
-#include "serial.h"
 #include "readline/tilde.h"
 #include "python.h"
 #include "extension-priv.h"
 #include "cli/cli-utils.h"
 #include <ctype.h>
 #include "location.h"
-#include "ser-event.h"
+#include "run-on-main-thread.h"
 
 /* Declared constants and enum for python stack printing.  */
 static const char python_excp_none[] = "none";
@@ -940,63 +939,81 @@
 
 /* Posting and handling events.  */
 
+/* A helper class to save and restore the GIL, but without touching
+   the other globals that are handled by gdbpy_enter.  */
+
+class gdbpy_gil
+{
+public:
+
+  gdbpy_gil ()
+    : m_state (PyGILState_Ensure ())
+  {
+  }
+
+  ~gdbpy_gil ()
+  {
+    PyGILState_Release (m_state);
+  }
+
+  DISABLE_COPY_AND_ASSIGN (gdbpy_gil);
+
+private:
+
+  PyGILState_STATE m_state;
+};
+
 /* A single event.  */
 struct gdbpy_event
 {
-  /* The Python event.  This is just a callable object.  */
-  PyObject *event;
-  /* The next event.  */
-  struct gdbpy_event *next;
+  gdbpy_event (gdbpy_ref<> &&func)
+    : m_func (func.release ())
+  {
+  }
+
+  gdbpy_event (gdbpy_event &&other)
+    : m_func (other.m_func)
+  {
+    other.m_func = nullptr;
+  }
+
+  gdbpy_event (const gdbpy_event &other)
+    : m_func (other.m_func)
+  {
+    gdbpy_gil gil;
+    Py_XINCREF (m_func);
+  }
+
+  ~gdbpy_event ()
+  {
+    gdbpy_gil gil;
+    Py_XDECREF (m_func);
+  }
+
+  gdbpy_event &operator= (const gdbpy_event &other) = delete;
+
+  void operator() ()
+  {
+    gdbpy_enter enter_py (get_current_arch (), current_language);
+
+    gdbpy_ref<> call_result (PyObject_CallObject (m_func, NULL));
+    if (call_result == NULL)
+      gdbpy_print_stack ();
+  }
+
+private:
+
+  /* The Python event.  This is just a callable object.  Note that
+     this is not a gdbpy_ref<>, because we have to take particular
+     care to only destroy the reference when holding the GIL. */
+  PyObject *m_func;
 };
 
-/* All pending events.  */
-static struct gdbpy_event *gdbpy_event_list;
-/* The final link of the event list.  */
-static struct gdbpy_event **gdbpy_event_list_end;
-
-/* So that we can wake up the main thread even when it is blocked in
-   poll().  */
-static struct serial_event *gdbpy_serial_event;
-
-/* The file handler callback.  This reads from the internal pipe, and
-   then processes the Python event queue.  This will always be run in
-   the main gdb thread.  */
-
-static void
-gdbpy_run_events (int error, gdb_client_data client_data)
-{
-  gdbpy_enter enter_py (get_current_arch (), current_language);
-
-  /* Clear the event fd.  Do this before flushing the events list, so
-     that any new event post afterwards is sure to re-awake the event
-     loop.  */
-  serial_event_clear (gdbpy_serial_event);
-
-  while (gdbpy_event_list)
-    {
-      /* Dispatching the event might push a new element onto the event
-	 loop, so we update here "atomically enough".  */
-      struct gdbpy_event *item = gdbpy_event_list;
-      gdbpy_event_list = gdbpy_event_list->next;
-      if (gdbpy_event_list == NULL)
-	gdbpy_event_list_end = &gdbpy_event_list;
-
-      gdbpy_ref<> call_result (PyObject_CallObject (item->event, NULL));
-      if (call_result == NULL)
-	gdbpy_print_stack ();
-
-      Py_DECREF (item->event);
-      xfree (item);
-    }
-}
-
 /* Submit an event to the gdb thread.  */
 static PyObject *
 gdbpy_post_event (PyObject *self, PyObject *args)
 {
-  struct gdbpy_event *event;
   PyObject *func;
-  int wakeup;
 
   if (!PyArg_ParseTuple (args, "O", &func))
     return NULL;
@@ -1008,38 +1025,13 @@
       return NULL;
     }
 
-  Py_INCREF (func);
-
-  /* From here until the end of the function, we have the GIL, so we
-     can operate on our global data structures without worrying.  */
-  wakeup = gdbpy_event_list == NULL;
-
-  event = XNEW (struct gdbpy_event);
-  event->event = func;
-  event->next = NULL;
-  *gdbpy_event_list_end = event;
-  gdbpy_event_list_end = &event->next;
-
-  /* Wake up gdb when needed.  */
-  if (wakeup)
-    serial_event_set (gdbpy_serial_event);
+  gdbpy_ref<> func_ref = gdbpy_ref<>::new_reference (func);
+  gdbpy_event event (std::move (func_ref));
+  run_on_main_thread (event);
 
   Py_RETURN_NONE;
 }
 
-/* Initialize the Python event handler.  */
-static int
-gdbpy_initialize_events (void)
-{
-  gdbpy_event_list_end = &gdbpy_event_list;
-
-  gdbpy_serial_event = make_serial_event ();
-  add_file_handler (serial_event_fd (gdbpy_serial_event),
-		    gdbpy_run_events, NULL);
-
-  return 0;
-}
-
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -1704,7 +1696,6 @@
       || gdbpy_initialize_linetable () < 0
       || gdbpy_initialize_thread () < 0
       || gdbpy_initialize_inferior () < 0
-      || gdbpy_initialize_events () < 0
       || gdbpy_initialize_eventregistry () < 0
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ie4431e60f328dae48bd96b6c6a8e875e70bda1de
Gerrit-Change-Number: 175
Gerrit-PatchSet: 5
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset