[PATCH] Replace deprecated_target_wait_hook by an observer

[PATCH] Replace deprecated_target_wait_hook by an observer

[Patrick Monnerat]

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);
 
 } /* 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;
 }
 
 /* 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

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Simon Marchi]

On 2021-08-22 12:42 p.m., Patrick Monnerat via Gdb-patches wrote:
> 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.

Hi,

If that works for Insight (I presume it does, since you are sending this
patch), then this is a welcome change.  Can you show to the
corresponding change in Insight?  I'm curious to see how Insight uses
this.

But otherwise, the change in gdb looks good to me.

Simon

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Patrick Monnerat]

On 8/23/21 6:26 PM, Simon Marchi wrote:
> Can you show to the
> corresponding change in Insight?  I'm curious to see how Insight uses
> this.
>
Hi Simon,

Sure! Here is the important part of the Insight diff:

-----

--- a/gdbtk/generic/gdbtk-hooks.c
+++ b/gdbtk/generic/gdbtk-hooks.c

@@ -685,16 +685,19 @@ gdbtk_post_add_symbol (void)
  /* This hook function is called whenever we want to wait for the
     target.  */

-static ptid_t
-gdbtk_wait (ptid_t ptid, struct target_waitstatus *ourstatus, void 
*options)
+static void
+gdbtk_waiting_for_target (bool enter, ptid_t ptid)
  {
-  gdbtk_force_detach = 0;
-  gdbtk_start_timer ();
-  ptid = target_wait (ptid, ourstatus, *(target_wait_flags *) options);
-  gdbtk_stop_timer ();
-  gdbtk_ptid = ptid;
-
-  return ptid;
+  if (enter)
+    {
+      gdbtk_force_detach = 0;
+      gdbtk_start_timer ();
+    }
+  else
+    {
+      gdbtk_stop_timer ();
+      gdbtk_ptid = ptid;
+    }
  }

  /*
-----

For more details, see Insight commit 
https://sourceware.org/git/?p=insight.git;a=commit;h=16042bf


Patrick

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Simon Marchi]

On 2021-08-23 1:36 p.m., Patrick Monnerat wrote:
> 
> On 8/23/21 6:26 PM, Simon Marchi wrote:
>> Can you show to the
>> corresponding change in Insight?  I'm curious to see how Insight uses
>> this.
>>
> Hi Simon,
> 
> Sure! Here is the important part of the Insight diff:
> 
> -----
> 
> --- a/gdbtk/generic/gdbtk-hooks.c
> +++ b/gdbtk/generic/gdbtk-hooks.c
> 
> @@ -685,16 +685,19 @@ gdbtk_post_add_symbol (void)
>  /* This hook function is called whenever we want to wait for the
>     target.  */
> 
> -static ptid_t
> -gdbtk_wait (ptid_t ptid, struct target_waitstatus *ourstatus, void *options)
> +static void
> +gdbtk_waiting_for_target (bool enter, ptid_t ptid)
>  {
> -  gdbtk_force_detach = 0;
> -  gdbtk_start_timer ();
> -  ptid = target_wait (ptid, ourstatus, *(target_wait_flags *) options);
> -  gdbtk_stop_timer ();
> -  gdbtk_ptid = ptid;
> -
> -  return ptid;
> +  if (enter)
> +    {
> +      gdbtk_force_detach = 0;
> +      gdbtk_start_timer ();
> +    }
> +  else
> +    {
> +      gdbtk_stop_timer ();
> +      gdbtk_ptid = ptid;
> +    }
>  }
> 
>  /*
> -----
> 
> For more details, see Insight commit https://sourceware.org/git/?p=insight.git;a=commit;h=16042bf

Thanks, that clarifies why you need it to be called both before and
after.

I think it would be good to add some comment above where the obsserver
is notified to say that this is used by Insight, otherwise someone might
be tempted to remove it, as it is unused in the GDB project itself.

I suggest leaving the review up for a week, if nobody has objections,
I'll merge your patch (and I can add the comment myself).

Thanks,

Simon

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Patrick Monnerat]

On 8/23/21 7:48 PM, Simon Marchi wrote:
>> For more details, see Insight commit https://sourceware.org/git/?p=insight.git;a=commit;h=16042bf
> Thanks, that clarifies why you need it to be called both before and
> after.
>
> I think it would be good to add some comment above where the obsserver
> is notified to say that this is used by Insight, otherwise someone might
> be tempted to remove it, as it is unused in the GDB project itself.
>
> I suggest leaving the review up for a week, if nobody has objections,
> I'll merge your patch (and I can add the comment myself).

That's fine for me, thanks.

Patrick

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Andrew Burgess]

* 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
> 

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Patrick Monnerat]

On 8/24/21 6:14 PM, Andrew Burgess wrote:
> * Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> [2021-08-22 18:42:56 +0200]:
>
>> +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'?
>
Yes, it's possible. Here are the two reasons why I did not name this 
observer 'target_wait':

1) Because the observer is not supposed to wait by itself, I fear it 
will be a source of confusion.

2) As a good old C programmer, I still have some reluctance naming an 
object as a global procedure. Not being declared in the same namespace 
though!

Comments are welcome!

>>   
>> -  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.

That would make sense, but in case we have an exception, event_ptid is 
not known. How would you handle it? pass it as null_ptid?

Thanks for review,

Patrick

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Andrew Burgess]

* Patrick Monnerat <patrick@monnerat.net> [2021-08-25 15:30:28 +0200]:

> 
> On 8/24/21 6:14 PM, Andrew Burgess wrote:
> > * Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> [2021-08-22 18:42:56 +0200]:
> > 
> > > +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'?
> > 
> Yes, it's possible. Here are the two reasons why I did not name this
> observer 'target_wait':
> 
> 1) Because the observer is not supposed to wait by itself, I fear it will be
> a source of confusion.
> 
> 2) As a good old C programmer, I still have some reluctance naming an object
> as a global procedure. Not being declared in the same namespace though!
> 
> Comments are welcome!

How about 'target_waiting' then?  This seems more inline with the
existing naming, seems to indicate that the target _is_ waiting, not
the that observer _should_ wait (so avoiding #1), and is a new name
(so avoiding #2).

> 
> > > -  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.
> 
> That would make sense, but in case we have an exception, event_ptid is not
> known. How would you handle it? pass it as null_ptid?

Yeah I guess that would make sense.

Thanks,
Andrew

> 
> Thanks for review,
> 
> Patrick
> 

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Simon Marchi]

On 2021-08-25 12:19 p.m., Andrew Burgess wrote:
> * Patrick Monnerat <patrick@monnerat.net> [2021-08-25 15:30:28 +0200]:
> 
>>
>> On 8/24/21 6:14 PM, Andrew Burgess wrote:
>>> * Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> [2021-08-22 18:42:56 +0200]:
>>>
>>>> +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'?
>>>
>> Yes, it's possible. Here are the two reasons why I did not name this
>> observer 'target_wait':
>>
>> 1) Because the observer is not supposed to wait by itself, I fear it will be
>> a source of confusion.
>>
>> 2) As a good old C programmer, I still have some reluctance naming an object
>> as a global procedure. Not being declared in the same namespace though!
>>
>> Comments are welcome!
> 
> How about 'target_waiting' then?  This seems more inline with the
> existing naming, seems to indicate that the target _is_ waiting, not
> the that observer _should_ wait (so avoiding #1), and is a new name
> (so avoiding #2).

Oh, I have always seen it as the caller waiting for the target to
produce some event (in a context where everything was blocking / not
async... nowadays, with async, I always find using the term "wait" for
"fetch an event" confusing).  So the event would be
"target_waited_on".  Another way (it's not our current style so I
wouldn't choose that) would be to name all events "on something".  So
"on_target_wait".

Regardless, since we are debating this, I would suggest splitting the
observer in two actuall:

 - target_pre_wait (or target_wait_pre or whatever)
 - target_post_wait (or target_wait_post or whatever)

I think it's better to keep 1 observable == 1 event, rather than
having two events in one with a boolean to differentiate.

>> That would make sense, but in case we have an exception, event_ptid is not
>> known. How would you handle it? pass it as null_ptid?
> 
> Yeah I guess that would make sense.

I think that's ok, I don't think target_ops::wait implementations ever
return null_ptid (although I haven't checked, it's just from memory).

Simon

Re: [PATCH] Replace deprecated_target_wait_hook by an observer

[Patrick Monnerat]

On 8/26/21 4:41 AM, Simon Marchi wrote:
> On 2021-08-25 12:19 p.m., Andrew Burgess wrote:
>> * Patrick Monnerat <patrick@monnerat.net> [2021-08-25 15:30:28 +0200]:
>>
>>> On 8/24/21 6:14 PM, Andrew Burgess wrote:
>>>> * Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> [2021-08-22 18:42:56 +0200]:
>>>>
>>>>> +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'?
>>>>
>>> Yes, it's possible. Here are the two reasons why I did not name this
>>> observer 'target_wait':
>>>
>>> 1) Because the observer is not supposed to wait by itself, I fear it will be
>>> a source of confusion.
>>>
>>> 2) As a good old C programmer, I still have some reluctance naming an object
>>> as a global procedure. Not being declared in the same namespace though!
>>>
>>> Comments are welcome!
>> How about 'target_waiting' then?  This seems more inline with the
>> existing naming, seems to indicate that the target _is_ waiting, not
>> the that observer _should_ wait (so avoiding #1), and is a new name
>> (so avoiding #2).
> Oh, I have always seen it as the caller waiting for the target to
> produce some event (in a context where everything was blocking / not
> async... nowadays, with async, I always find using the term "wait" for
> "fetch an event" confusing).  So the event would be
> "target_waited_on".  Another way (it's not our current style so I
> wouldn't choose that) would be to name all events "on something".  So
> "on_target_wait".
>
> Regardless, since we are debating this, I would suggest splitting the
> observer in two actuall:
>
>   - target_pre_wait (or target_wait_pre or whatever)
>   - target_post_wait (or target_wait_post or whatever)
>
> I think it's better to keep 1 observable == 1 event, rather than
> having two events in one with a boolean to differentiate.

If it is not a luxury to have 2 observers for such a function, I think 
it's the best solution/naming.

>
>>> That would make sense, but in case we have an exception, event_ptid is not
>>> known. How would you handle it? pass it as null_ptid?
>> Yeah I guess that would make sense.
> I think that's ok, I don't think target_ops::wait implementations ever
> return null_ptid (although I haven't checked, it's just from memory).
>
An updated patch follows.

Patrick