From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4540 invoked by alias); 10 Dec 2013 11:04:46 -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 4525 invoked by uid 89); 10 Dec 2013 11:04:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Dec 2013 11:04:45 +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 rBAB4bTC008913 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 10 Dec 2013 06:04:37 -0500 Received: from localhost.localdomain (ovpn-112-30.ams2.redhat.com [10.36.112.30]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rBAB4ZGD002881; Tue, 10 Dec 2013 06:04:36 -0500 Message-ID: <52A6F543.9040201@redhat.com> Date: Tue, 10 Dec 2013 11:04:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: Nick Bull , gdb-patches@sourceware.org Subject: Re: [PATCH v2] Events when inferior is modified References: <51CDBD48.6010903@gmail.com> <87k3k7kbdy.fsf@fleche.redhat.com> <528A0A83.1080203@gmail.com> <52931CEC.7040407@redhat.com> <52A60249.5010909@gmail.com> In-Reply-To: <52A60249.5010909@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2013-12/txt/msg00364.txt.bz2 On 09/12/13 17:47, Nick Bull wrote: > On 25 November 2013 09:48, Phil Muldoon 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