[v3,5/8] Introduce run_on_main_thread

  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(+)
  

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
  

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
  

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
  

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