Re: [PATCH 6/7] py-minsymbol: Add interface to access minimal_symbols

[Phil Muldoon <pmuldoon@redhat.com>]

On 04/02/16 17:29, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
>
> This patch adds a new gdb.MinSymbol object to export the minimal_symbol
> interface.  These objects can be resolved using a new
> gdb.lookup_minimal_symbol call.
> ---
>  gdb/Makefile.in              |   6 +
>  gdb/python/py-minsymbol.c    | 390 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |   6 +
>  gdb/python/python.c          |   5 +
>  4 files changed, 407 insertions(+)
>  create mode 100644 gdb/python/py-minsymbol.c
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 3eadbbc..c2f7eef 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -400,6 +400,7 @@ SUBDIR_PYTHON_OBS = \
>  	py-infthread.o \
>  	py-lazy-string.o \
>  	py-linetable.o \
> +	py-minsymbol.o \
>  	py-newobjfileevent.o \
>  	py-objfile.o \
>  	py-param.o \
> @@ -440,6 +441,7 @@ SUBDIR_PYTHON_SRCS = \
>  	python/py-infthread.c \
>  	python/py-lazy-string.c \
>  	python/py-linetable.c \
> +	python/py-minsymbol.c \
>  	python/py-newobjfileevent.c \
>  	python/py-objfile.c \
>  	python/py-param.c \
> @@ -2635,6 +2637,10 @@ py-linetable.o: $(srcdir)/python/py-linetable.c
>  	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-linetable.c
>  	$(POSTCOMPILE)
>  
> +py-minsymbol.o: $(srcdir)/python/py-minsymbol.c
> +	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-minsymbol.c
> +	$(POSTCOMPILE)
> +
>  py-newobjfileevent.o: $(srcdir)/python/py-newobjfileevent.c
>  	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-newobjfileevent.c
>  	$(POSTCOMPILE)
> diff --git a/gdb/python/py-minsymbol.c b/gdb/python/py-minsymbol.c
> new file mode 100644
> index 0000000..d8b56bc
> --- /dev/null
> +++ b/gdb/python/py-minsymbol.c
> @@ -0,0 +1,390 @@
> +/* Python interface to minsymbols.
> +
> +   Copyright (C) 2008-2013 Free Software Foundation, Inc.
Wrong Copyright date.

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "block.h"
> +#include "exceptions.h"
> +#include "frame.h"
> +#include "symtab.h"
> +#include "python-internal.h"
> +#include "objfiles.h"
> +#include "value.h"
> +
> +typedef struct msympy_symbol_object {
> +  PyObject_HEAD
> +  /* The GDB minimal_symbol structure this object is wrapping.  */
> +  struct minimal_symbol *minsym;
> +  struct objfile *objfile;
> +
> +  struct type *type;
> +  /* A symbol object is associated with an objfile, so keep track with
> +     doubly-linked list, rooted in the objfile.  This lets us
> +     invalidate the underlying struct minimal_symbol when the objfile is
> +     deleted.  */
> +  struct msympy_symbol_object *prev;
> +  struct msympy_symbol_object *next;
> +} minsym_object;
Can you please document all members of a struct here.
> +/* Return the symbol that is wrapped by this symbol object.  */
> +static struct minimal_symbol *
> +minsym_object_to_minsym (PyObject *obj)
> +{
> +  if (! PyObject_TypeCheck (obj, &minsym_object_type))
> +    return NULL;
> +  return ((minsym_object *) obj)->minsym;
> +}
> +
> +static struct objfile *
> +minsym_object_to_objfile (PyObject *obj)
> +{
> +  if (! PyObject_TypeCheck (obj, &minsym_object_type))
> +    return NULL;
> +  return ((minsym_object *) obj)->objfile;
> +}
> +
> +/* Require a valid symbol.  All access to minsym_object->symbol should be
> +   gated by this call.  */
> +#define MSYMPY_REQUIRE_VALID(minsym_obj, minsym)	\
> +  do {							\
> +    minsym = minsym_object_to_minsym (minsym_obj);	\
> +    if (minsym == NULL)				\
> +      {							\
> +	PyErr_SetString (PyExc_RuntimeError,		\
> +			 _("MinSymbol is invalid."));	\
> +	return NULL;					\
> +      }							\
> +  } while (0)
> +
> +#define MSYMPY_REQUIRE_VALID_BOUND(minsym_obj, minsym, objfile)	\
> +  do {								\
> +    minsym = minsym_object_to_minsym (minsym_obj);		\
> +    objfile = minsym_object_to_objfile (minsym_obj);		\
> +    if (minsym == NULL || objfile == NULL)			\
> +      {								\
> +	PyErr_SetString (PyExc_RuntimeError,			\
> +			 _("MinSymbol is invalid."));		\
> +	return NULL;						\
> +      }								\
> +  } while (0)
> +
> +static PyObject *
> +msympy_str (PyObject *self)
> +{
> +  PyObject *result;
> +  struct minimal_symbol *minsym = NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +
> +  result = PyString_FromString (MSYMBOL_PRINT_NAME (minsym));
> +
> +  return result;
> +}
> +
> +static PyObject *
> +msympy_get_name (PyObject *self, void *closure)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +
> +  return PyString_FromString (MSYMBOL_NATURAL_NAME (minsym));
> +}
> +
> +static PyObject *
> +msympy_get_file_name (PyObject *self, void *closure)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +
> +  return PyString_FromString (minsym->filename);
> +}
> +
> +static PyObject *
> +msympy_get_linkage_name (PyObject *self, void *closure)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +
> +  return PyString_FromString (MSYMBOL_LINKAGE_NAME (minsym));
> +}
> +
> +static PyObject *
> +msympy_get_print_name (PyObject *self, void *closure)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +
> +  return msympy_str (self);
> +}
> +
> +static PyObject *
> +msympy_get_section (PyObject *self, void *closure)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +  struct objfile *objfile = NULL;
> +  struct obj_section *section;
> +  const char *name;
> +
> +  MSYMPY_REQUIRE_VALID_BOUND (self, minsym, objfile);
> +
> +  section = MSYMBOL_OBJ_SECTION (objfile, minsym);
> +  if (section) 
The { on a newline.
> {
> +    name = bfd_section_name (objfile->obfd, section->the_bfd_section);
> +    if (name)
> +      return PyString_FromString (name);
> +  }
> +
> +  Py_RETURN_NONE;
> +
> +}
> +
> +static PyObject *
> +msympy_is_valid (PyObject *self, PyObject *args)
> +{
> +  struct minimal_symbol *minsym = NULL;
> +
> +  minsym = minsym_object_to_minsym (self);
> +  if (minsym == NULL)
> +    Py_RETURN_FALSE;
> +
> +  Py_RETURN_TRUE;
> +}
> +
> +/* Implementation of gdb.MinSymbol.value (self) -> gdb.Value.  Returns
> +   the value of the symbol, or an error in various circumstances.  */
> +
> +static PyObject *
> +msympy_value (PyObject *self, PyObject *args)
> +{
> +  minsym_object *minsym_obj = (minsym_object *)self;
> +  struct minimal_symbol *minsym = NULL;
> +  struct value *value = NULL;
> +
> +  if (!PyArg_ParseTuple (args, ""))
> +    return NULL;
> +
> +  MSYMPY_REQUIRE_VALID (self, minsym);
> +  TRY
> +    {
> +      value = value_from_ulongest(minsym_obj->type,
> +				  MSYMBOL_VALUE_RAW_ADDRESS(minsym));
Space after function name and before (. This and other places.

The patch series, in principle, looks fine to me other than the nits I've highlighted.

However it needs tests for the new functionality.  It would be helpful if you could indicate if you have run the testsuite
and checked for regressions.

It also needs documentation additions and changes, and a documentation review.

Also please wait for a maintainer to sign off on it, too

Cheers

Phil