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

[Tom Tromey <tromey@redhat.com>]

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