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

Tom Tromey
2010-02-18T02:50:08Z

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