Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

> Since commit b2d8657, having a per-interpreter event/command loop is not
> possible anymore.
>
> As Insight uses a GUI that has its own event loop, gdb and GUI event
> loops have then to be "merged" (i.e.: work together). But this is
> problematic as gdb_do_one_event is not aware of this alternate event
> loop and thus may wait forever.
>
> The solution is to implement a wait timeout to gdb_do_one_event. This
> cannot be done externally as gdb timers are event sources themselves.
>
> The new parameter defaults to "no timeout": as it is used by Insight
> only, there is no need to update calls from the gdb source tree.
> ---
>  gdbsupport/event-loop.cc | 45 +++++++++++++++++++++++++++++++---------
>  gdbsupport/event-loop.h  |  2 +-
>  2 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
> index 385b45b2de1..14168cae5a6 100644
> --- a/gdbsupport/event-loop.cc
> +++ b/gdbsupport/event-loop.cc
> @@ -33,6 +33,8 @@
>  #include <sys/types.h>
>  #include "gdbsupport/gdb_sys_time.h"
>  #include "gdbsupport/gdb_select.h"
> +#include "gdbsupport/gdb_optional.h"
> +#include "gdbsupport/scope-exit.h"
>  
>  /* See event-loop.h.  */
>  
> @@ -177,12 +179,17 @@ static int update_wait_timeout (void);
>  static int poll_timers (void);
>  \f
>  /* Process one high level event.  If nothing is ready at this time,
> -   wait for something to happen (via gdb_wait_for_event), then process
> -   it.  Returns >0 if something was done otherwise returns <0 (this
> -   can happen if there are no event sources to wait for).  */
> +   wait at most MSTIMEOUT milliseconds for something to happen (via
> +   gdb_wait_for_event), then process it.  Returns >0 if something was
> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
> +   A timeout of 0 allows to serve an already pending event, but does not
> +   wait if none found.
> +   Setting the timeout to a negative value disables it.
> +   The timeout is never used by gdb itself, it is however needed to
> +   integrate gdb event handling within Insight's GUI event loop. */
>  
>  int
> -gdb_do_one_event (void)
> +gdb_do_one_event (int mstimeout)
>  {
>    static int event_source_head = 0;
>    const int number_of_sources = 3;
> @@ -229,17 +236,35 @@ gdb_do_one_event (void)
>  	return 1;
>      }
>  
> +  if (!mstimeout)
> +    return 0;	/* Null timeout: do not wait for an event. */
> +

This should be:

  if (mstimeout == 0)

As mstimeout is an int, not a bool.

>    /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
>       we should get out because this means that there are no event
>       sources left.  This will make the event loop stop, and the
> -     application exit.  */
> +     application exit.
> +     If a timeout has been given, a new timer is set accordingly
> +     to abort event wait.  It is deleted upon gdb_wait_for_event
> +     termination and thus should never be triggered.
> +     When the timeout is reached, events are not monitored again:
> +     they already have been checked in the loop above. */
>  
> -  if (gdb_wait_for_event (1) < 0)
> -    return -1;
> +  gdb::optional<int> timer_id;
>  
> -  /* If gdb_wait_for_event has returned 1, it means that one event has
> -     been handled.  We break out of the loop.  */
> -  return 1;
> +  SCOPE_EXIT 
> +    {
> +      if (timer_id.has_value ())
> +	delete_timer (*timer_id);
> +    };
> +
> +  if (mstimeout > 0)
> +    timer_id = create_timer (mstimeout,
> +			     [] (gdb_client_data arg)
> +			     {
> +			       ((gdb::optional<int> *) arg)->reset ();
> +			     },
> +			     &timer_id);
> +  return gdb_wait_for_event (1);

More just a warning really, but this isn't going to work in all cases.
If a target doesn't support async-mode then GDB will block waiting for
events in the check_async_event_handlers call above, and never gets down
this far.

I'm interested in this because I also want to have the event loop run
under a timeout for a patch I'm working on, and everything works fine
except for the case when I run with async support disabled.

I'm currently investigating having non-async targets ask the event loop
for a maximum wait time before they block, then return to the event loop
in order to check timers.

If I can get this working, I'll want to move your create_timer call to
the start of gdb_do_one_event, so that the timer is in place before the
call to check_async_event_handlers - though I'm not quite sure how this
would be expected to interact with the case where 'mstimeout == 0'.

Thanks,
Andrew

>  }
>  
>  /* See event-loop.h  */
> diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
> index 9ed592877ab..c82493e9bdf 100644
> --- a/gdbsupport/event-loop.h
> +++ b/gdbsupport/event-loop.h
> @@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
>  
>  /* Exported functions from event-loop.c */
>  
> -extern int gdb_do_one_event (void);
> +extern int gdb_do_one_event (int mstimeout = -1);
>  extern void delete_file_handler (int fd);
>  
>  /* Add a file handler/descriptor to the list of descriptors we are
> -- 
> 2.35.1