From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9672 invoked by alias); 6 Feb 2013 19:58:50 -0000 Received: (qmail 9655 invoked by uid 22791); 6 Feb 2013 19:58:49 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS 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, 06 Feb 2013 19:58:42 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r16Jwe3s025469 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 6 Feb 2013 14:58:40 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r16Jwdcq013979 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 6 Feb 2013 14:58:39 -0500 From: Tom Tromey To: Siva Chandra Cc: gdb-patches@sourceware.org Subject: Re: [RFC - Python Scripting] New method gdb.Architecture.disassemble References: Date: Wed, 06 Feb 2013 19:58:00 -0000 In-Reply-To: (Siva Chandra's message of "Mon, 4 Feb 2013 06:09:35 -0800") Message-ID: <87y5f1w6xc.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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: 2013-02/txt/msg00160.txt.bz2 >>>>> "Siva" == Siva Chandra writes: Siva> The attached patch adds a new method 'disassemble' to the class Siva> gdb.Architecture. I have not yet added docs and tests, but will do so Siva> after I get feedback that adding such a method is OK. Why is it a method on gdb.Architecture and not elsewhere? For example, we have gdb.Inferior.read_memory. Siva> + if (!PyArg_ParseTuple (args, "KK|i", &low, &high, &opcodes)) Usually if we support more than one argument, we allow keywords as well. Siva> + if (opcodes) Siva> + flags = DISASSEMBLY_RAW_INSN; Why an int treated as a boolean rather than a real boolean? Siva> + TRY_CATCH (except, RETURN_MASK_ALL) Siva> + { Siva> + gdb_disassembly (gdbarch, py_out, NULL, flags, -1, low, high); Why is py_out a global instead of something created and destroyed here? Siva> + if (! (PyList_Check (temp) && PyList_Size (temp) > 0)) Siva> + return NULL; You can't return NULL without setting the exception. Siva> +struct ui_out *py_out; I'd rather avoid new globals, and I don't see what this one adds. Siva> +/* This data structure captures the Python version of ui_out. The Python Siva> + version is not used to display output to a user, but to capture the results Siva> + from GDB's internals in to a Python data structure. Hence, it does not have Siva> + any representation for table headers. However, it can be viewed as a Siva> + recursive table structure wherin the highest level is a list of rows. All Siva> + rows in this list can either be a list themselves, or all of them can be Siva> + dicts holding the table's fields. If they were lists, then they follow the Siva> + same recurrsive structure as the higher levels. Typo, "recursive". I like this -- I've wanted it before -- but I wonder whether it handles all cases. See http://sourceware.org/bugzilla/show_bug.cgi?id=11688#c6 Siva> +static struct row_data * Siva> +new_row (struct row_data *parent) Siva> +{ Siva> + struct row_data *row; Siva> + Siva> + row = (struct row_data *) xmalloc (sizeof (struct row_data)); You don't need the cast. Siva> +PyObject * Siva> +fetch_and_reset_py_out_object (struct ui_out *ui_out) All new functions should have an intro comment. Siva> + temp = py_out_data->table->data; Siva> + py_out_data->table->data = PyList_New (0); What if PyList_New returns NULL here? Then the exception will be set, making problems later. Siva> + /* Commit the row to the parent list. */ Siva> + PyList_Append (py_out_data->current_row->parent_row->data, Siva> + py_out_data->current_row->data); What if this fails? This applies to many calls into Python in the patch. Siva> +static struct ui_out_impl py_ui_out_impl = Siva> +{ Siva> + 0, /* table_begin */ Siva> + 0, /* table_body */ Siva> + 0, /* table_end */ Why are these NULL? Siva> + 0 /* is_mi_like_p */ It seems to me that this should be 1. (Well, really, this is all a hack that should go away, but I doubt I'll live to see that.) Tom