Re: [PATCH 02/11] Generalize varobj iterator

[Keith Seitz <keiths at redhat dot com>]

On 11/23/2013 06:09 PM, Yao Qi wrote:
> This patch generalizes varobj iterator, in a python-independent way.
> Note varobj_item is still a typedef of PyObject, we can only focus on
> API changes, and leave the data type changes to the next patch.  As a
> result, we include "varobj-iter.h" after the typedef of PyObject in
> varobj.c, but it is an intermediate state.  Finally, varobj-iter.h is
> independent of PyObject.

This looks good to me with the trivial problems mentioned corrected.

> gdb:
>
> 2013-11-24  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
>
> 	* Makefile.in (SUBDIR_PYTHON_OBS): Add "py-varobj.o".
> 	(SUBDIR_PYTHON_SRCS): Add "python/py-varobj.c".
> 	(HFILES_NO_SRCDIR): Add "varobj-iter.h".
> 	(py-varobj.o): New rule.
> 	* python/py-varobj.c: New file.
> 	* python/python-internal.h (py_varobj_get_iterator): Declare.
> 	* varobj-iter.h: New file.
> 	* varobj.c: Include "varobj-iter.h"
                                           ^
Missing '.'

> 	(struct varobj) <child_iter>: Change its type from "PyObject *"
> 	to "struct varobj_iter *".
> 	<saved_item>: Likewise.
> 	[HAVE_PYTHON] (varobj_ensure_python_env): Make it extern.
> 	[HAVE_PYTHON] (varobj_get_iterator): New function.
> 	(update_dynamic_varobj_children) [HAVE_PYTHON]: Move
> 	python-specific code to python/py-varobj.c.
> 	(install_visualizer): Call varobj_iter_delete instead of
> 	Py_XDECREF.
> 	* varobj.h (varobj_ensure_python_env): Declare.
> ---
>   gdb/Makefile.in              |   13 +++-
>   gdb/python/py-varobj.c       |  183 ++++++++++++++++++++++++++++++++++++++++++
>   gdb/python/python-internal.h |    4 +
>   gdb/varobj-iter.h            |   62 ++++++++++++++
>   gdb/varobj.c                 |  111 ++++++++------------------
>   gdb/varobj.h                 |    2 +
>   6 files changed, 295 insertions(+), 80 deletions(-)
>   create mode 100644 gdb/python/py-varobj.c
>   create mode 100644 gdb/varobj-iter.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 0591279..7d8a365 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -312,7 +312,8 @@ SUBDIR_PYTHON_OBS = \
>   	py-threadevent.o \
>   	py-type.o \
>   	py-utils.o \
> -	py-value.o
> +	py-value.o \
> +	py-varobj.o
>
>   SUBDIR_PYTHON_SRCS = \
>   	python/python.c \
> @@ -348,7 +349,8 @@ SUBDIR_PYTHON_SRCS = \
>   	python/py-threadevent.c \
>   	python/py-type.c \
>   	python/py-utils.c \
> -	python/py-value.c
> +	python/py-value.c \
> +	python/py-varobj.c
>   SUBDIR_PYTHON_DEPS =
>   SUBDIR_PYTHON_LDFLAGS=
>   SUBDIR_PYTHON_CFLAGS=
> @@ -797,7 +799,8 @@ proc-utils.h aarch64-tdep.h arm-tdep.h ax-gdb.h ppcfbsd-tdep.h \
>   ppcnbsd-tdep.h cli-out.h gdb_expat.h breakpoint.h infcall.h obsd-tdep.h \
>   exec.h m32r-tdep.h osabi.h gdbcore.h solib-som.h amd64bsd-nat.h \
>   i386bsd-nat.h xml-support.h xml-tdesc.h alphabsd-tdep.h gdb_obstack.h \
> -ia64-tdep.h ada-lang.h varobj.h frv-tdep.h nto-tdep.h serial.h \
> +ia64-tdep.h ada-lang.h ada-varobj.h varobj.h varobj-iter.h frv-tdep.h \
> +nto-tdep.h serial.h \
>   c-lang.h d-lang.h go-lang.h frame.h event-loop.h block.h cli/cli-setshow.h \
>   cli/cli-decode.h cli/cli-cmds.h cli/cli-utils.h \
>   cli/cli-script.h macrotab.h symtab.h common/version.h \
> @@ -2285,6 +2288,10 @@ py-value.o: $(srcdir)/python/py-value.c
>   	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-value.c
>   	$(POSTCOMPILE)
>
> +py-varobj.o: $(srcdir)/python/py-varobj.c
> +	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-varobj.c
> +	$(POSTCOMPILE)
> +
>   #
>   # Dependency tracking.  Most of this is conditional on GNU Make being
>   # found by configure; if GNU Make is not found, we fall back to a
> diff --git a/gdb/python/py-varobj.c b/gdb/python/py-varobj.c
> new file mode 100644
> index 0000000..6588fc2
> --- /dev/null
> +++ b/gdb/python/py-varobj.c
> @@ -0,0 +1,183 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +   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 "python-internal.h"
> +#include "varobj.h"
> +#include "varobj-iter.h"
> +
> +/* A dynamic varobj iterator "class" for python pretty-printed
> +   varobjs.  This inherits struct varobj_iter.  */
> +
> +struct py_varobj_iter
> +{
> +  /* The 'base class'.  */
> +  struct varobj_iter base;
> +
> +  /* The python iterator returned by the printer's 'children' method,
> +     or NULL if not available.  */
> +  PyObject *iter;
> +};
> +
> +/* Implementation of the 'dtor' method of pretty-printed varobj
> +   iterators.  */
> +
> +static void
> +py_varobj_iter_dtor (struct varobj_iter *self)
> +{
> +  struct py_varobj_iter *dis = (struct py_varobj_iter *) self;
> +
> +  Py_XDECREF (dis->iter);
> +}
> +
> +/* Implementation of the 'next' method of pretty-pretty varobj
                                             ^^^^^^^^^^^^^

"pretty-printed" :-)

> +   iterators.  */
> +
> +static varobj_item *
> +py_varobj_iter_next (struct varobj_iter *self)
> +{
> +  struct py_varobj_iter *t = (struct py_varobj_iter *) self;
> +  struct cleanup *back_to;
> +  PyObject *item;
> +
> +  back_to = varobj_ensure_python_env (self->var);
> +
> +  item = PyIter_Next (t->iter);
> +
> +  if (item == NULL)
> +    {
> +      /* Normal end of iteration.  */
> +      if (!PyErr_Occurred ())
> +	return NULL;
> +
> +      /* If we got a memory error, just use the text as the item.  */
> +      if (PyErr_ExceptionMatches (gdbpy_gdb_memory_error))
> +	{
> +	  PyObject *type, *value, *trace;
> +	  char *name_str, *value_str;
> +
> +	  PyErr_Fetch (&type, &value, &trace);
> +	  value_str = gdbpy_exception_to_string (type, value);
> +	  Py_XDECREF (type);
> +	  Py_XDECREF (value);
> +	  Py_XDECREF (trace);
> +	  if (value_str == NULL)
> +	    {
> +	      gdbpy_print_stack ();
> +	      return NULL;
> +	    }
> +
> +	  name_str = xstrprintf ("<error at %d>",
> +				 self->next_raw_index++);
> +	  item = Py_BuildValue ("(ss)", name_str, value_str);
> +	  xfree (name_str);
> +	  xfree (value_str);
> +	  if (item == NULL)
> +	    {
> +	      gdbpy_print_stack ();
> +	      return NULL;
> +	    }
> +	}
> +      else
> +	{
> +	  /* Any other kind of error.  */
> +	  gdbpy_print_stack ();
> +	  return NULL;
> +	}
> +    }
> +
> +  self->next_raw_index++;
> +  do_cleanups (back_to);
> +  return item;
> +}
> +
> +/* The 'vtable' of pretty-printed python varobj iterators.  */
> +
> +static const struct varobj_iter_ops py_varobj_iter_ops =
> +{
> +  py_varobj_iter_dtor,
> +  py_varobj_iter_next
> +};
> +
> +/* Constructor of pretty-printed varobj iterators.  VAR is the varobj
> +   whose children the iterator will be iterating over.  PYITER is the
> +   python iterator actually responsible for the iteration.  */
> +
> +static void
> +py_varobj_iter_ctor (struct py_varobj_iter *self,
> +		      struct varobj *var, PyObject *pyiter)
> +{
> +  self->base.var = var;
> +  self->base.ops = &py_varobj_iter_ops;
> +  self->base.next_raw_index = 0;
> +  self->iter = pyiter;
> +}
> +
> +/* Allocate and construct a pretty-printed varobj iterator.  VAR is
> +   the varobj whose children the iterator will be iterating over.
> +   PYITER is the python iterator actually responsible for the
> +   iteration.  */
> +
> +static struct py_varobj_iter *
> +py_varobj_iter_new (struct varobj *var, PyObject *pyiter)
> +{
> +  struct py_varobj_iter *self;
> +
> +  self = xmalloc (sizeof (struct py_varobj_iter));

You can use "XNEW (struct py_varobj_iter)" as a shorthand for this.

> +  py_varobj_iter_ctor (self, var, pyiter);
> +  return self;
> +}
> +
> +/* Return a new pretty-printed varobj iterator suitable to iterate
> +   over VAR's children.  */
> +
> +struct varobj_iter *
> +py_varobj_get_iterator (struct varobj *var, PyObject *printer)
> +{
> +  PyObject *children;
> +  int i;
> +  PyObject *iter;
> +  struct py_varobj_iter *py_iter;
> +  struct cleanup *back_to = varobj_ensure_python_env (var);
> +
> +  if (!PyObject_HasAttr (printer, gdbpy_children_cst))
> +    {
> +      do_cleanups (back_to);
> +      return NULL;
> +    }
> +
> +  children = PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
> +					 NULL);
> +  if (children == NULL)
> +    {
> +      gdbpy_print_stack ();
> +      error (_("Null value returned for children"));
> +    }
> +
> +  make_cleanup_py_decref (children);
> +
> +  iter = PyObject_GetIter (children);
> + if (iter == NULL)
> +    {
> +      gdbpy_print_stack ();
> +      error (_("Could not get children iterator"));
> +    }
> +
> +  py_iter = py_varobj_iter_new (var, iter);
> +
> +  do_cleanups (back_to);
> +
> +  return &py_iter->base;
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 7d8c4ad..93f9c0a 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -494,4 +494,8 @@ int gdb_pymodule_addobject (PyObject *module, const char *name,
>   			    PyObject *object)
>     CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
>
> +struct varobj_iter;
> +struct varobj;
> +struct varobj_iter *py_varobj_get_iterator (struct varobj *var,
> +					    PyObject *printer);
>   #endif /* GDB_PYTHON_INTERNAL_H */
> diff --git a/gdb/varobj-iter.h b/gdb/varobj-iter.h
> new file mode 100644
> index 0000000..59be278
> --- /dev/null
> +++ b/gdb/varobj-iter.h
> @@ -0,0 +1,62 @@
> +/* Iterator of varobj.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +   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/>.  */
> +
> +struct varobj_iter_ops;
> +
> +typedef PyObject varobj_item;
> +
> +/* A dynamic varobj iterator "class".  */
> +
> +struct varobj_iter
> +{
> +  /* The 'vtable'.  */
> +  const struct varobj_iter_ops *ops;
> +
> +  /* The varobj this iterator is listing children for.  */
> +  struct varobj *var;
> +
> +  /* The next raw index we will try to check is available.  If is

"If it is" or "If this is"

> +     equal to number_of_children, then we've already iterated the
> +     whole set.  */
> +  int next_raw_index;
> +};
> +
> +/* The vtable of the varobj iterator class.  */

