[PATCH] Replace deprecated_target_wait_hook by observers

[PATCH] Replace deprecated_target_wait_hook by observers

[patrick]

From: Patrick Monnerat <patrick@monnerat.net>

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 two
target_wait observers:

target_pre_wait (ptid_t ptid)
target_post_wait (ptid_t event_ptid)

Upon target_wait entry, target_pre_wait is notified with the ptid
passed to target_wait. Upon exit, target_post_wait is notified with
the event ptid returned by target_wait. Should an exception occur,
event_ptid is null_ptid.

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 |  2 ++
 gdb/observable.h |  6 ++++++
 gdb/target.c     | 14 +++++++++++++-
 gdb/top.c        |  7 -------
 7 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e3c1db73749..bc6521c8ec6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -367,7 +367,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;
@@ -3515,7 +3515,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
@@ -3630,12 +3629,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
@@ -4591,10 +4585,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 3e84805accb..1209b4188e9 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 a475d8790c9..b8df3d781e7 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -357,7 +357,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 78f315793b6..afe81394594 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -79,6 +79,8 @@ DEFINE_OBSERVABLE (styling_changed);
 DEFINE_OBSERVABLE (current_source_symtab_and_line_changed);
 DEFINE_OBSERVABLE (gdb_exiting);
 DEFINE_OBSERVABLE (connection_removed);
+DEFINE_OBSERVABLE (target_pre_wait);
+DEFINE_OBSERVABLE (target_post_wait);
 
 } /* namespace observers */
 } /* namespace gdb */
diff --git a/gdb/observable.h b/gdb/observable.h
index 0cdf4767f04..f426c1a761f 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -256,6 +256,12 @@ extern observable<int> gdb_exiting;
 /* When a connection is removed.  */
 extern observable<process_stratum_target */* target */> connection_removed;
 
+/* About to enter target_wait (). */
+extern observable <ptid_t /* ptid */> target_pre_wait;
+
+/* About to leave target_wait (). */
+extern observable <ptid_t /* event_ptid */> target_post_wait;
+
 } /* namespace observers */
 
 } /* namespace gdb */
diff --git a/gdb/target.c b/gdb/target.c
index 658698b4e2b..e3df9275c9a 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"
@@ -2609,7 +2610,18 @@ target_wait (ptid_t ptid, struct target_waitstatus *status,
   if (!target_can_async_p (target))
     gdb_assert ((options & TARGET_WNOHANG) == 0);
 
-  return target->wait (ptid, status, options);
+  try
+    {
+      gdb::observers::target_pre_wait.notify (ptid);
+      ptid_t event_ptid = target->wait (ptid, status, options);
+      gdb::observers::target_post_wait.notify (event_ptid);
+      return event_ptid;
+    }
+  catch (...)
+    {
+      gdb::observers::target_post_wait.notify (null_ptid);
+      throw;
+    }
 }
 
 /* See target.h.  */
diff --git a/gdb/top.c b/gdb/top.c
index a94ed5cebdb..7831b4f96ca 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -247,13 +247,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.35.1

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Tom Tromey]

> From: Patrick Monnerat <patrick@monnerat.net>
> 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.

Thanks, I'm pushing this now.

Tom

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Patrick Monnerat]

On 3/14/22 14:53, Tom Tromey wrote:
>> From: Patrick Monnerat <patrick@monnerat.net>
>> 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.
> Thanks, I'm pushing this now.

Works perfectly with Insight.

Thanks for your action.

Regards,

Patrick

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Tom Tromey]

We discussed this a bit on irc but I thought I'd move the discussion
here.

Simon> I started experimenting with this, asserting if an observer throws:
Simon> https://review.lttng.org/c/binutils-gdb/+/6527
Simon> But I am seeing some failures.  As long as we are not enforcing this, i
Simon> would favor the safer approach.

I tried this too, using:

diff --git a/gdbsupport/observable.h b/gdbsupport/observable.h
index 8ed56612ad0..5eccbed48bc 100644
--- a/gdbsupport/observable.h
+++ b/gdbsupport/observable.h
@@ -139,7 +139,7 @@ class observable
   }
 
   /* Notify all observers that are attached to this observable.  */
-  void notify (T... args) const
+  void notify (T... args) const noexcept
   {
     OBSERVER_SCOPED_DEBUG_START_END ("observable %s notify() called", m_name);
 

Then I ran the test suite.  I found a few failures this way, and I
tracked most of them down to get_frame_address_in_block_if_available.
This hack fixed those:

diff --git a/gdb/frame.c b/gdb/frame.c
index b121892f799..2ec3c161529 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2668,9 +2668,7 @@ get_frame_address_in_block_if_available (frame_info *this_frame,
     }
   catch (const gdb_exception_error &ex)
     {
-      if (ex.error == NOT_AVAILABLE_ERROR)
-	return false;
-      throw;
+      return false;
     }
 
   return true;


However, I still get this one:

    terminate called after throwing an instance of 'gdb_exception_quit'
    [...]
    0xba17eb tui_on_normal_stop
            ../../binutils-gdb/gdb/tui/tui-interp.c:99
    [...]
    0x804b10 _ZNK3gdb9observers10observableIJP7bpstatsiEE6notifyES3_i
            ../../binutils-gdb/gdb/../gdbsupport/observable.h:150
    0x7ffee6 _Z11normal_stopv
            ../../binutils-gdb/gdb/infrun.c:8602

This one is disturbing to me, because it is a C-c that's being handled
by an observer.

As I said on irc, my mental model for observers is that they are a "veil
of ignorance" way for one module to subscribe to state changes from
another module.  They shouldn't normally be "slow" or block -- they
should not require interruption.  Furthermore, each observer should be
independent (notwithstanding the dependency scheme -- in most cases this
isn't applicable).

This latter requirement explains why an observer should not throw an
exception: if one does, then different orders of observer will have
different effects in gdb, and anyway this would make an observer an
unreliable thing.  But if they are unreliable, then that's very bad,
because it means they're unusable for things like cache flushing.

So one idea here is to suppress quits when notifying an observer.
(Or, say, suppressing quits in tui_on_normal_stop at least.)


However, I looked very briefly and found this in remote.c:

  /* Hook into new objfile notification.  */
  gdb::observers::new_objfile.attach (remote_new_objfile, "remote");

The implementation of this observer sends packets to the remote (via
remote_check_symbols)... surely this is the kind of thing that ought to
be interruptible.  (I don't know if it is right now.)


So I'm not really sure what route is best.  One idea would be to ignore
and log ordinary exceptions from observers, and then re-throw on a quit.
I'm not confident this will be correct though.

Tom

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Simon Marchi]

On 2021-09-23 16:10, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> The only dangerous things about this is if an observer throws an
> Simon> exception, an exception will exit the destructor, and that aborts the
> Simon> process.
> 
> I think observers generally aren't supposed to throw.
> 
> If we want gdb to robust against this, I think the thing to do would be
> add a try/catch in observable::notify.
> 
> Tom
> 

I started experimenting with this, asserting if an observer throws:

https://review.lttng.org/c/binutils-gdb/+/6527

But I am seeing some failures.  As long as we are not enforcing this, i
would favor the safer approach.

Simon

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> The only dangerous things about this is if an observer throws an
Simon> exception, an exception will exit the destructor, and that aborts the
Simon> process.

I think observers generally aren't supposed to throw.

If we want gdb to robust against this, I think the thing to do would be
add a try/catch in observable::notify.

Tom

Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Simon Marchi]

> diff --git a/gdb/target.c b/gdb/target.c
> index ae2d659583e..22e58484a18 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"
> @@ -2591,12 +2592,45 @@ target_disconnect (const char *args, int from_tty)
>    current_inferior ()->top_target ()->disconnect (args, from_tty);
>  }
>  
> +/* RAII class to handle target_wait observers.
> +   These observers allow the Insight GUI to monitor user interaction while
> +   waiting on the target. */
> +
> +class scoped_target_wait
> +{
> +public:
> +  explicit scoped_target_wait (ptid_t ptid)
> +  {
> +    m_event_ptid = null_ptid;
> +    gdb::observers::target_pre_wait.notify (ptid);
> +  }
> +
> +  ~scoped_target_wait ()
> +  {
> +    gdb::observers::target_post_wait.notify (m_event_ptid);

The only dangerous things about this is if an observer throws an
exception, an exception will exit the destructor, and that aborts the
process.

Maybe something safer in that regard would be:

try
  {
    gdb::observers::target_pre_wait.notify (ptid);
    ptid_t event_ptid = target_wait (...);
    gdb::observers::target_post_wait.notify (event_ptid);
    return event_ptid;
  }
catch
  {
    gdb::observers::target_post_wait.notify (null_ptid);
    throw;
  }

Simon

[PATCH] Replace deprecated_target_wait_hook by observers

[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 two
target_wait observers:

target_pre_wait (ptid_t ptid)
target_post_wait (ptid_t event_ptid)

Upon target_wait entry, target_pre_wait is notified with the ptid
passed to target_wait. Upon exit, target_post_wait is notified with
the event ptid returned by target_wait. Should an exception occur,
event_ptid is null_ptid.

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 |  2 ++
 gdb/observable.h |  6 ++++++
 gdb/target.c     | 37 ++++++++++++++++++++++++++++++++++++-
 gdb/top.c        |  7 -------
 7 files changed, 49 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..5724b712604 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -77,6 +77,8 @@ 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 (target_pre_wait);
+DEFINE_OBSERVABLE (target_post_wait);
 
 } /* namespace observers */
 } /* namespace gdb */
diff --git a/gdb/observable.h b/gdb/observable.h
index 915770ff363..652fb3df43f 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -251,6 +251,12 @@ extern observable<> source_styling_changed;
 
 extern observable<> current_source_symtab_and_line_changed;
 
+/* About to enter target_wait (). */
+extern observable <ptid_t /* ptid */> target_pre_wait;
+
+/* About to leave target_wait (). */
+extern observable <ptid_t /* event_ptid */> target_post_wait;
+
 } /* namespace observers */
 
 } /* namespace gdb */
