From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32264 invoked by alias); 14 Jul 2008 17:10:37 -0000 Received: (qmail 32256 invoked by uid 22791); 14 Jul 2008 17:10:36 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 14 Jul 2008 17:10:09 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7516F2A9736; Mon, 14 Jul 2008 13:10:07 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id pDMbIaCkER+w; Mon, 14 Jul 2008 13:10:07 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 0D0032A9732; Mon, 14 Jul 2008 13:10:07 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id E6A3DE7ACD; Mon, 14 Jul 2008 10:10:04 -0700 (PDT) Date: Mon, 14 Jul 2008 17:10:00 -0000 From: Joel Brobecker To: Tom Tromey , vladimir@codesourcery.com Cc: gdb-patches@sources.redhat.com Subject: Re: RFA: Remove gdb-events Message-ID: <20080714171004.GC3998@adacore.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.2i Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-07/txt/msg00283.txt.bz2 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 > > * 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 > > * 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 > > * 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