Missing newline

> +struct varobj_iter_ops
> +{
> +  /* Destructor.  Releases everything from SELF (but not SELF
> +     itself).  */
> +  void (*dtor) (struct varobj_iter *self);
> +
> +  /* Returns the next object or NULL if it has reached the end.  */
> +  varobj_item *(*next) (struct varobj_iter *self);
> +};
> +
> +/* Returns the next varobj or NULL if it has reached the end.  */
> +
> +#define varobj_iter_next(ITER)	(ITER)->ops->next (ITER)
> +
> +/* Delete a varobj_iter object.  */
> +
> +#define varobj_iter_delete(ITER)	       \
> +  do					       \
> +    {					       \
> +      if (ITER)			       \

I believe we are now requested an explicit check against NULL.

> +	{				       \
> +	  (ITER)->ops->dtor (ITER);	       \
> +	  xfree (ITER);		       \
> +	}				       \
> +    } while (0)
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 4a10617..7a8305b 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -41,6 +41,8 @@
>   typedef int PyObject;
>   #endif
>
> +#include "varobj-iter.h"
> +
>   /* Non-zero if we want to see trace of varobj level stuff.  */
>
>   unsigned int varobjdebug = 0;
> @@ -139,14 +141,14 @@ struct varobj_dynamic
>
>     /* The iterator returned by the printer's 'children' method, or NULL
>        if not available.  */
> -  PyObject *child_iter;
> +  struct varobj_iter *child_iter;
>
>     /* We request one extra item from the iterator, so that we can
>        report to the caller whether there are more items than we have
>        already reported.  However, we don't want to install this value
>        when we read it, because that will mess up future updates.  So,
>        we stash it here instead.  */
> -  PyObject *saved_item;
> +  varobj_item *saved_item;
>   };
>
>   struct cpstack
> @@ -255,7 +257,7 @@ is_root_p (struct varobj *var)
>   #ifdef HAVE_PYTHON
>   /* Helper function to install a Python environment suitable for
>      use during operations on VAR.  */
> -static struct cleanup *
> +struct cleanup *
>   varobj_ensure_python_env (struct varobj *var)
>   {
>     return ensure_python_env (var->root->exp->gdbarch,
> @@ -772,6 +774,19 @@ dynamic_varobj_has_child_method (struct varobj *var)
>     return result;
>   }
>
> +/* Dynamic varobj's iterator factory.  Returns an iterator object
> +   suitable for iterating over VAR's children.  */

This first bit is a tiny bit awkward. How about "A factory for creating 
dynamic varobj iterators." ?

> +
> +static struct varobj_iter *
> +varobj_get_iterator (struct varobj *var)
> +{
> +  if (var->dynamic->pretty_printer)
> +    return py_varobj_get_iterator (var, var->dynamic->pretty_printer);
> +
> +  gdb_assert_not_reached (_("\
> +requested an interator from a non-dynamic varobj"));
                ^^^^^^^^^
"iterator"

> +}
> +
>   #endif
>
>   static int
> @@ -787,9 +802,7 @@ update_dynamic_varobj_children (struct varobj *var,
>   {
>   #if HAVE_PYTHON
>     struct cleanup *back_to;
> -  PyObject *children;
>     int i;
> -  PyObject *printer = var->dynamic->pretty_printer;
>
>     if (!gdb_python_initialized)
>       return 0;
> @@ -797,37 +810,22 @@ update_dynamic_varobj_children (struct varobj *var,
>     back_to = varobj_ensure_python_env (var);
>
>     *cchanged = 0;
> -  if (!PyObject_HasAttr (printer, gdbpy_children_cst))
> -    {
> -      do_cleanups (back_to);
> -      return 0;
> -    }
>
>     if (update_children || var->dynamic->child_iter == NULL)
>       {
> -      children = PyObject_CallMethodObjArgs (printer, gdbpy_children_cst,
> -					     NULL);
> +      varobj_iter_delete (var->dynamic->child_iter);
> +      var->dynamic->child_iter = varobj_get_iterator (var);
>
> -      if (!children)
> -	{
> -	  gdbpy_print_stack ();
> -	  error (_("Null value returned for children"));
> -	}
> +      Py_XDECREF (var->dynamic->saved_item);
> +      var->dynamic->saved_item = NULL;
>
> -      make_cleanup_py_decref (children);
> +      i = 0;
>
> -      Py_XDECREF (var->dynamic->child_iter);
> -      var->dynamic->child_iter = PyObject_GetIter (children);
>         if (var->dynamic->child_iter == NULL)
>   	{
> -	  gdbpy_print_stack ();
> -	  error (_("Could not get children iterator"));
> +	  do_cleanups (back_to);
> +	  return 0;
>   	}
> -
> -      Py_XDECREF (var->dynamic->saved_item);
> -      var->dynamic->saved_item = NULL;
> -
> -      i = 0;
>       }
>     else
>       i = VEC_length (varobj_p, var->children);
> @@ -837,7 +835,6 @@ update_dynamic_varobj_children (struct varobj *var,
>     for (; to < 0 || i < to + 1; ++i)
>       {
>         PyObject *item;
> -      int force_done = 0;
>
>         /* See if there was a leftover from last time.  */
>         if (var->dynamic->saved_item)
> @@ -846,52 +843,17 @@ update_dynamic_varobj_children (struct varobj *var,
>   	  var->dynamic->saved_item = NULL;
>   	}
>         else
> -	item = PyIter_Next (var->dynamic->child_iter);
> -
> -      if (!item)
>   	{
> -	  /* Normal end of iteration.  */
> -	  if (!PyErr_Occurred ())
> -	    break;
> -
> -	  /* If we got a memory error, just use the text as the
> -	     item.  */
> -	  if (PyErr_ExceptionMatches (gdbpy_gdb_memory_error))
> -	    {
> -	      PyObject *type, *value, *trace;
> -	      char *name_str, *value_str;
> -
> -	      PyErr_Fetch (&type, &value, &trace);
> -	      value_str = gdbpy_exception_to_string (type, value);
> -	      Py_XDECREF (type);
> -	      Py_XDECREF (value);
> -	      Py_XDECREF (trace);
> -	      if (!value_str)
> -		{
> -		  gdbpy_print_stack ();
> -		  break;
> -		}
> -
> -	      name_str = xstrprintf ("<error at %d>", i);
> -	      item = Py_BuildValue ("(ss)", name_str, value_str);
> -	      xfree (name_str);
> -	      xfree (value_str);
> -	      if (!item)
> -		{
> -		  gdbpy_print_stack ();
> -		  break;
> -		}
> -
> -	      force_done = 1;
> -	    }
> -	  else
> -	    {
> -	      /* Any other kind of error.  */
> -	      gdbpy_print_stack ();
> -	      break;
> -	    }
> +	  item = varobj_iter_next (var->dynamic->child_iter);
>   	}
>
> +      if (item == NULL)
> +	{
> +	  /* Iteration is done.  Remove iterator from VAR.  */
> +	  varobj_iter_delete (var->dynamic->child_iter);
> +	  var->dynamic->child_iter = NULL;
> +	  break;
> +	}
>         /* We don't want to push the extra child on any report list.  */
>         if (to < 0 || i < to)
>   	{
> @@ -931,9 +893,6 @@ update_dynamic_varobj_children (struct varobj *var,
>   	     element.  */
>   	  break;
>   	}
> -
> -      if (force_done)
> -	break;
>       }
>
>     if (i < VEC_length (varobj_p, var->children))
> @@ -952,8 +911,6 @@ update_dynamic_varobj_children (struct varobj *var,
>       *cchanged = 1;
>
>     var->num_children = VEC_length (varobj_p, var->children);
> -
> -  do_cleanups (back_to);

Doesn't this cleanup still need to run?

>     return 1;
>   #else
> @@ -1244,7 +1201,7 @@ install_visualizer (struct varobj_dynamic *var, PyObject *constructor,
>     Py_XDECREF (var->pretty_printer);
>     var->pretty_printer = visualizer;
>
> -  Py_XDECREF (var->child_iter);
> +  varobj_iter_delete (var->child_iter);
>     var->child_iter = NULL;
>   }
>
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 978d9b9..0f68311 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -308,6 +308,8 @@ extern int varobj_has_more (struct varobj *var, int to);
>
>   extern int varobj_pretty_printed_p (struct varobj *var);
>
> +extern  struct cleanup *varobj_ensure_python_env (struct varobj *var);
> +

IIRC, we are now adding documentation near declarations, too. Many are 
often as simple as "See description in varobj.c."

>   extern int varobj_default_value_is_changeable_p (struct varobj *var);
>   extern int varobj_value_is_changeable_p (struct varobj *var);