From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4312 invoked by alias); 22 Jul 2013 20:47:44 -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 4219 invoked by uid 89); 22 Jul 2013 20:47:44 -0000 X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_40,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS,TW_BJ autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 22 Jul 2013 20:47:43 +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.14.4/8.14.4) with ESMTP id r6MKlYnc028635 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 22 Jul 2013 16:47:34 -0400 Received: from barimba (ovpn-113-128.phx2.redhat.com [10.3.113.128]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r6MKlXMs025081 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 22 Jul 2013 16:47:33 -0400 From: Tom Tromey To: Siva Chandra Cc: gdb-patches Subject: Re: [RFC] Debug Methods in GDB Python References: <87r4hefx59.fsf@fleche.redhat.com> <871u995pbt.fsf@fleche.redhat.com> Date: Mon, 22 Jul 2013 20:47:00 -0000 In-Reply-To: (Siva Chandra's message of "Mon, 17 Jun 2013 11:59:44 -0700") Message-ID: <87ehaq5nkr.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-07/txt/msg00516.txt.bz2 >>>>> "Siva" == Siva Chandra writes: Siva> Took me longer than I had expected I but could spend some time last Siva> couple of weeks and address all of Tom's comments from last time. Like Siva> before, I do not have docs or tests as the Python side API is largely Siva> un-reviewed I guess. However, I have put in code comments in the Siva> latest version. The patch is attached and the ChangeLog is as below: Thanks. I appreciate that you don't want to write all the docs and tests for a moving target. On the other hand, it's harder to review a patch that doesn't have these. A compromise would be if you wrote just the docs describing how it works, say the Python API. Then we could discuss this; and once ok, the implementation would follow. But, that is for future changes. For this one, I think we're in general agreement about the exposed API, and so polishing is a vital component. Siva> +/* Registers an extension language with GDB. */ Siva> + Siva> +void Siva> +register_ext_lang (struct ext_lang *lang) Siva> +{ Siva> + if (ext_lang_vec == NULL) Siva> + ext_lang_vec = VEC_alloc (ext_lang_p, 1); Siva> + Siva> + VEC_safe_push (ext_lang_p, ext_lang_vec, lang); The "extension language" code here seems like a lot of work for no real benefit. It's fine for the functions themselves, since we'd like to keep the API to the Python layer reasonably thin. But for get_matching_ext_methods, e.g., it is simple to follow the existing pattern: declare a function in python.h and provide both with- and without-Python implementations. Siva> +struct ext_lang Siva> + { Siva> + clone_ext_obj_ftype *clone_ext_object; Siva> + free_ext_obj_ftype *free_ext_obj; Siva> + get_matching_ext_methods_ftype *get_matching_ext_methods; Siva> + get_ext_fn_argtypes_ftype *get_ext_fn_argtypes; Siva> + invoke_method_ftype *invoke_method; Siva> + }; I think this struct should be given a different name. Typical in gdb is "ops". Siva> +extern struct ext_fn_descriptor *new_ext_function (struct ext_lang *lang, Siva> + int is_method, Siva> + void *ext_obj); It seems like 'lang' should be const here. Siva> + and the method name. It next get the argument types of these methods via s/get/gets/ Siva> + for old_method in existing_method_list: Siva> + objfile.debug_methods.remove(old_method) Siva> + objfile.debug_methods.extend(debug_methods) I notice this patch adds "debug_methods" to the objfile but not the program space. Updating the latter, and also providing a global debug_methods, is more in line with the other hooks we provide. Siva> +py_free_ext_object (void *ext_object) Siva> +{ Siva> + struct py_ext_object *o = (struct py_ext_object *) ext_object; You don't need casts to or from "void *". Siva> + enabled = PyObject_IsTrue (enabled_field); Siva> + if (enabled == -1) Siva> + { Siva> + PyErr_Clear (); Siva> + do_cleanups (cleanups); Silently ignoring errors doesn't seem right. There are a few instances of this. I'm not really sure about the error-handling strategy in this function. Siva> +static VEC (ext_fn_descriptor_p) * Siva> +py_debugmethod_name_match (struct type *obj_type, const char *method_name) [...] Siva> + if (method_vec == NULL) Siva> + method_vec = VEC_alloc (ext_fn_descriptor_p, 1); You don't need to explicitly allocate a vec like this. VEC_safe_push handles it. Siva> + self->debug_methods = PyList_New (0); Siva> + if (!self->debug_methods) Now we write "== NULL". Siva> + object->debug_methods = PyList_New (0); Siva> + if (!object->debug_methods) Ditto. Siva> +int gdbpy_initialize_debugmethods (void) Siva> + CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION; I'm curious if you ran this through the checker. It isn't a requirement; the checker gives enough false reports that it is a bit of a pain. Tom