Re: RFA: Remove gdb-events

[Joel Brobecker <brobecker at adacore dot com>]

Eli: if you could take a look at the documentation of the new observers,
that would be great! I'm going to approve the technical part, and we'll
follow any suggestion you might have either before or after commit.

Vladimir: This patch touches MI. Could you take a look at this part
of the patch? The changes seem fine to me, but see below my suggestion.

Hi Tom,

First of all, thanks very much for undertaking this task. I think
we have been talking of getting rid of hooks for at least since
the day we introduced the observers.

Here are my comments on the patch.

> * Removing the call to clear_gdb_event_hooks.  In theory I think this
>   could confuse some caller.  However, in practice I think it is
>   probably nothing to worry about.

We do have to be a little bit careful - because we don't want TUI
observers to trigger when TUI is inactive, or MI observers to trigger
when the interpreter is CLI. But I see that you definitely handled that.

My thought on this is that the observers should be installed permanently
and should have a way of determining whether the current interpreter is
the one they have been installed for before doing anything else. For
instance, inside MI, you'd have:

    current_interp_is_mi ()
    {
      return (current_interpreter () == ...
              || current_interpreter () == ...)
    }
    
    mi_breakpoint_notify (int b)
    {
      if (!current_interp_is_mi ())
        return;
      gdb_breakpoint_query (uiout, b, NULL)
    }

But, for now, the way you implemented things makes this unnecessary.
This should be discussed as a separate change, however, as I think
it would simplify things a bit inside mi and tui.

> * The architecture_changed observer is needed for gdbtk, but the only
>   place where this is notified is deprecated.  Since my goal here is
>   to remove events, I think this is probably fine.  It certainly
>   doesn't seem any worse.

The gdbtk team will probably be grateful that you took a look at their
code, but it's actually maintained by a different group, with a different
discussion list. I took a look, and the changes certainly look reasonable.
Let's send them a copy of the gdbtk patch (insight@sourceware.org) when
we're happy about the changes in GDB.

> * MI installs a new set of event handlers, does some work, then
>   uninstalls them.  Instead of trying to replicate this behavior --
>   say by deinstalling other observers -- I just made the MI observer
>   conditional on a global flag, which is set and reset at the
>   appropriate points.
> 
>   I think this is probably the friendliest approach; my thinking here
>   is that any given module should not be able to mess around with
>   observers installed by a separate module.

I would like Vladimir to review this part. It seems fine to me,
except that I would simply install the observer during the _init
phase instead of doing it on the fly with a static global used
to make sure they are installed only once.

Regarding the set/reset thing, see discussion above about interpreters
being able to know which interpreter is current.

> ChangeLog:
> 2008-07-12  Tom Tromey  <tromey@redhat.com>
> 
> 	* tui/tui-hooks.c: Include observer.h.
> 	(tui_event_default, tui_old_event_hooks, tui_event_hooks):
> 	Remove.
> 	(tui_bp_created_observer, tui_bp_deleted_observer,
> 	tui_bp_modified_observer): New globals.
> 	(tui_install_hooks): Use observers, not events.
> 	(tui_remove_hooks): Likewise.
> 	* mi/mi-cmd-break.c: Include observer.h, not gdb-events.h.
> 	(mi_breakpoint_observers_installed, mi_can_breakpoint_notify): New
> 	globals.
> 	(breakpoint_notify): Check mi_can_breakpoint_notify.
> 	(breakpoint_hooks): Remove.
> 	(mi_cmd_break_insert): Attach observers.  Don't use events.
> 	* tracepoint.c: Include observer.h, not gdb-events.h.
> 	(tracepoint_operation, trace_pass_command): Notify observer.
> 	* interps.c: Don't include gdb-events.h.
> 	(clear_interpreter_hooks): Don't call clear_gdb_event_hooks.
> 	* gdbarch.c: Include observer.h, not gdb-events.h.
> 	(deprecated_current_gdbarch_select_hack): Notify observer.
> 	* breakpoint.h: Don't include gdb-events.h.
> 	* breakpoint.c: Don't include gdb-events.h.
> 	(condition_command): Notify observer.
> 	(commands_command): Likewise.
> 	(commands_from_control_command): Likewise.
> 	(mention, delete_breakpoint, set_ignore_count): Likewise.
> 	(disable_breakpoint, do_enable_breakpoint): Likewise.
> 	* Makefile.in (gdb_events_h): Remove.
> 	(breakpoint_h): Update.
> 	(COMMON_OBS): Remove gdb-events.o.
> 	(gdb-events.o): Remove.
> 	(breakpoint.o, gdbarch.o, interps.o, tracepoint.o, gdbtk-bp.o,
> 	gdbtk-hooks.o, mi-cmd-break.o, tui-hooks.o): Update.
> 	* gdb-events.c: Remove.
> 	* gdb-events.h: Remove.
> 	* gdb-events.sh: Remove.

Overall, this looks OK to me. Let's wait for Vladimir to provide
his comments the MI part of your changes. There are a few nits that
need fixing before this is checked in, but except for the MI part,
this is pre-approved.

First nit: gdbarch.c is a generated file. You'll need to modify gdbarch.sh
instead, and regenerate gdbarch.c. You'll see that the changes should
be identical to the ones you made, but in a different file.

> doc/ChangeLog:
> 2008-07-12  Tom Tromey  <tromey@redhat.com>
> 
> 	* observer.texi (GDB Observers): Document new observers:
> 	breakpoint_created, breakpoint_deleted, breakpoint_modified,
> 	tracepoint_created, tracepoint_deleted, tracepoint_modified,
> 	architecture_changed.

OK.

> gdbtk/ChangeLog:
> 2008-07-12  Tom Tromey  <tromey@redhat.com>
> 
> 	* generic/gdbtk-hooks.c: Include observer.h, not gdb-events.h.
> 	(gdbtk_add_hooks): Use observers, not events.
> 	(gdbtk_architecture_changed): Add argument, for observer.
> 	* generic/gdbtk-bp.c: Include observer.h.
> 	(gdb_set_bp): Notify observer.
> 	(gdb_set_bp_addr): Likewise.

This is insight "territory". 

> Index: Makefile.in
> ===================================================================
> RCS file: /cvs/src/src/gdb/Makefile.in,v
> retrieving revision 1.1035
> diff -u -r1.1035 Makefile.in
> --- Makefile.in	11 Jul 2008 11:07:38 -0000	1.1035
> +++ Makefile.in	13 Jul 2008 00:04:57 -0000

For this part, could you re-do a pass to check that the dependencies
have all been correctly updated. For instance, I noticed that for
tracepoint.c, you correctly removed the dependency on gdb-events.h,
but forgot to add the one on observer.h. As far as I can tell, this
is the only case were something was missed.

Let's isolate the Makefile.in changes related to gdbtk and send them
to the insight list together with your other gdbtk changes.


-- 
Joel