From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24591 invoked by alias); 19 Jan 2011 16:17:58 -0000 Received: (qmail 24515 invoked by uid 22791); 19 Jan 2011 16:17:42 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 19 Jan 2011 16:17:36 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id p0JGHY3A002341 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 19 Jan 2011 11:17:34 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p0JGHYtu012442; Wed, 19 Jan 2011 11:17:34 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p0JGHXvI014127; Wed, 19 Jan 2011 11:17:33 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id E9005378142; Wed, 19 Jan 2011 09:17:32 -0700 (MST) From: Tom Tromey To: sami wagiaalla Cc: gdb-patches@sourceware.org Subject: Re: [patch] Support inferior events in python References: <4D2342A2.7060102@redhat.com> <4D34AF3F.4090006@redhat.com> Date: Wed, 19 Jan 2011 16:42:00 -0000 In-Reply-To: <4D34AF3F.4090006@redhat.com> (sami wagiaalla's message of "Mon, 17 Jan 2011 16:06:07 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2011-01/txt/msg00397.txt.bz2 >>>>> "Sami" == sami wagiaalla writes: Sami> This patch contains changes requested by Eli and Tom, and includes Sami> documentation for the new features. Thank you very much Tom> Instead of evregpy_get_nlisteners checking the list size, I think it Tom> would be better to have "evregpy_any_listeners_p" return a boolean, and Tom> then check whether the list is empty. This may be more efficient and is Tom> what you want to do anyhow. Sami> How about evregpy_no_listeners_p ? Fits better with the way the code Sami> was written. Perfect. Tom> Why not just return the new registry, or NULL on error? Tom> That would be simpler. Sami> Adding the registry, and doing the needed error checking here, makes Sami> the calling code simpler. The calling code is what will be extended in Sami> future development. I looked at this in the new patch, and I see what you mean. I agree with your approach. Tom> However, I think this is pretty ugly. Tom> It seems like there should be a better way to get this than looking up Tom> a convenience variable. Sami> Hmm I looked through the code to find another way but could Sami> not. handle_inferior_event which sets the convenience variable uses Sami> execution_control_state which I don't have access to. I don't want to hold up this patch for a detail like this, but I wonder if we could put the exit code into the struct inferior as well as in the convenience variable. Tom> It is more usual to just use PyObject* everywhere, and not cast to the Tom> more specific types. Tom> This change should let you eliminate other casts in the patch. Sami> Done. I also, updated breakpoint_event_object. I was going with the Sami> oposite mindset of keeping the type information until a cast is Sami> required. Yeah, ordinarily that would be better, but Python pretty much assumes PyObject* everywhere. [create_stop_event_object] Tom> I think it would be better to just have one cast at the end, instead of Tom> lots of casts in the body. Sami> Hmm if I change stop_event_object* to event_object it would eliminate Sami> two casts but also add two. One when setting the inferior thread and Sami> one for returning. Same goes for all the create_* functions or should Sami> I change all of those to return more generic objects ? It seems to me that the callers of the create_* functions then proceed to downcast anyway: Sami> + event = (event_object *) create_continue_event_object (); Sami> + if (event) Sami> + return evpy_emit_event (event, gdb_py_events.cont); Sami> + return -1; Given this, I think just returning the more generic type is ok. Sami> I improved this by using enum target_signal and target_signal_to_name Sami> to convert the signal to a string when notifying python Sami> listeners. That looks OK IMO, but we can also create a module Sami> gdb.signal, create a pyhon Signal type, add Signal types for all Sami> signals to gdb.signal, and use a Signal object to notify python Sami> listeners. I think it is ok to use strings. Sami> +* Python Support for Inferior events. Sami> + Python scripts can add observers to be notified of events Sami> + occurring the in process being debugged. Sami> + The following events are currently supported: Sami> + - gdb.events.breakpoint Breakpoint hit event. Sami> + - gdb.events.cont Continue event. Sami> + - gdb.events.signal Signal received event. Sami> + - gdb.events.exited Inferior exited event. There is a "Python scripting" section in NEWS already. This should be in that section. Sami> +* Inferior Events In Python:: Listening for events from the process being debugged. Line wraps. I think the node name should not have "Inferior" in it, since we may well use this facility for events not originating from the inferior. Sami> +The Python API allows scripts to listen for events coming from the inferior process Sami> +and its threads. In order to listen for events the script must register an observer Sami> +by connecting it to the appropriate event registry. Event registries can be accessed Sami> +through the @code{gdb.events} module. All the lines in this paragraph wrap. This occurs in a few places in the documentation patch. Also, we use two spaces after a period. I think the introduction could be expanded on a bit. How about something like: GDB provides a general event facility so that Python code can be notified of various state changes, particularly changes that occur in the inferior. An @defn{event} is just an object that describes some state change. The type of the object and its attributes will vary depending on the details of the change. All the existing events are described below. In order to be notified of an event, you must register an event handler with an event registry. An @defn{event registry} is just an object in the @code{gdb.event} module which dispatches particular events. A registry provides methods to register and unregister event handlers. Sami> +Here is an example: Sami> + Sami> +@smallexample Sami> +def exit_handler (event): Sami> + if (isinstance (event, gdb.ExitedEvent)): We shouldn't use isinstance in an example. I think that is sort of bad style, particularly if the `exited' event registry can only ever dispatch ExitedEvent objects. Sami> +@table @code Sami> +@item events.breakpoint Sami> +@item events.cont Sami> +@item events.exited Sami> +@item events.signal Sami> +@end table This seems weird. At least each of these should have a description indicating what it is good for. And, before this, there should be a section describing the event registry class itself. I didn't see any documentation for the connect and disconnect methods. Instead of separate signal and breakpoint registries, why not a general "stopped" registry that emits different events depending on why the inferior stopped? Sami> +These registries emit the following events in respective order: I think it would be better to use nested tables here. That is, it should be pretty easy to read the text and see what a given registry does and what kind of events it can generate. Sami> +@defivar SignalEvent stop_signal Sami> +The signal received by the inferior Sami> +@end defivar This should describe the possible values for this attribute. I think all the attributes should have this sort of documentation. Sami> +@node gdb.events Sami> +@subsubsection gdb.events Sami> +@cindex gdb.events I understand why you put this here, but I think it isn't the best place for it. True, gdb.events is a module -- but I think it would be clearer for the reader if it were just documented where the discussion of events takes place. (This node was really created for modules actually written in Python. But, that doesn't actually matter to users, either... perhaps this existing text should be moved, but of course that isn't related to this patch.) Sami> + callback_list = (PyObject *) registry->callbacks; Right now `callbacks' is a PyListObject*. If it were a PyObject* then I think all the casts associated with it would go away. Sami> +extern int emit_stop_event (struct bpstats *bs, enum target_signal stop_signal); This line is too long. Sami> +extern event_object * create_event_object (PyTypeObject *py_type); Remove space after "*". Sami> + PyList_Append (callback_list, func); Needs an error check. Sami> +static PyObject * Sami> +evregpy_disconnect (PyObject *self, PyObject *function) [...] Sami> + if (!PyCallable_Check (func)) Sami> + { Sami> + PyErr_SetString (PyExc_RuntimeError, "Function is not callable"); Sami> + return NULL; I don't think we need to bother with this check here. Sami> + if (index < 0) Sami> + { Sami> + PyErr_SetString (PyExc_RuntimeError, "Function not found"); Sami> + return NULL; And I think it is ok to just ignore this case. It seems ok to me to have an attempted removal of an item simply do nothing if the item isn't on the list. Sami> + if (get_internalvar_integer (lookup_internalvar ("_exitcode"), &exitcode_val)) This line is too long. Sami> + signal_name = (char *) target_signal_to_name (stop_signal); Sami> + signal_event_obj->stop_signal = PyString_FromString (signal_name); I think you can make signal_name a const char * and avoid the cast here. Or did we have some problem with this elsewhere? I can't remember. Sami> +PyObject * Sami> +get_stopped_thread () `(void)' Sami> +extern stop_event_object * create_stop_event_object (PyTypeObject *py_type, Remove space after "*". Tom