This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Events when inferior is modified


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]