Re: [PATCH][gdb/python] Add interface to access minimal_symbols

[Tom Tromey <tom@tromey.com>]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This patch adds a new gdb.MinSymbol object to export the minimal_symbol
Tom> interface.

I hate to bikeshed the name to start with, but I'm going to anyway.  Why
the abbreviated name rather than gdb.MinimalSymbol?  Or alternatively
why not something less "gdb-ish", like gdb.LinkerSymbol?

Tom> +Typical symbols like functions, variables, etc are represented by
Tom> +@code{gdb.Symbol} objects in Python.  Some symbols are defined with less
Tom> +information associated with them, like linker script variables
Tom> +or assembly labels.  Python represents these minimal symbols in @value{GDBN}
Tom> +with the @code{gdb.MinSymbol} object.

I think this is mildly misleading and perhaps should also mention that
linker symbols will generally end up in here.

Tom> +@defvar MinSymbol.filename
Tom> +The file name of the source file where the minimal symbol is defined.

As mentioned elsewhere in the thread, I think this field is largely not
useful.  I feel like we once considered removing it from minsyms
entirely.  So, I tend to think this one should just be dropped, unless
you have some specific need for it (which would be interesting to hear
about).

Tom> +@defvar MinSymbol.section_name
Tom> +The name of the section in the object file containing this minimal symbol.

Are there platforms where sections do not have names?
What will happen to this on those?

Tom> +  return PyString_FromString (minsym->filename);

I suspect filename can be NULL here.
But see above.

Tom> +  if (section != NULL)
Tom> +    {
Tom> +      name = bfd_section_name (objfile->obfd, section->the_bfd_section);
Tom> +      if (name != NULL)
Tom> +	return PyString_FromString (name);
Tom> +    }
Tom> +
Tom> +  Py_RETURN_NONE;

Ok, I see the answer to my question here.  This should be documented.

Tom> +  type = builtin_type (minsym_gdbarch (self))->builtin_func_ptr;
Tom> +
Tom> +  if (minsym_type (self) == type)
Tom> +    Py_RETURN_TRUE;

This seems kind of roundabout to me.

Tom> +  type = builtin_type (minsym_gdbarch (self))->builtin_data_ptr;
Tom> +
Tom> +  if (minsym_type (self) == type)
Tom> +    Py_RETURN_TRUE;

Same here.

Tom> +  minsym_object *minsym_obj = (minsym_object *)self;

Space after the ")".

Tom> +static void
Tom> +set_symbol (minsym_object *obj, struct bound_minimal_symbol *bound)

I think a const reference for "bound" would be more natural here.

Tom> +static PyObject *
Tom> +bound_minsym_to_minsym_object (struct bound_minimal_symbol *bound)

Here too.

Tom> +  if (sfile_obj != NULL && sfile_obj != Py_None)
Tom> +    {
Tom> +      sfile_tmp = gdbpy_obj_to_string (sfile_obj);

If this returns NULL then this function should early-return as well.

Tom> +  if (bound_minsym.minsym != NULL)
Tom> +    msym_obj = bound_minsym_to_minsym_object (&bound_minsym);

If bound_minsym_to_minsym_object returns NULL, then an exception has
been set, so this function must return NULL.  But:

Tom> +  if (msym_obj != NULL)
Tom> +    return msym_obj;

... that isn't happening.

So the logic needs a bit of reordering here.

Tom> +/* Initialize gdb.MinSymbol.  Return -1 on error, 0 on success.  */
Tom> +
Tom> +int
Tom> +gdbpy_initialize_minsymbols (void)

Don't need the "void" here any more.

Tom> +/* Return struct objfile reference that is wrapped by the SELF object.  */
Tom> +
Tom> +struct objfile *
Tom> +objectfile_object_to_objfile (PyObject *self)

I think this should be named objfile_object_to_objfile instead.

Tom> +++ b/gdb/testsuite/gdb.python/py-minsymbol.exp

Some of the lines here are too long and can reasonably be split.

I think it would also make sense to check for the minsym for "main"
itself.

Tom