Patchwork [v3,5/8] Introduce run_on_main_thread

login
register
mail settings
Submitter Tom Tromey
Date May 29, 2019, 9:29 p.m.
Message ID <20190529212916.23721-6-tom@tromey.com>
Download mbox | patch
Permalink /patch/32907/
State New
Headers show

Comments

Tom Tromey - May 29, 2019, 9:29 p.m.
This introduces a way for a callback to be run on the main thread.

gdb/ChangeLog
2019-05-29  Tom Tromey  <tom@tromey.com>

	* ser-event.h (run_on_main_thread): Declare.
	* ser-event.c (runnable_event, runnables, runnable_mutex): New
	globals.
	(run_events, run_on_main_thread, _initialize_ser_event): New
	functions.
---
 gdb/ChangeLog   |  8 ++++++
 gdb/ser-event.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/ser-event.h |  6 +++++
 3 files changed, 83 insertions(+)
Pedro Alves - May 30, 2019, 1:12 p.m.
On 5/29/19 10:29 PM, Tom Tromey wrote:
> This introduces a way for a callback to be run on the main thread.

Can Python's gdb.post_event be built on top of this?
It would fix that nasty "atomically enough" race in gdbpy_run_events,
I guess.

> 
> gdb/ChangeLog
> 2019-05-29  Tom Tromey  <tom@tromey.com>
> 
> 	* ser-event.h (run_on_main_thread): Declare.
> 	* ser-event.c (runnable_event, runnables, runnable_mutex): New
> 	globals.
> 	(run_events, run_on_main_thread, _initialize_ser_event): New
> 	functions.
> ---
>  gdb/ChangeLog   |  8 ++++++
>  gdb/ser-event.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/ser-event.h |  6 +++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/gdb/ser-event.c b/gdb/ser-event.c
> index d3956346246..4870433b287 100644
> --- a/gdb/ser-event.c
> +++ b/gdb/ser-event.c
> @@ -20,6 +20,10 @@
>  #include "ser-event.h"
>  #include "serial.h"
>  #include "common/filestuff.h"
> +#if CXX_STD_THREAD
> +#include <mutex>
> +#endif
> +#include "event-loop.h"
>  
>  /* On POSIX hosts, a serial_event is basically an abstraction for the
>     classical self-pipe trick.
> @@ -217,3 +221,68 @@ serial_event_clear (struct serial_event *event)
>    ResetEvent (state->event);
>  #endif
>  }
> +
> +
> +
> +/* The serial event used when posting runnables.  */
> +
> +static struct serial_event *runnable_event;
> +
> +/* Runnables that have been posted.  */
> +
> +static std::vector<std::function<void ()>> runnables;
> +
> +#if CXX_STD_THREAD
> +
> +/* Mutex to hold when handling runnable_event or runnables.  */
> +
> +static std::mutex runnable_mutex;
> +
> +#endif
> +
> +/* Run all the queued runnables.  */
> +
> +static void
> +run_events (int error, gdb_client_data client_data)
> +{
> +  std::vector<std::function<void ()>> local;
> +
> +  /* Hold the lock while changing the globals, but not while running
> +     the runnables.  */
> +  {
> +#if CXX_STD_THREAD
> +    std::lock_guard<std::mutex> lock (runnable_mutex);
> +#endif
> +
> +    /* Clear the event fd.  Do this before flushing the events list,
> +       so that any new event post afterwards is sure to re-awaken the
> +       event loop.  */
> +    serial_event_clear (runnable_event);
> +
> +    /* Move the vector in case running a runnable pushes a new
> +       runnable.  */
> +    std::swap (local, runnables);
> +  }
> +
> +  for (auto &item : local)
> +    item ();

I'd think this should swallow errors when calling
each item, instead of letting an exception escape and
discard all other items, since each item call should be
in principle logically unrelated?

Maybe we could unit test this code.

Thanks,
Pedro Alves
Tom Tromey - May 30, 2019, 1:20 p.m.
Pedro> Can Python's gdb.post_event be built on top of this?
Pedro> It would fix that nasty "atomically enough" race in gdbpy_run_events,
Pedro> I guess.

Yes, and actually I wrote a patch to do this once, but I dropped it from
this series because it was unrelated.

>> +  for (auto &item : local)
>> +    item ();

Pedro> I'd think this should swallow errors when calling
Pedro> each item, instead of letting an exception escape and
Pedro> discard all other items, since each item call should be
Pedro> in principle logically unrelated?

Pedro> Maybe we could unit test this code.

I wasn't sure how.  But for this maybe the callback should be noexcept.

Tom
Pedro Alves - May 30, 2019, 1:56 p.m.
On 5/30/19 2:20 PM, Tom Tromey wrote:
> Pedro> Can Python's gdb.post_event be built on top of this?
> Pedro> It would fix that nasty "atomically enough" race in gdbpy_run_events,
> Pedro> I guess.
> 
> Yes, and actually I wrote a patch to do this once, but I dropped it from
> this series because it was unrelated.
> 
>>> +  for (auto &item : local)
>>> +    item ();
> 
> Pedro> I'd think this should swallow errors when calling
> Pedro> each item, instead of letting an exception escape and
> Pedro> discard all other items, since each item call should be
> Pedro> in principle logically unrelated?
> 
> Pedro> Maybe we could unit test this code.
> 
> I wasn't sure how.

In a unit test, the main thread code could spawn a thread that calls
run_on_main_thread.  The main thread would then nest an event loop
to run the main-thread events.
The posted event would write to a global that would unlock the nested
event loop.  Something similar to wait_sync_command_done.

>  But for this maybe the callback should be noexcept.

Hmm, maybe.  But then if someone forgets to try/catch, we end up
aborting gdb.  Pick what you prefer, as long as we decide and document
something.

Thanks,
Pedro Alves
Pedro Alves - May 30, 2019, 2 p.m.
On 5/30/19 2:20 PM, Tom Tromey wrote:
> Pedro> Can Python's gdb.post_event be built on top of this?
> Pedro> It would fix that nasty "atomically enough" race in gdbpy_run_events,
> Pedro> I guess.
> 
> Yes, and actually I wrote a patch to do this once, but I dropped it from
> this series because it was unrelated.

Forgot to say: I wouldn't call it unrelated.  You could use it as yet-another
proof that run_on_main_thread is a useful abstraction, which even fixes
a race/bug.  I'd be happy to see such a patch.  ( but no rush :-) )

Thanks,
Pedro Alves

Patch

diff --git a/gdb/ser-event.c b/gdb/ser-event.c
index d3956346246..4870433b287 100644
--- a/gdb/ser-event.c
+++ b/gdb/ser-event.c
@@ -20,6 +20,10 @@ 
 #include "ser-event.h"
 #include "serial.h"
 #include "common/filestuff.h"
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
+#include "event-loop.h"
 
 /* On POSIX hosts, a serial_event is basically an abstraction for the
    classical self-pipe trick.
@@ -217,3 +221,68 @@  serial_event_clear (struct serial_event *event)
   ResetEvent (state->event);
 #endif
 }
+
+
+
+/* The serial event used when posting runnables.  */
+
+static struct serial_event *runnable_event;
+
+/* Runnables that have been posted.  */
+
+static std::vector<std::function<void ()>> runnables;
+
+#if CXX_STD_THREAD
+
+/* Mutex to hold when handling runnable_event or runnables.  */
+
+static std::mutex runnable_mutex;
+
+#endif
+
+/* Run all the queued runnables.  */
+
+static void
+run_events (int error, gdb_client_data client_data)
+{
+  std::vector<std::function<void ()>> local;
+
+  /* Hold the lock while changing the globals, but not while running
+     the runnables.  */
+  {
+#if CXX_STD_THREAD
+    std::lock_guard<std::mutex> lock (runnable_mutex);
+#endif
+
+    /* Clear the event fd.  Do this before flushing the events list,
+       so that any new event post afterwards is sure to re-awaken the
+       event loop.  */
+    serial_event_clear (runnable_event);
+
+    /* Move the vector in case running a runnable pushes a new
+       runnable.  */
+    std::swap (local, runnables);
+  }
+
+  for (auto &item : local)
+    item ();
+}
+
+/* See ser-event.h.  */
+
+void
+run_on_main_thread (std::function<void ()> &&func)
+{
+#if CXX_STD_THREAD
+  std::lock_guard<std::mutex> lock (runnable_mutex);
+#endif
+  runnables.emplace_back (std::move (func));
+  serial_event_set (runnable_event);
+}
+
+void
+_initialize_ser_event ()
+{
+  runnable_event = make_serial_event ();
+  add_file_handler (serial_event_fd (runnable_event), run_events, nullptr);
+}
diff --git a/gdb/ser-event.h b/gdb/ser-event.h
index 137348557f9..61a84f9cc79 100644
--- a/gdb/ser-event.h
+++ b/gdb/ser-event.h
@@ -19,6 +19,8 @@ 
 #ifndef SER_EVENT_H
 #define SER_EVENT_H
 
+#include <functional>
+
 /* This is used to be able to signal the event loop (or any other
    select/poll) of events, in a race-free manner.
 
@@ -48,4 +50,8 @@  extern void serial_event_set (struct serial_event *event);
    call is made.  */
 extern void serial_event_clear (struct serial_event *event);
 
+/* Send a runnable to the main thread.  */
+
+extern void run_on_main_thread (std::function<void ()> &&);
+
 #endif