Re: [patch][python] 1/3 Python representation of GDB line tables (Python code)

[Tom Tromey <tromey@redhat.com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> 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