public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Pedro Alves <palves@redhat.com>
To: Tom Tromey <tromey@redhat.com>
Cc: Nick Bull <nicholaspbull@gmail.com>, gdb-patches@sourceware.org
Subject: Re: Events when inferior is modified
Date: Thu, 01 Aug 2013 15:47:00 -0000	[thread overview]
Message-ID: <51FA8306.6020803@redhat.com> (raw)
In-Reply-To: <87k3k7kbdy.fsf@fleche.redhat.com>

On 07/30/2013 10:04 PM, Tom Tromey wrote:
>>>>>> >>>>> "Nick" == Nick Bull <nicholaspbull@gmail.com> 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

  reply	other threads:[~2013-08-01 15:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-28 17:07 Nick Bull
2013-07-30 21:04 ` Tom Tromey
2013-08-01 15:47   ` Pedro Alves [this message]
2013-08-07 14:41   ` Nick Bull
2013-11-18 12:51     ` [PATCH v2] " Nick Bull
2013-11-18 15:15       ` Eli Zaretskii
2013-11-25 11:50       ` Phil Muldoon
2013-12-01 15:23         ` Nick Bull
2013-12-09 17:48           ` Nick Bull
2013-12-10 11:04             ` Phil Muldoon
2013-12-12 17:12               ` Nick Bull
2013-12-21 21:26                 ` Nick Bull

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51FA8306.6020803@redhat.com \
    --to=palves@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=nicholaspbull@gmail.com \
    --cc=tromey@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).