Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> [2021-08-22 18:42:56 +0200]:

> Commit b60cea7 (Make target_wait options use enum flags) broke
> deprecated_target_wait_hook usage: there's a commit comment telling
> this hook has not been converted.
> 
> Rather than trying to mend it, this patch replaces the hook by a
> target_wait observer:
> 
> waiting_for_target (bool entering, ptid_t ptid)
> 
> Upon target_wait entry, it is notified with entering=TRUE and ptid passed
> to target_wait. Upon exit, it is notified again with entering=FALSE and
> ptid = event ptid returned by target_wait.
> 
> This change benefits to Insight (out-of-tree): there's no real use of the
> late hook in gdb itself.
> ---
>  gdb/infrun.c     | 15 +++------------
>  gdb/infrun.h     |  5 ++---
>  gdb/interps.c    |  1 -
>  gdb/observable.c |  1 +
>  gdb/observable.h |  3 +++
>  gdb/target.c     |  7 ++++++-
>  gdb/top.c        |  7 -------
>  7 files changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 5ee650fa464..695e3b0a586 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -366,7 +366,7 @@ show_stop_on_solib_events (struct ui_file *file, int from_tty,
>  static bool stop_print_frame;
>  
>  /* This is a cached copy of the target/ptid/waitstatus of the last
> -   event returned by target_wait()/deprecated_target_wait_hook().
> +   event returned by target_wait().
>     This information is returned by get_last_target_status().  */
>  static process_stratum_target *target_last_proc_target;
>  static ptid_t target_last_wait_ptid;
> @@ -3478,7 +3478,6 @@ static ptid_t
>  do_target_wait_1 (inferior *inf, ptid_t ptid,
>  		  target_waitstatus *status, target_wait_flags options)
>  {
> -  ptid_t event_ptid;
>    struct thread_info *tp;
>  
>    /* We know that we are looking for an event in the target of inferior
> @@ -3594,12 +3593,7 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
>    if (!target_can_async_p ())
>      options &= ~TARGET_WNOHANG;
>  
> -  if (deprecated_target_wait_hook)
> -    event_ptid = deprecated_target_wait_hook (ptid, status, options);
> -  else
> -    event_ptid = target_wait (ptid, status, options);
> -
> -  return event_ptid;
> +  return target_wait (ptid, status, options);
>  }
>  
>  /* Wrapper for target_wait that first checks whether threads have
> @@ -4558,10 +4552,7 @@ poll_one_curr_target (struct target_waitstatus *ws)
>       don't get any event.  */
>    target_dcache_invalidate ();
>  
> -  if (deprecated_target_wait_hook)
> -    event_ptid = deprecated_target_wait_hook (minus_one_ptid, ws, TARGET_WNOHANG);
> -  else
> -    event_ptid = target_wait (minus_one_ptid, ws, TARGET_WNOHANG);
> +  event_ptid = target_wait (minus_one_ptid, ws, TARGET_WNOHANG);
>  
>    if (debug_infrun)
>      print_target_wait_results (minus_one_ptid, event_ptid, ws);
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index 5a577365f94..a14e20b5f94 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -124,9 +124,8 @@ extern process_stratum_target *user_visible_resume_target (ptid_t resume_ptid);
>  extern int normal_stop (void);
>  
>  /* Return the cached copy of the last target/ptid/waitstatus returned
> -   by target_wait()/deprecated_target_wait_hook().  The data is
> -   actually cached by handle_inferior_event(), which gets called
> -   immediately after target_wait()/deprecated_target_wait_hook().  */
> +   by target_wait().  The data is actually cached by handle_inferior_event(),
> +   which gets called immediately after target_wait().  */
>  extern void get_last_target_status (process_stratum_target **target,
>  				    ptid_t *ptid,
>  				    struct target_waitstatus *status);
> diff --git a/gdb/interps.c b/gdb/interps.c
> index ec19822b571..55bd10467c3 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -356,7 +356,6 @@ clear_interpreter_hooks (void)
>    deprecated_readline_hook = 0;
>    deprecated_readline_end_hook = 0;
>    deprecated_context_hook = 0;
> -  deprecated_target_wait_hook = 0;
>    deprecated_call_command_hook = 0;
>    deprecated_error_begin_hook = 0;
>  }
> diff --git a/gdb/observable.c b/gdb/observable.c
> index 51f5edb0a1f..08d45c77ea7 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -77,6 +77,7 @@ DEFINE_OBSERVABLE (register_changed);
>  DEFINE_OBSERVABLE (user_selected_context_changed);
>  DEFINE_OBSERVABLE (source_styling_changed);
>  DEFINE_OBSERVABLE (current_source_symtab_and_line_changed);
> +DEFINE_OBSERVABLE (waiting_for_target);

Given we already have events 'target_changed' and 'target_resumed', I
wonder if it would be more consistent to name this event 'target_wait'?

>  
>  } /* namespace observers */
>  } /* namespace gdb */
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 915770ff363..d61b5468c9e 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -251,6 +251,9 @@ extern observable<> source_styling_changed;
>  
>  extern observable<> current_source_symtab_and_line_changed;
>  
> +/* About to enter target_wait () or leave it. */
> +extern observable <bool /* entering */, ptid_t /* ptid */> waiting_for_target;
> +
>  } /* namespace observers */
>  
>  } /* namespace gdb */
> diff --git a/gdb/target.c b/gdb/target.c
> index ae2d659583e..df7c64e204f 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -26,6 +26,7 @@
>  #include "symtab.h"
>  #include "inferior.h"
>  #include "infrun.h"
> +#include "observable.h"
>  #include "bfd.h"
>  #include "symfile.h"
>  #include "objfiles.h"
> @@ -2599,13 +2600,17 @@ target_wait (ptid_t ptid, struct target_waitstatus *status,
>  {
>    target_ops *target = current_inferior ()->top_target ();
>    process_stratum_target *proc_target = current_inferior ()->process_target ();
> +  ptid_t event_ptid;
>  
>    gdb_assert (!proc_target->commit_resumed_state);
>  
>    if (!target->can_async_p ())
>      gdb_assert ((options & TARGET_WNOHANG) == 0);
>  
> -  return target->wait (ptid, status, options);
> +  gdb::observers::waiting_for_target.notify (true, ptid);
> +  event_ptid = target->wait (ptid, status, options);
> +  gdb::observers::waiting_for_target.notify (false, event_ptid);
> +  return event_ptid;

I would be tempted to wrap this notification inside an RAII class,
then we will be guaranteed to send the second notification, even in
the event that the wait call exits via an exception.

Thanks,
Andrew



>  }
>  
>  /* See target.h.  */
> diff --git a/gdb/top.c b/gdb/top.c
> index 0c49f4f9eb4..368166720ac 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -249,13 +249,6 @@ void (*deprecated_readline_end_hook) (void);
>  void (*deprecated_attach_hook) (void);
>  void (*deprecated_detach_hook) (void);
>  
> -/* Called when going to wait for the target.  Usually allows the GUI
> -   to run while waiting for target events.  */
> -
> -ptid_t (*deprecated_target_wait_hook) (ptid_t ptid,
> -				       struct target_waitstatus *status,
> -				       int options);
> -
>  /* Used by UI as a wrapper around command execution.  May do various
>     things like enabling/disabling buttons, etc...  */
>  
> -- 
> 2.31.1
>