From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31748 invoked by alias); 4 Feb 2010 23:25:07 -0000 Received: (qmail 31702 invoked by uid 22791); 4 Feb 2010 23:25:05 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,SPF_HELO_PASS,SPF_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; Thu, 04 Feb 2010 23:25:00 +0000 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o14NOx36005774 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 4 Feb 2010 18:24:59 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx04.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o14NOw9X023742; Thu, 4 Feb 2010 18:24:58 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o14NOvHU002792; Thu, 4 Feb 2010 18:24:57 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 2C60B379961; Thu, 4 Feb 2010 16:24:57 -0700 (MST) From: Tom Tromey To: Phil Muldoon Cc: gdb-patches ml Subject: Re: [patch][python] Add symbol, symbol table and frame block support to GDB API References: <4B66DA35.7080701@redhat.com> Reply-To: tromey@redhat.com Date: Thu, 04 Feb 2010 23:25:00 -0000 In-Reply-To: <4B66DA35.7080701@redhat.com> (Phil Muldoon's message of "Mon, 01 Feb 2010 13:42:13 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2010-02/txt/msg00148.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> This patch adds symbol, symbol table and block support to the Phil> Python GDB API. It also reconstitutes several functions missing Phil> from py-frame, and adds the glue back into python.c to make these Phil> code segments work together. This is for the most part a merge of Phil> the code that has existed in the archer repository for sometime. Phil> OK? Thanks for doing this. The code bits look mostly ok to me. There are a few formatting nits, of course ;-), but also a more substantial issue. Phil> +typedef struct { Phil> + PyObject_HEAD Phil> + struct block *block; Phil> +} block_object; When an objfile is destroyed, so are blocks coming from that objfile. However, this module doesn't take that into account. That means it can lead to crashes. One possible fix would be to have a notion of an "invalid" Block object, where the block field is ==NULL. Then each method of Block would check validity right away, throwing an error if it is invalid. Then, arrange to clear these fields when an objfile is destroyed. See py-type.c for an example of how the latter is done -- Types can't be invalidated this way but they are instead copied to the heap when the objfile disappears. This same problem affects symtabs and I suppose sals and block iterators as well, by extension. Phil> +static PyObject * Phil> +blpy_get_superblock (PyObject *self, void *closure) Phil> +{ [...] Phil> +PyObject * Phil> +block_to_block_object (struct block *block) Phil> +{ I'm sorry to foist this off on you, but I think exported functions like block_to_block_object should have header comments. Functions like blpy_get_superblock, which are just methods, don't need comments (IMO). Phil> +static PyObject * Phil> +blpy_block_syms_iter (PyObject *self) Phil> +{ Phil> + return self; Does this need a Py_INCREF? Phil> + return (sym == NULL)? NULL : symbol_to_symbol_object (sym); Space before the "?" Phil> + ret = asprintf (&s, "symbol for %s", Phil> + SYMBOL_PRINT_NAME (((symbol_object *) self)->symbol)); Use xstrprintf instead. Phil> + return PyBool_FromLong (!SYMBOL_IS_ARGUMENT (self_sym->symbol) Phil> + && (class == LOC_LOCAL || class == LOC_REGISTER || class == LOC_STATIC Phil> + || class == LOC_COMPUTED || class == LOC_OPTIMIZED_OUT)); The indentation looks weird here. Phil> +PyObject *gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw) Newline before gdbpy_lookup_symbol. Phil> + ret_tuple = PyTuple_New (2); Phil> + if (!ret_tuple) Phil> + { Phil> + PyErr_SetString (PyExc_MemoryError, "Could not allocate tuple object."); Phil> + return NULL; Phil> + } If ret_tuple==NULL, you don't need to call PyErr_SetString, just return NULL; PyTuple_New will have set the error. Phil> + ret = asprintf (&s, "symbol table for %s", Phil> + ((symtab_object *) self)->symtab->filename); xstrprintf Phil> +static PyObject * Phil> +stpy_get_filename (PyObject *self, void *closure) Phil> +{ Phil> + symtab_object *self_symtab = (symtab_object *) self; Phil> + PyObject *str_obj; Phil> + Phil> + /* FIXME: Can symtab->filename really be NULL? */ Phil> + if (self_symtab->symtab->filename) Could you take a quick look through other parts of gdb and see if anything else does this check? I'd prefer we eliminate this FIXME before it goes in. Phil> + filename = (sal_obj->symtab == (symtab_object *) Py_None)? "" : Phil> + sal_obj->symtab->symtab->filename; Formatting looks weird. Phil> + ret = asprintf (&s, "symbol and line for %s, line %d", filename, Phil> + sal_obj->sal->line); xstrprintf Phil> + sal_obj->sal = (struct symtab_and_line *) Phil> + xmalloc (sizeof (struct symtab_and_line)); Phil> + *(sal_obj->sal) = sal; This assignment looks weird; just use xmemdup instead. Tom