This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Events when inferior is modified
- From: Phil Muldoon <pmuldoon at redhat dot com>
- To: Nick Bull <nicholaspbull at gmail dot com>, gdb-patches at sourceware dot org
- Date: Tue, 10 Dec 2013 11:04:35 +0000
- Subject: Re: [PATCH v2] Events when inferior is modified
- Authentication-results: sourceware.org; auth=none
- References: <51CDBD48 dot 6010903 at gmail dot com> <87k3k7kbdy dot fsf at fleche dot redhat dot com> <CABbKtXsh+BG6=s6ZmaoEFYeiFR-9SAWqMYk3DqP-Ej047zd7LQ at mail dot gmail dot com> <528A0A83 dot 1080203 at gmail dot com> <52931CEC dot 7040407 at redhat dot com> <CABbKtXsCHz_aDihw5X0dwnv=wmOe=YUqbkLqut+tZ6D4FiLVfg at mail dot gmail dot com> <52A60249 dot 5010909 at gmail dot com>
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