From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18214 invoked by alias); 1 Aug 2013 15:47:29 -0000 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 Received: (qmail 18205 invoked by uid 89); 1 Aug 2013 15:47:29 -0000 X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 01 Aug 2013 15:47:28 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r71FlKUe020166 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 1 Aug 2013 11:47:20 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r71FlIS3029639; Thu, 1 Aug 2013 11:47:19 -0400 Message-ID: <51FA8306.6020803@redhat.com> Date: Thu, 01 Aug 2013 15:47:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: Nick Bull , gdb-patches@sourceware.org Subject: Re: Events when inferior is modified References: <51CDBD48.6010903@gmail.com> <87k3k7kbdy.fsf@fleche.redhat.com> In-Reply-To: <87k3k7kbdy.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00029.txt.bz2 On 07/30/2013 10:04 PM, Tom Tromey wrote: >>>>>> >>>>> "Nick" == Nick Bull writes: > Nick> This patch adds new observers, and corresponding Python events, for > Nick> various actions on an inferior: calling a function (by hand), > Nick> modifying registers or modifying memory. > > Thanks. > > Nick> This is my first patch submission, so (a) apologies for things I've > Nick> got wrong and (b) I believe I may need to do some paperwork for > Nick> copyright assignment. > > I don't remember if I got you started on the paperwork or not. > I will send you email off-list. > > Don't worry about (a). This review is mostly little nits. > > Nick> +@deftypefun void inferior_call_pre (void) > Nick> +An inferior function is about to be called. > Nick> +@end deftypefun > Nick> + > Nick> +@deftypefun void inferior_call_post (void) > Nick> +An inferior function has just been called. > Nick> +@end deftypefun > > I'm curious to know why you wanted pre- and post-observers for inferior > calls, but not anything else. > > Also, the patch got a little mangled (see the space before the "+" on > the first quoted line here). This happened in a few spots. > > Nick> extern int emit_exited_event (const LONGEST *exit_code, struct inferior *inf); > Nick> +typedef enum { > > Blank line between the previous declaration and this one. > The brace should go on its own line. > > Nick> + > Nick> +static PyObject * > Nick> +create_inferior_call_event_object (int flag) > Nick> +{ > > Needs an intro comment. It can be short. > > I think it would be better if the "int" were instead > "inferior_altered_kind". > > Nick> + PyObject * event; > > No space after "*". > > Nick> + default: > Nick> + return NULL; > > I think gdb_assert_not_reached is more correct here. > However if you want to return NULL then you have to set the Python > exception. > > Nick> +int > Nick> +emit_inferior_altered_event (inferior_altered_kind kind) > Nick> +{ > > I think it would be fine to make this static and put all the observers > into this file. > > Nick> diff --git a/gdb/target.c b/gdb/target.c > Nick> index 920f916..1e85bdd 100644 > Nick> --- a/gdb/target.c > Nick> +++ b/gdb/target.c > Nick> @@ -43,6 +43,7 @@ > Nick> #include "tracepoint.h" > Nick> #include "gdb/fileio.h" > Nick> #include "agent.h" > Nick> +#include "observer.h" > Nick> static void target_info (char *, int); > Nick> @@ -1611,6 +1612,8 @@ memory_xfer_partial_1 (struct target_ops *ops, > Nick> enum target_object object, > Nick> do > Nick> { > Nick> + if (writebuf) > Nick> + observer_notify_memory_change (); > > I think "if (writebuf != NULL)", here and elsewhere. > > > It seem to me that this may just be an approximation of "dirty" in the > remote case. E.g., gdbserver may write a breakpoint and otherwise > modify things without notifying gdb. > > On the other hand, maybe you want to rule out breakpoints from being > considered "dirty". But then the change has to be more complicated in > another way. Yeah. Also, on several targets, including x86, GDB adjusts the PC register whenever a breakpoint is hit (as it points at the instruction after the trap, not at the trap address itself). Also, we've discussed the limitations of memory/registers changed events before: http://sourceware.org/ml/gdb-patches/2012-11/msg00156.html http://sourceware.org/ml/gdb-patches/2012-11/msg00127.html There are other things that can be changed and "dirty" the execution. Say, clobbering $_siginfo. The use case here is just "dirty". I wonder whether the all-encompassing "target_changed" event from http://sourceware.org/bugzilla/show_bug.cgi?id=7574 wouldn't be more prudent. Whatever the case, the use case, and conditions these events trigger should be very well document, or we risk using them for other use cases going forward, and breaking the original intentions. > @@ -1733,6 +1736,8 @@ target_xfer_partial (struct target_ops *ops, > if (raw_object == TARGET_OBJECT_RAW_MEMORY) > raw_object = TARGET_OBJECT_MEMORY; > > + if (writebuf) > + observer_notify_memory_change (); > retval = ops->to_xfer_partial (ops, raw_object, annex, readbuf, > writebuf, offset, len); Note this path handles other objects, not just memory. -- Pedro Alves