From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22157 invoked by alias); 18 Feb 2010 02:50:08 -0000 Received: (qmail 22140 invoked by uid 22791); 18 Feb 2010 02:50:05 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,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, 18 Feb 2010 02:50:01 +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.13.8/8.13.8) with ESMTP id o1I2nw9X003230 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Feb 2010 21:49:59 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o1I2nwNt015813; Wed, 17 Feb 2010 21:49: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 o1I2nuqO025036; Wed, 17 Feb 2010 21:49:57 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 80E6E37818E; Wed, 17 Feb 2010 19:49:56 -0700 (MST) From: Tom Tromey To: Phil Muldoon Cc: Eli Zaretskii , gdb-patches ml Subject: Re: [patch][python] Add symbol, symbol table and frame block support to GDB API References: <4B66DA35.7080701@redhat.com> <4B796506.7010909@redhat.com> Reply-To: tromey@redhat.com Date: Thu, 18 Feb 2010 02:50:00 -0000 In-Reply-To: <4B796506.7010909@redhat.com> (Phil Muldoon's message of "Mon, 15 Feb 2010 15:15:18 +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/msg00449.txt.bz2 Phil> I've added life-cycle support for symbols, symbol tables, symbol tables Phil> and line, and blocks. This was a little more involved than I first Phil> thought. I've also added the other changes you requested Thanks for doing this. I think this could use a NEWS entry. I suggest waiting until my earlier patch is approved, then adding items to that block... assuming your patch goes in before 7.1. Otherwise, add a new Python block in the post-7.1 changes. Phil> +/* Require a valid block. All access to block_object->block should be Phil> + gated by this call. This must be called inside a TRY_CATCH, or Phil> + another context in which a gdb exception is allowed. */ Phil> +#define BLPY_REQUIRE_VALID(block_obj, block) \ Phil> + do { \ Phil> + block = block_object_to_block (block_obj); \ Phil> + if (block == NULL) \ Phil> + error (_("Block is invalid.")); \ Phil> + } while (0) All the uses of this macro appear in a TRY_CATCH, which is then followed by GDB_PY_HANDLE_EXCEPTION. That is quite roundabout; it would be better to bypass the middle man and just set the Python exception directly and `return NULL'. Phil> +static PyObject * Phil> +blpy_block_syms_iternext (PyObject *self) [...] Phil> + return (sym == NULL) ? NULL : symbol_to_symbol_object (sym); If you return NULL you must set the Python exception. I think in this case you want to raise StopIteration. Phil> +/* Require a valid symbol. All access to symbol_object->symbol should be Phil> + gated by this call. This must be called inside a TRY_CATCH, or Phil> + another context in which a gdb exception is allowed. */ Phil> +#define SYMPY_REQUIRE_VALID(symbol_obj, symbol) \ Phil> + do { \ Phil> + symbol = symbol_object_to_symbol (symbol_obj); \ Phil> + if (symbol == NULL) \ Phil> + error (_("Symbol is invalid.")); \ Phil> + } while (0) The comment that applied to BLPY_REQUIRE_VALID applies here. Phil> +static PyObject * Phil> +sympy_str (PyObject *self) Phil> +{ Phil> + char *s; Phil> + PyObject *result; Phil> + struct symbol *symbol = NULL; Phil> + volatile struct gdb_exception except; Phil> + Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + SYMPY_REQUIRE_VALID (self, symbol); Phil> + } Phil> + GDB_PY_HANDLE_EXCEPTION (except); Phil> + Phil> + s = xstrprintf ("symbol for %s", Phil> + SYMBOL_PRINT_NAME (symbol)); Why not just have this be SYMBOL_PRINT_NAME? I forgot the difference between the various python to-string methods. Maybe this one is supposed to be more verbose? Let me know. Phil> + obj->next = objfile_data (symbol->symtab->objfile, sympy_objfile_data_key); Use the SYMBOL_SYMTAB macro here, and anywhere you have to refer to the symbol's symtab. Phil> + set_objfile_data (sym_obj->symbol->symtab->objfile, sympy_objfile_data_key, sym_obj->next); This wraps, I think a couple of other lines nearby do too. Line break after a comma somewhere. Phil> +/* Require a valid symbol table. All access to symtab_object->symtab Phil> + should be gated by this call. This must be called inside a Phil> + TRY_CATCH, or another context in which a gdb exception is Phil> + allowed. */ Phil> +#define STPY_REQUIRE_VALID(symtab_obj, symtab) \ Phil> + do { \ Phil> + symtab = symtab_object_to_symtab (symtab_obj); \ Phil> + if (symtab == NULL) \ Phil> + error (_("Symbol table is invalid.")); \ Phil> + } while (0) As above. Also SALPY_REQUIRE_VALID. Phil> + s = xstrprintf ("symbol table for %s", Phil> + symtab->filename); Also as above. Phil> + result = objfile_to_objfile_object (symtab->objfile); Phil> + Py_INCREF (result); Phil> + return result; objfile_to_objfile_object can return NULL, which would make that incref crash. Phil> + s = xstrprintf ("symbol and line for %s, line %d", filename, Phil> + sal->line); As above. Phil> +static void Phil> +set_sal (sal_object *sal_obj, struct symtab_and_line sal) Phil> +{ Phil> + Spurious blank line. Phil> + if (symtab_obj == NULL) Phil> + { Phil> + Py_DECREF (sal_obj); Phil> + return; Phil> + } This does not make sense to me. This destroys the sal_obj but the caller then proceeds to return it. Phil> + sizeof(struct symtab_and_line)); Space before paren. Phil> +PyObject * Phil> +symtab_and_line_to_sal_object (struct symtab_and_line sal) Phil> + Phil> +{ Phil> + sal_object *sal_obj; Phil> + symtab_object *symtab_obj; Phil> + Phil> + sal_obj = PyObject_New (sal_object, &sal_object_type); Phil> + Phil> + if (sal_obj) Phil> + set_sal (sal_obj, sal); Phil> + Phil> + return (PyObject *) sal_obj; Here's the bad return. If set_sal destroy sal_obj, gdb will crash. Phil> +static PyMethodDef symtab_object_methods[] = { Phil> + { "fullname", stpy_fullname, METH_NOARGS, Phil> + "Return the symtab's full source filename." }, For methods we're trying to put the type info into the help. I forgot to check the other new methods in this patch, but please do this. See the existing python.c for examples. Phil> "lookup_type (name [, block]) -> type\n\ Phil> Return a Type corresponding to the given name." }, It is funny that this "[, block]" help is still there. I must have forgot to remove this when I stripped the block stuff from a patch I upstreamed. Anyway, I was going to mention this ... could you put the block code back into lookup_type? It is fine by me if you'd rather do this as a second patch. There may be some other block-related addition on the branch that should go in now. I forget. thanks, Tom