From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30951 invoked by alias); 23 Oct 2013 20:46:06 -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 30937 invoked by uid 89); 23 Oct 2013 20:46:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 23 Oct 2013 20:46:04 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9NKk3dA012157 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 23 Oct 2013 16:46:03 -0400 Received: from barimba (ovpn-113-54.phx2.redhat.com [10.3.113.54]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9NKk1Dn006441 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 23 Oct 2013 16:46:02 -0400 From: Tom Tromey To: Phil Muldoon Cc: gdb-patches@sourceware.org Subject: Re: [patch][python] 1/3 Python representation of GDB line tables (Python code) References: <5253F4D4.1050600@redhat.com> Date: Wed, 23 Oct 2013 20:46:00 -0000 In-Reply-To: <5253F4D4.1050600@redhat.com> (Phil Muldoon's message of "Tue, 08 Oct 2013 13:04:36 +0100") Message-ID: <87ob6fd8c7.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-10/txt/msg00734.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> This patch series (three emails) introduces GDB line table representation Phil> to Python. Phil> This first email contains the GDB and Python code to represent the line Phil> table data as Python objects. Phil> What do you think? Thanks, Phil. Really just nits here. Phil> +typedef struct { Phil> + PyObject_HEAD Phil> + /* The symtab python object. We store the Python object here as the Phil> + underlying symtab can become invalid, and we have to run validity Phil> + checks on it. */ Phil> + PyObject *symtab; The formatting is wrong in this comment. Subsequent lines should be indented. Phil> + when an object file has been freed. Extract from this the symtab Phil> + in a lazy fashion. */ This sentence reads strangely. Phil> +static PyObject * Phil> +get_symtab (PyObject *linetable) Phil> +{ Phil> + linetable_object *lt = (linetable_object *) linetable; Phil> + return lt->symtab; Empty line between declarations and code. Phil> + return (PyObject *)ltable; Space after close paren. Phil> +static PyObject * Phil> +build_tuple_from_vector (gdb_py_longest line, VEC (CORE_ADDR) *vec) Phil> +{ Phil> + int vec_len = 0; Phil> + PyObject *tuple; Phil> + CORE_ADDR pc; Phil> + int i; Phil> + volatile struct gdb_exception except; Phil> + Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + vec_len = VEC_length (CORE_ADDR, vec); Phil> + } Phil> + GDB_PY_HANDLE_EXCEPTION (except); VEC_length cannot throw, so the exception handling isn't needed here. Phil> + if (vec_len < 1) Phil> + return Py_None; Py_RETURN_NONE Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { I don't think anything in this loop can throw, either. You can rely on VEC_iterate not throwing. So I think this TRY_CATCH can be removed as well. Phil> + else if (PyTuple_SetItem (tuple, i, obj) != 0) Phil> + { Phil> + Py_DECREF (obj); Phil> + Py_XDECREF (tuple); I don't think you need Py_XDECREF here. Just Py_DECREF will do. Using Py_XDECREF isn't harmful, but is maybe mildly confusing as it implies that tuple could possibly be NULL at this point. Phil> +static PyObject * Phil> +ltpy_get_pcs_for_line (PyObject *self, PyObject *args) ... Phil> + gdb_py_longest py_line; ... Phil> + if (! PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &py_line)) Phil> + return NULL; I think this should use GDB_PY_LL_ARG instead, as py_line is signed. This function never frees the 'pcs' VEC. Phil> +static PyObject * Phil> +ltpy_has_pcs (PyObject *self, PyObject *args) ... Phil> + if (! PyArg_ParseTuple (args, GDB_PY_LLU_ARG, &py_line)) Phil> + return NULL; Ditto for GDB_PY_LL_ARG. Phil> + for (index = 0; index <= symtab->linetable->nitems; index++) I think it's preferred to use the LINETABLE accessor. This function must account for the possibility that symtab->linetable is NULL. Phil> + { Phil> + struct linetable_entry *item = &(symtab->linetable->item[index]); Accessor. Phil> +static PyObject * Phil> +ltpy_get_all_source_lines (PyObject *self, PyObject *args) Phil> +{ ... Phil> + for (index = 0; index <= symtab->linetable->nitems; index++) Accessor plus NULL check. Phil> + item = &(symtab->linetable->item[index]); Accessor. Phil> + /* Return a frozen set to remove duplicated line numbers in the case Phil> + where a source line has more than one instruction and more than one Phil> + entry in the line table. */ Phil> + fset = PyFrozenSet_New (source_list); Phil> + Py_DECREF (source_list); PyFrozenSet_New is new in 2.5. I think it's cheaper, and no more difficult, to just build a set object from the start rather than make a list and the convert it. I'm not sure what to about Python 2.4 though. Phil> +static void Phil> +ltpy_dealloc (PyObject *self) Need some header comment. Phil> +{ Phil> + linetable_object *obj = (linetable_object *) self; Phil> + Phil> + Py_DECREF (obj->symtab); Phil> + Py_TYPE (self)->tp_free (self); Phil> + return; Don't need that return. Phil> +int Phil> +gdbpy_initialize_linetable (void) Phil> +{ Header comment. Phil> +static PyObject * Phil> +ltpy_entry_get_line (PyObject *self, void *closure) Phil> +{ Phil> + linetable_entry_object *obj = (linetable_entry_object *) self; Phil> + return PyLong_FromUnsignedLongLong (obj->line); Phil> +} Phil> + Blank line between declarations and code. Phil> +/* Implementation of gdb.LineTableEntry.pc (self) -> Unsigned Long. Phil> + Returns an unsigned long associated with the PC of the line table Phil> + entry. */ Phil> + Phil> +static PyObject * Phil> +ltpy_entry_get_pcs (PyObject *self, void *closure) Phil> +{ Phil> + linetable_entry_object *obj = (linetable_entry_object *) self; Phil> + return PyLong_FromUnsignedLongLong (obj->pc); Phil> +} Phil> + Blank line between declarations and code. Phil> +static PyObject * Phil> +ltpy_iter (PyObject *self) ... Phil> + if (ltpy_iter_obj == NULL) Phil> + return NULL; Wrong indentation. Phil> +static void Phil> +ltpy_iterator_dealloc (PyObject *obj) Header comment. Phil> +{ Phil> + ltpy_iterator_object *iter_obj = (ltpy_iterator_object *) obj; Phil> + Phil> + Py_XDECREF (iter_obj->source); Can this ever be NULL? If not, s/X//. If so -- how? Phil> +static PyObject * Phil> +ltpy_iternext (PyObject *self) Phil> +{ ... Phil> + if (iter_obj->current_line >= symtab->linetable->nitems) Phil> + goto stop_iteration; Phil> + Phil> + item = &(symtab->linetable->item[iter_obj->current_line]); Accessor in two spots. "current_line" seems to be a misnomer as the value actually seems to index into the line table. So it should be "current_index" or something along those lines. Phil> + if (iter_obj->current_line >= symtab->linetable->nitems) Phil> + goto stop_iteration; Phil> + item = &(symtab->linetable->item[iter_obj->current_line]); Accessors. Phil> +static PyObject * Phil> +ltpy_iter_is_valid (PyObject *self, PyObject *args) Phil> +{ Phil> + struct symtab *symtab = NULL; Phil> + ltpy_iterator_object *iter_obj = (ltpy_iterator_object *) self; Phil> + Phil> + LTPY_REQUIRE_VALID (iter_obj->source, symtab); It's wrong to use LTPY_REQUIRE_VALID here. This function shouldn't throw an exception if the underling object is invalid; it should just return False. Phil> +static PyMethodDef linetable_object_methods[] = { I think a line table should also have an is_valid method. Phil> static PyObject * Phil> +stpy_get_linetable (PyObject *self, PyObject *args) Phil> +{ Header comment. Tom