* [review v2] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
@ 2019-10-20 21:02 ` Tom Tromey (Code Review)
2019-11-22 23:56 ` [review v4] " Tom Tromey (Code Review)
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-20 21:02 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [review v4] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
2019-10-20 21:02 ` [review v2] " Tom Tromey (Code Review)
@ 2019-11-22 23:56 ` Tom Tromey (Code Review)
2019-11-26 19:26 ` Pedro Alves (Code Review)
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Tom Tromey (Code Review) @ 2019-11-22 23:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [review v4] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
2019-10-20 21:02 ` [review v2] " Tom Tromey (Code Review)
2019-11-22 23:56 ` [review v4] " Tom Tromey (Code Review)
@ 2019-11-26 19:26 ` Pedro Alves (Code Review)
2019-11-26 19:53 ` Tom Tromey (Code Review)
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Pedro Alves (Code Review) @ 2019-11-26 19:26 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [review v4] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
` (2 preceding siblings ...)
2019-11-26 19:26 ` Pedro Alves (Code Review)
@ 2019-11-26 19:53 ` Tom Tromey (Code Review)
2019-11-26 21:14 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-11-26 21:21 ` Sourceware to Gerrit sync (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: Tom Tromey (Code Review) @ 2019-11-26 19:53 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pushed] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
` (3 preceding siblings ...)
2019-11-26 19:53 ` Tom Tromey (Code Review)
@ 2019-11-26 21:14 ` Sourceware to Gerrit sync (Code Review)
2019-11-26 21:21 ` Sourceware to Gerrit sync (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-26 21:14 UTC (permalink / raw)
To: Tom Tromey, gdb-patches; +Cc: Pedro Alves
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pushed] Use run_on_main_thread in gdb.post_event
2019-10-20 3:55 [review] Use run_on_main_thread in gdb.post_event Tom Tromey (Code Review)
` (4 preceding siblings ...)
2019-11-26 21:14 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-11-26 21:21 ` Sourceware to Gerrit sync (Code Review)
5 siblings, 0 replies; 7+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-26 21:21 UTC (permalink / raw)
To: Tom Tromey, Pedro Alves, gdb-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread