Re: [PATCH] Replace deprecated_target_wait_hook by observers

[Tom Tromey <tom@tromey.com>]

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