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: [PATCH v2] Events when inferior is modified


On 09/12/13 17:47, Nick Bull wrote:
> On 25 November 2013 09:48, Phil Muldoon <pmuldoon@redhat.com> wrote:
> 
> Now with the nits addressed, rejigging to use the existing
> memory_changed observer, some test coverage and documentation.
> make check ran without regressions.

Thanks.


>         (emit_memory_changed_event): New prototype.
>         * python/py-events.h (events_object): New registries
>     inferior_call, memory_changed and register_changed.

Might be a tab or space issue, or even a mailer issue, but indention
looks incorrect  here.

> +Emits @code{gdb.MemoryChangedEvent} which indicates that the memory of the
> +inferior has been modified by the GDB user, for instance via a command like
> +@code{set *addr = value}.  The event has the following attributes:
> +
> +@defvar MemoryChangedEvent.address
> +The start address of the changed region.
> +@end defvar
> +@defvar MemoryChangedEvent.length
> +Length in bytes of the changed region.
> +@end defvar
> +
> +@item register_changed
> +Emits @code{gdb.RegisterChangedEvent} which indicates that a register in the
> +inferior has been modified by the GDB user.

I apologize for not catching this initially, but is it possible to
have what registers changed? I suppose you could scan beforehand, and
scan after.  But it would be a better API if registers' names were
included as an attribute. If this requires major surgery though, we
can look again at the original API for inclusion. Also it is not clear
from the tests. but would this callback trigger on frame change (either
through execution or the user selecting a different frame)?  Registers
are swapped in and out as each frame changes.


> +  PyObject * event;
> +  PyObject *addr_obj = NULL;
> +  PyObject *len_obj = NULL;
> +  int failed;
> +
> +  event = create_event_object (&memory_changed_event_object_type);
> +
> +  if (!event)
> +    goto fail;

This is a fairly new rule, but for all new submissions any pointer
expression has to be explicitly written.  So, rewrite to:

if (event == NULL)

> +  addr_obj = PyLong_FromLongLong (addr);
> +  if (addr_obj == NULL)
> +    goto fail;
> +
> +  failed = evpy_add_attribute (event, "address", addr_obj) < 0;
> +  Py_DECREF (addr_obj);
> +  if (failed)
> +    goto fail;

I believe there is a problem here in the failed branch.  You have
already decremented the reference count of addr_obj once, and in the
fail goto you do it again.  In these cases of conditional cleanups I
find the make_cleanup/do_cleanup kind of logic easier to write. If you
believe you will be saved by the XDECREF call not working on NULL,
remember that Py_DECREF just decrements the reference count of the
object, and nothing else.  So if the Python GC collected that object,
any further writes to that address would result in a possible sigsegv.

> +  len_obj = PyLong_FromLong (len);
> +  if (len_obj == NULL)
> +    goto fail;

Same here with addr_obj being decremented twice,

> +  failed = evpy_add_attribute (event, "length", len_obj) < 0;
> +  Py_DECREF (len_obj);
> +  if (failed)
> +    goto fail;

Here too.


> +# Test register changed event
> +gdb_test_no_output {set $old_sp = $sp}
> +gdb_test {set $sp = 0} ".*event type: register-changed.*"
> +gdb_test {set $sp = 1} ".*event type: register-changed.*"
> +gdb_test {set $sp = $old_sp} ".*event type: register-changed.*"

We need some tests to see if register changes initiated by frame
selection or program execution trigger the event mechanism.

> +# Test memory changed event
> +gdb_test_no_output {set $saved = *(int*) $sp}
> +gdb_test {set *(int*) $sp = 0} ".*event type: memory-changed.*
> +.*address: .*
> +.*length: .*"
> +gdb_test {set *(int*) $sp = $saved} ".*event type: memory-changed.*
> +.*address: .*
> +.*length: .*"

We need some tests here to test whether breakpoint hit, creation and
deletion trigger these (I don't think they should, but let's
create a barrier test to prove it).

Looking very good in general. I look forward to the next revision!

Cheers,

Phil


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