Re: [patch][python] Add symbol, symbol table and frame block support to GDB API

[Tom Tromey <tromey@redhat.com>]

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