From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24591 invoked by alias); 10 May 2013 19:33:13 -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 24580 invoked by uid 89); 10 May 2013 19:33:12 -0000 X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_XZ autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 10 May 2013 19:33:12 +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 r4AJX92R017174 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 10 May 2013 15:33:10 -0400 Received: from barimba (ovpn-113-133.phx2.redhat.com [10.3.113.133]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r4AJX7V3011592 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 10 May 2013 15:33:08 -0400 From: Tom Tromey To: Siva Chandra Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Debug Methods in GDB Python References: Date: Fri, 10 May 2013 19:33:00 -0000 In-Reply-To: (Siva Chandra's message of "Mon, 28 Jan 2013 17:49:08 -0800") Message-ID: <87r4hefx59.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-05/txt/msg00398.txt.bz2 >>>>> "Siva" == Siva Chandra writes: Siva> Attached is a new version of the patch which I have now regression Siva> tested. Except for tests and docs, I think it is complete wrt the Siva> feature set I have in mind. Hence, the caveats from the previous mail Siva> still apply. I'm sorry about the delay here. I think the approach is generally a good one. I didn't read the overloading bits in detail. I'm not too concerned about that though. Siva> +static ULONGEST enabled_ext_languages = EXT_LANG_PYTHON; I don't think we need this. I'd rather just have this follow the "virtual methods" approach where a function is an object that has whatever methods it needs, plus a destructor. Siva> + struct ext_fn_descriptor *ext_fn = Siva> + (struct ext_fn_descriptor *) xzalloc (sizeof (struct ext_fn_descriptor)); FWIW, XCNEW encapsulates all this. Siva> + if (item->is_method) Siva> + { Siva> + if (item->lang == EXT_LANG_PYTHON) Siva> + py_free_ext_object (item->ext_object); Siva> + } ... then here this could just call the destructor method, and not need to know anything about Python specifically. Siva> +struct ext_fn_descriptor * Siva> +ext_fn_list_extend (struct ext_fn_descriptor *l1, struct ext_fn_descriptor *l2) Siva> +{ Siva> + struct ext_fn_descriptor *item = l1; Siva> + Siva> + if (!l1) Siva> + return l2; Siva> + Siva> + if (!l2) Siva> + return l1; Siva> + Siva> + while (item->next) Siva> + item = item->next; Siva> + Siva> + item->next = l2; Why a linked list instead of a VEC? We already have all this support code written for VECs. Siva> +#ifdef HAVE_PYTHON Siva> + if ((enabled_ext_languages && EXT_LANG_PYTHON) Siva> + && (ext_fn->lang == EXT_LANG_PYTHON)) Siva> + new_ext_fn->ext_object = py_clone_ext_object (ext_fn->ext_object); Siva> +#endif This is the kind of thing that can be avoided with the virtual method approach. Basically any #ifdef HAVE_PYTHON is suspect. Lots of littler nits in the patch -- function comments, more things should be 'static'. Siva> +PyObject * Siva> +gdbpy_enable_debug_methods (PyObject *self, PyObject *args) Why is a special enabling method needed? Siva> + if (PyCallable_Check (match_methods_temp) Siva> + && PyCallable_Check (get_argtypes_temp) Siva> + && PyCallable_Check (invoke_method_temp)) Siva> + { ... Siva> + } Siva> + Siva> + return result; This can return NULL without setting the exception. Also, it does decrefs before increfs, but I think it is better to do this the other way around. Siva> + struct ext_fn_descriptor *ext_fn = new_ext_function (); Siva> + struct py_ext_object *ext_object; Siva> + Siva> + ext_object = (struct py_ext_object *) xzalloc (sizeof (struct py_ext_object)); Siva> + ext_object->object = item; Siva> + ext_object->match_py_obj_type = py_obj_type; Siva> + ext_object->match_method = method; Siva> + Siva> + ext_fn->lang = EXT_LANG_PYTHON; Siva> + ext_fn->is_method = 1; Siva> + ext_fn->ext_object = (void *) ext_object; Siva> + Siva> + return ext_fn; It seems like there should be a way to hang the needed data directly on the ext_fn, say via subclassing. Siva> +struct ext_fn_descriptor * Siva> +py_debugmethod_name_match (struct type *obj_type, const char *method_name) Siva> +{ Siva> + PyObject *py_type, *py_debugmethod_list = NULL, *list_iter, *item; Siva> + struct cleanup *cleanups; Siva> + struct ext_fn_descriptor *method_list = NULL; Siva> + Siva> + if (!obj_type || !match_methods_callable) Siva> + return NULL; Siva> + Siva> + cleanups = ensure_python_env (get_current_arch (), current_language); I don't think this is thread-safe. ensure_python_env acquires the GIL, but this checks match_methods_callable outside the lock. Siva> + list_iter = PyObject_GetIter (py_debugmethod_list); Siva> + while ((item = PyIter_Next (list_iter))) Error check for list_iter. Siva> + { Siva> + struct ext_fn_descriptor *ext_fn; Siva> + Siva> + ext_fn = new_python_ext_method (item, py_type, method_name); Siva> + method_list = ext_fn_list_prepend (method_list, ext_fn); Siva> + } Siva> + Siva> + Py_DECREF (list_iter); Need an error check here too -- the loop may exit with an error. Siva> + py_argtype_list = PyObject_CallFunction (get_argtypes_callable, Siva> + "OOs", Siva> + ext_object->object, Siva> + ext_object->match_py_obj_type, Siva> + ext_object->match_method); Siva> + Siva> + list_iter = PyObject_GetIter (py_argtype_list); Error checking here and elsewhere. Siva> + { "enable_debug_methods", gdbpy_enable_debug_methods, METH_VARARGS, Siva> + "inferiors (meth_match_function, args_match_function) -> None.\n\ Siva> +Enables Python debug methods feature." }, Wrong doc string. Tom