From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57035 invoked by alias); 25 Apr 2019 19:42:38 -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 57027 invoked by uid 89); 25 Apr 2019 19:42:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: gateway30.websitewelcome.com Received: from gateway30.websitewelcome.com (HELO gateway30.websitewelcome.com) (192.185.184.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 Apr 2019 19:42:36 +0000 Received: from cm11.websitewelcome.com (cm11.websitewelcome.com [100.42.49.5]) by gateway30.websitewelcome.com (Postfix) with ESMTP id EB4DC3E2C for ; Thu, 25 Apr 2019 14:42:34 -0500 (CDT) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id JkGQhjYpadnCeJkGQhUmKX; Thu, 25 Apr 2019 14:42:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=9AQjTS4HXaw4VkU5orCBw+CVazU48rkffI2BGZvdLGs=; b=fXt3XWnoNTxzKyN52HA6ytrjNc kDOIRbNdq7qhfY03D5OsT9TQJMT+WpdbG8cHcp2ejaz0o4tCgPq9EA7Y6rWM5KAIWiZFrm22bCeMp WkFXzx8WUp81dDEiMOpxD7JKb; Received: from 97-122-168-123.hlrn.qwest.net ([97.122.168.123]:59158 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1hJkGQ-001oC5-Ml; Thu, 25 Apr 2019 14:42:34 -0500 From: Tom Tromey To: Jan Vrany Cc: gdb-patches@sourceware.org, Didier Nadeau Subject: Re: [RFC 3/8] Create MI commands using python. References: <20190418152337.6376-1-jan.vrany@fit.cvut.cz> <20190418152337.6376-4-jan.vrany@fit.cvut.cz> Date: Thu, 25 Apr 2019 19:42:00 -0000 In-Reply-To: <20190418152337.6376-4-jan.vrany@fit.cvut.cz> (Jan Vrany's message of "Thu, 18 Apr 2019 16:23:32 +0100") Message-ID: <8736m598ie.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-04/txt/msg00560.txt.bz2 >>>>> "Jan" == Jan Vrany writes: Lots of nits on this one, mostly I think because it was converted from C? Anything, nothing very major, just lots of little things. Jan> +mi_command_py::mi_command_py (const char *name, int *suppress_notification, Jan> + void *object) Jan> + : mi_command (name, suppress_notification), Jan> + pyobj (object) Jan> +{} I think it would be better to put this sort of thing somewhere in the Python code, so that it can use gdbpy_ref<> rather than "void *". Jan> + py_mi_invoke (this->pyobj, parse->argv, parse->argc); This sort of indirection wouldn't be needed then either. Jan> +class mi_command_py : public mi_command Jan> +{ Jan> + public: Jan> + mi_command_py (const char *name, int *suppress_notification, Jan> + void *object); Jan> + void invoke (struct mi_parse *parse) override; Jan> + Jan> + private: Jan> + void *pyobj; Jan> + Jan> +}; Extra blank line before the "}". Jan> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c Jan> new file mode 100644 Jan> index 0000000000..ee612e2bc5 Jan> --- /dev/null Jan> +++ b/gdb/python/py-micmd.c Jan> @@ -0,0 +1,290 @@ Jan> +/* gdb MI commands implemented in Python */ Jan> + Jan> +#include Files need a copyright header. Jan> +#include "defs.h" defs.h must come first. I wonder if string.h is needed at all. Jan> +/* If the command invoked returns a list, this function parses it and create an Jan> + appropriate MI out output. Jan> + Jan> + The returned values must be Python string, and can be contained within Python Jan> + lists and dictionaries. It is possible to have a multiple levels of lists Jan> + and/or dictionaries. */ Jan> + Jan> +static void Jan> +parse_mi_result (PyObject *result, const char *field_name) Jan> +{ Jan> + struct ui_out *uiout = current_uiout; Jan> + Jan> + if(!field_name) gdb+gnu style would be: "if (field_name == nullptr)" Jan> + if (PyString_Check(result)) Need a space before open parens. There are a few instances of this. Jan> + { Jan> + const char *string = gdbpy_obj_to_string (result).release (); Jan> + uiout->field_string (field_name, string); Jan> + xfree ( (void *)string); Better to take advantage of the unique pointer, like: gdb::unique_xmalloc_ptr string = gdbpy_obj_to_string (result); uiout->field_string (field_name, string.get ()); However, this code is ignoring Python errors. Probably this function should use one of the standard conventions and return -1 on error. Jan> + //struct cleanup *cleanup_item = make_cleanup_ui_out_tuple_begin_end (uiout, NULL); There are a couple of these; remove them. Jan> + item = PyList_GetItem (result, i); This can fail, so it needs a check. Jan> + else if ( PyDict_Check (result)) There's an extra space after paren. Jan> + char *key_string = gdbpy_obj_to_string (key).release (); Error checking. Jan> + xfree ( (void *) key_string); Again, no need to do this explicitly. Jan> + PyObject *argobj, *result, **strings; Extra space. Jan> + if (! obj) Jan> + error(_("Invalid invocation of Python micommand object.")); This could probably be improved somehow, for instance mentioning the MI command name. I think that's sort of standard in MI commands, so this should follow it in all the errors, I think. Jan> + strings = (PyObject **) malloc (sizeof(PyObject *) * argc); Not sure this is needed, see below. But if it is it should use XNEWVEC or xmalloc. Jan> + for (i = 0; i < argc; ++i) Jan> + { Jan> + strings[i] = PyUnicode_Decode (argv[i], strlen(argv[i]), host_charset (), NULL); Jan> + if (PyList_SetItem (argobj, i, strings[i]) != 0) PyList_SetItem steals a reference, so I think the ref-counting in this function is incorrect. But, since it steals a reference, I think "strings" isn't needed. Jan> + { Jan> + error (_("Failed to create the python arguments list.")); This will leak references. However if you use gdbpy_ref<>, you should be ok. Jan> + /* Skip preceding whitespaces. */ Jan> + /* Find first character of the final word. */ Not sure if the first comment documents something that should be there and is not, or if it should just be removed. Probably the latter, I think it's fine to require command names to be valid. I'd probably skip stripping the trailing whitespace as well. Jan> + cmd_name = micmdpy_parse_command_name (name); Jan> + if (! cmd_name) Jan> + return -1; Like this could just give an error if the NAME isn't a valid MI command name, and not bother with trying to fix it up, or copy it. Jan> + PyErr_Format (except.reason == RETURN_QUIT Jan> + ? PyExc_KeyboardInterrupt : PyExc_RuntimeError, Jan> + "%s", except.message); I think GDB_PY_SET_HANDLE_EXCEPTION is better for this. Jan> +/* Initialize the MI command object. */ Jan> + Jan> +int Jan> +gdbpy_initialize_micommands(void) Don't need (void) any more. Jan> +++ b/gdb/python/py-micmd.h Jan> @@ -0,0 +1,6 @@ Jan> +#ifndef PY_MICMDS_H Jan> +#define PY_MICMDS_H Jan> + Jan> +void py_mi_invoke (void *py_obj, char **argv, int argc); Jan> + I'd just stick this in python-internal.h and not have a new header. thanks, Tom