From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2317 invoked by alias); 4 Jan 2011 20:09:36 -0000 Received: (qmail 2307 invoked by uid 22791); 4 Jan 2011 20:09:34 -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; Tue, 04 Jan 2011 20:09:25 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id p04K9OMV009543 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 4 Jan 2011 15:09:24 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p04K9NrB001543; Tue, 4 Jan 2011 15:09:24 -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 p04K9NaU013338; Tue, 4 Jan 2011 15:09:23 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id CC4EB37848D; Tue, 4 Jan 2011 13:09:22 -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> Date: Tue, 04 Jan 2011 20:09:00 -0000 In-Reply-To: <4D2342A2.7060102@redhat.com> (sami wagiaalla's message of "Tue, 04 Jan 2011 10:54:10 -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/msg00067.txt.bz2 >>>>> "Sami" == sami wagiaalla writes: Sami> This patch is base on the work done by Oguz Kayral in the summer of Sami> 2009. Tom Tromey and Phil Muldoon also provided a lot of help and Sami> guidance on this. Thanks for doing this. This is a very important feature. Sami> A documentation patch will follow soon. I think it is better for a patch to include documentation and, when applicable (it is for this patch), a NEWS entry. Sami> Thanks in advance for the review. One thing I noticed is that now all the copyright headers should be updated to include 2011 :-) Sami> +breakpoint_event_object * Sami> +create_breakpoint_event_object (struct bpstats *bs, thread_object *stopped_thread) Wrap this line at the ",". Sami> +continue_event_object * Sami> +create_continue_event_object () Change to (void). Sami> +/* Callback function which notifies observers when a continue event occurs. Sami> + This function will create a new Python continue event object. Sami> + Return -1 if emit fails. */ Sami> + Sami> +int Sami> +emit_continue_event (ptid_t ptid) Sami> +{ Sami> + event_object *event; Sami> + if (evregpy_get_nlisteners (gdb_py_events.cont) == 0) Newline between the above 2 lines. Instead of evregpy_get_nlisteners checking the list size, I think it would be better to have "evregpy_any_listeners_p" return a boolean, and then check whether the list is empty. This may be more efficient and is what you want to do anyhow. Sami> + return evpy_emit_event (event, Sami> + gdb_py_events.cont); I don't think this line needs to be split. Sami> +event_object * Sami> +create_event_object (PyTypeObject *py_type) Sami> +{ Sami> + event_object *event_obj; Sami> + Sami> + event_obj = PyObject_New (event_object, py_type); Sami> + if (!event_obj) Sami> + goto fail; Sami> + Sami> + event_obj->dict = PyDict_New (); Sami> + if (!event_obj->dict) Sami> + goto fail; Sami> + Sami> + return event_obj; Sami> + Sami> + fail: Sami> + Py_XDECREF (event_obj); Sami> + Py_XDECREF (event_obj->dict); Won't decrefing event_obj automatically free the dict when needed? I didn't look closely but maybe other create_* functions have this issue. Sami> +/* Implementation of EventRegistry.disconnect () -> NULL. Sami> + Remove FUNCTION to the list of listeners. */ s/to/from/ Sami> + PySequence_DelItem (callback_list, PySequence_Index (callback_list, func)); I think you have to separate the calls here and then check the PySequence_Index result. Sami> + eventregistry_obj->callbacks = (PyListObject *) PyList_New (0); This needs a failure check. Sami> +static int Sami> +add_new_registry (eventregistry_object **registryp, char *name) Sami> +{ Sami> + *registryp = create_eventregistry_object (); Sami> + if(*registryp == NULL) Newline between these lines. Space before open paren. Why not just return the new registry, or NULL on error? That would be simpler. Sami> +void Sami> +gdbpy_initialize_py_events () Sami> +{ Sami> + No blank line here. Sami> + Py_INCREF (gdb_py_events.module); Sami> + if(PyModule_AddObject (gdb_module, Space before open paren. Sami> + PyLongObject *exit_code; I think you could remove a lot of casts if this were just a PyObject*. Sami> +emit_exited_event (LONGEST *exit_code) Sami> +{ Sami> + event_object *event; Sami> + if (evregpy_get_nlisteners (gdb_py_events.exited) == 0) Blank line after declarations. Sami> + if (event) Sami> + return evpy_emit_event ((event_object *) Sami> + event, I think this cast isn't needed. Sami> +static void Sami> +python_inferior_exit (struct inferior *inf) Sami> +{ Sami> + struct cleanup *cleanup; Sami> + LONGEST exitcode_val; Sami> + LONGEST *exit_code; Sami> + Sami> + cleanup = ensure_python_env (get_current_arch (), current_language); Sami> + Sami> + if (get_internalvar_integer (lookup_internalvar ("_exitcode"), &exitcode_val)) Sami> + exit_code = &exitcode_val; Sami> + Sami> + if (exit_code Sami> + && emit_exited_event (exit_code) < 0) You have to initialize exit_code to NULL for this to work properly. However, I think this is pretty ugly. It seems like there should be a better way to get this than looking up a convenience variable. I think we need an event representing a thread exit. It is ok by me if this comes in a separate patch. (FWIW I have a few new events on my branch; I'll update those once this patch goes in.) Sami> + signal_event_obj->stop_signal = Sami> + (PyStringObject *) PyString_FromString (stop_signal); It is more usual to just use PyObject* everywhere, and not cast to the more specific types. This change should let you eliminate other casts in the patch. Sami> +stop_event_object * Sami> +create_stop_event_object (PyTypeObject *py_type, thread_object *thread) Sami> +{ Sami> + stop_event_object *stop_event_obj = Sami> + (stop_event_object *) create_event_object (py_type); Sami> + Sami> + if (!stop_event_obj) Sami> + goto fail; Sami> + Sami> + stop_event_obj->inferior_thread = (PyObject *) thread; Sami> + Sami> + if (evpy_add_attribute ((event_object *) stop_event_obj, Sami> + "inferior_thread", Sami> + stop_event_obj->inferior_thread) < 0) Sami> + goto fail; Sami> + Sami> + Sami> + return stop_event_obj; I think it would be better to just have one cast at the end, instead of lots of casts in the body. Sami> + if (non_stop) Sami> + stopped_thread = find_thread_object (inferior_ptid); Sami> + else Sami> + stopped_thread = (thread_object *) Py_None; This cast is not ok. Also this code must incref even in this case, since it is decrefed when the event is destroyed. Sami> + if (bs && bs->breakpoint_at Sami> + && bs->breakpoint_at->type == bp_breakpoint) Sami> + { Sami> + if (evregpy_get_nlisteners (gdb_py_events.breakpoint) == 0) Sami> + return 0; I think the short-circuiting logic should be hoisted to the top of the function. This is more efficient and also lets you avoid having to deal with reference counting problems involving objects made earlier. Sami> + /* Check if the signal is "Signal 0" or "Trace/breakpoint trap". */ Sami> + if ((strcmp (stop_signal, "0") != 0) Sami> + && (strcmp (stop_signal, "SIGTRAP") != 0)) I didn't look this up, but this seems questionable. Is this really how this is done? Sami> +typedef struct Sami> +{ Sami> + PyObject *inferior_thread; Sami> + event_object event; Sami> +} stop_event_object; For the inheritance scheme to work, the 'event' field has to come first. I didn't audit the other event object types, but please make sure they are all correct. Sami> +/* Returns a a copy of the give LIST. Sami> + Creates a new reference which must be handled by the caller. */ Sami> + Sami> +PyObject * Sami> +copy_py_list (PyObject *list) I don't think we need this function. Just use PySequence_List. Tom