diff --git a/gdb/target.c b/gdb/target.c
index ae2d659583e..22e58484a18 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"
@@ -2591,12 +2592,45 @@ target_disconnect (const char *args, int from_tty)
   current_inferior ()->top_target ()->disconnect (args, from_tty);
 }
 
+/* RAII class to handle target_wait observers.
+   These observers allow the Insight GUI to monitor user interaction while
+   waiting on the target. */
+
+class scoped_target_wait
+{
+public:
+  explicit scoped_target_wait (ptid_t ptid)
+  {
+    m_event_ptid = null_ptid;
+    gdb::observers::target_pre_wait.notify (ptid);
+  }
+
+  ~scoped_target_wait ()
+  {
+    gdb::observers::target_post_wait.notify (m_event_ptid);
+  }
+
+  void set (ptid_t event_ptid) noexcept
+  {
+    m_event_ptid = event_ptid;
+  }
+
+  ptid_t get () const noexcept
+  {
+    return m_event_ptid;
+  }
+
+private:
+  ptid_t m_event_ptid;
+};
+
 /* See target/target.h.  */
 
 ptid_t
 target_wait (ptid_t ptid, struct target_waitstatus *status,
 	     target_wait_flags options)
 {
+  scoped_target_wait event_ptid(ptid);
   target_ops *target = current_inferior ()->top_target ();
   process_stratum_target *proc_target = current_inferior ()->process_target ();
 
@@ -2605,7 +2639,8 @@ target_wait (ptid_t ptid, struct target_waitstatus *status,
   if (!target->can_async_p ())
     gdb_assert ((options & TARGET_WNOHANG) == 0);
 
-  return target->wait (ptid, status, options);
+  event_ptid.set (target->wait (ptid, status, options));
+  return event_ptid.get ();
 }
 
 /* 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