From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6386 invoked by alias); 12 Feb 2013 21:18:35 -0000 Received: (qmail 6359 invoked by uid 22791); 12 Feb 2013 21:18:30 -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; Tue, 12 Feb 2013 21:18:19 +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 r1CLIIpm026863 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 12 Feb 2013 16:18:18 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1CLIGWU029369 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 12 Feb 2013 16:18:17 -0500 From: Tom Tromey To: Siva Chandra Cc: Doug Evans , gdb-patches Subject: Re: [RFC - Python Scripting] New method gdb.Architecture.disassemble References: <20753.38272.55066.651097@ruffy2.mtv.corp.google.com> Date: Tue, 12 Feb 2013 21:18:00 -0000 In-Reply-To: (Siva Chandra's message of "Tue, 12 Feb 2013 06:56:51 -0800") Message-ID: <87txphmdt3.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/msg00289.txt.bz2 >>>>> "Siva" == Siva Chandra writes: Siva> Thanks for your detailed reply. I now have a patch which does not use Siva> ui_out. Instead of calling gdb_disassembly, it essentially Siva> re-implements disasm.c:dump_insns. The patch is attached. I don't have any problem with this approach. I do have some nits on the patch though. Siva> +/* Implementation of gdb.Architecture.disassemble (self, low, high) -> List. Siva> + Returns a list of instructions in a memory address range. Each instruction Siva> + in the list is a dict object with the following string keys: Siva> + You can write a shorter comment here and just put all the informative stuff into the docs. Siva> + static char unknown_str[] = { '<', 'u', 'n', 'k', 'n', 'o', 'w', 'n', '>', '\0' }; This line looks too long. I think it isn't too hard to reorganize so that this can be an ordinary const char *. Siva> + result_list = PyList_New (0); Siva> + if (!result_list) Siva> + { Siva> + PyErr_SetString (PyExc_MemoryError, Siva> + _("Unable to create a list of disassembled " Siva> + "instructions.")); Siva> + return NULL; You don't need PyErr_SetString here. PyList_New will have done that already. So just return NULL. Siva> + struct ui_file *memfile = mem_fileopen (); Siva> + PyObject *insn_dict = PyDict_New (); Siva> + volatile struct gdb_exception except; Siva> + Siva> + if (!insn_dict) Siva> + { Siva> + Py_DECREF (result_list); Siva> + PyErr_SetString (PyExc_MemoryError, Siva> + _("Unable to create a dict for instruction.")); Siva> + Siva> + return NULL; Here too. This leaks memfile on failure. Siva> + } Siva> + if (PyList_Append (result_list, insn_dict)) Siva> + { Siva> + Py_DECREF (result_list); Siva> + Py_DECREF (insn_dict); Siva> + Siva> + return NULL; /* PyList_Append Sets the exception. */ This leaks it too. Siva> + if (funcname) Siva> + xfree (funcname); Siva> + if (filename) Siva> + xfree (filename); Siva> + if (asm_code) Siva> + xfree (asm_code); xfree handles NULL arguments fine, so remove the "if"s. Siva> + if (PyDict_SetItemString (insn_dict, "addr", Siva> + gdb_py_long_from_ulongest (pc)) Siva> + || PyDict_SetItemString (insn_dict, "asm", Siva> + PyString_FromString (asm_code)) I think PyString_FromString handles const char * arguments fine. So you can make this: asm_code == NULL ? unknown_str : asm_code and avoid some hair above and below, and fix up unknown_str as well. Siva> + { Siva> + Py_DECREF (result_list); Siva> + PyErr_SetString (PyExc_MemoryError, Siva> + _("Unable to add fields to instruction dict.")); Siva> + Siva> + return NULL; You don't need the PyErr_SetString. And this leaks some memory. Siva> + if (funcname && funcname != unknown_str) Siva> + xfree (funcname); Siva> + if (filename && filename != unknown_str) Siva> + xfree (filename); Siva> + if (asm_code && asm_code != unknown_str) Siva> + xfree (asm_code); No need to check nullity; but with the other changes this could all be unconditional. Tom