Re: [PATCH 03/11] Iterate over 'struct varobj_item' instead of PyObject

[Keith Seitz <keiths@redhat.com>]

On 11/23/2013 06:09 PM, Yao Qi wrote:
> gdb:
>
> 2013-11-24  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
>
> 	* python/py-varobj.c (py_varobj_iter_next): Move some code
> 	from varobj.c.
> 	(py_varobj_get_iterator):
          ^^^^^^^^^^^^^^^^^^^^^^
Superfluous? I don't see any changes to that function.

> 	* varobj-iter.h (struct varobj_item): New.  Moved from
> 	varobj.c.

I would remove "New." That threw me a little while reading the patch. It 
looked familiar! Simply saying that you moved it is sufficient.

> 	* varobj.c: Move "varobj-iter.h" inclusion earlier.
> 	(struct varobj_item): Remove.

I think it is more concise to say "Moved to varobj-iter.h" instead of 
"Remove".

> 	(varobj_clear_saved_item): New function.
> 	(update_dynamic_varobj_children): Move python-related code to
> 	py-varobj.c.
> 	(free_variable): Call varobj_clear_saved_item.
> ---
>   gdb/python/py-varobj.c |   17 ++++++++++-
>   gdb/varobj-iter.h      |   12 ++++++-
>   gdb/varobj.c           |   73 ++++++++++++++++-------------------------------
>   3 files changed, 51 insertions(+), 51 deletions(-)
>
> diff --git a/gdb/python/py-varobj.c b/gdb/python/py-varobj.c
> index 6588fc2..506e865 100644
> --- a/gdb/python/py-varobj.c
> +++ b/gdb/python/py-varobj.c
> @@ -51,6 +51,9 @@ py_varobj_iter_next (struct varobj_iter *self)
>     struct py_varobj_iter *t = (struct py_varobj_iter *) self;
>     struct cleanup *back_to;
>     PyObject *item;
> +  PyObject *py_v;
> +  varobj_item *vitem;
> +  const char *name = NULL;
>
>     back_to = varobj_ensure_python_env (self->var);
>
> @@ -98,9 +101,21 @@ py_varobj_iter_next (struct varobj_iter *self)
>   	}
>       }
>
> +  if (!PyArg_ParseTuple (item, "sO", &name, &py_v))
> +    {
> +      gdbpy_print_stack ();
> +      error (_("Invalid item from the child list"));
> +    }
> +
> +  vitem = xmalloc (sizeof *vitem);

You can use "XNEW (struct varobj_item)" here, too, although there isn't 
any real overriding reason to do so (other than personal preference). 
Feel free to ignore me, though. There is no hard rule for this in 
gdb-land as far as I know.

> +  vitem->value = convert_value_from_python (py_v);
> +  if (vitem->value == NULL)
> +    gdbpy_print_stack ();
> +  vitem->name = xstrdup (name);
> +
>     self->next_raw_index++;
>     do_cleanups (back_to);
> -  return item;
> +  return vitem;
>   }
>
>   /* The 'vtable' of pretty-printed python varobj iterators.  */
> diff --git a/gdb/varobj-iter.h b/gdb/varobj-iter.h
> index 59be278..b4bda82 100644
> --- a/gdb/varobj-iter.h
> +++ b/gdb/varobj-iter.h
> @@ -14,9 +14,17 @@
>      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;
> +/* A node or item of varobj, composed by the name and the value.  */
> +
> +typedef struct varobj_item
> +{
> +  /* Name of this item.  */
> +  char *name;
> +  /* Value of this item.  */
> +  struct value *value;
> +} varobj_item;
>

[Changes mentioned in last patch for above hunk]

> -typedef PyObject varobj_item;
> +struct varobj_iter_ops;
>
>   /* A dynamic varobj iterator "class".  */
>
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 7a8305b..28e934c 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -33,6 +33,7 @@
>   #include "vec.h"
>   #include "gdbthread.h"
>   #include "inferior.h"
> +#include "varobj-iter.h"
>
>   #if HAVE_PYTHON
>   #include "python/python.h"
> @@ -41,8 +42,6 @@
>   typedef int PyObject;
>   #endif
>
> -#include "varobj-iter.h"
> -
>   /* Non-zero if we want to see trace of varobj level stuff.  */
>
>   unsigned int varobjdebug = 0;
> @@ -110,16 +109,6 @@ struct varobj_root
>     struct varobj_root *next;
>   };
>
> -/* A node or item of varobj, composed by the name and the value.  */
> -
> -struct varobj_item
> -{
> -  /* Name of this item.  */
> -  char *name;
> -  /* Value of this item.  */
> -  struct value *value;
> -};
> -
>   /* Dynamic part of varobj.  */
>
>   struct varobj_dynamic
> @@ -787,6 +776,18 @@ varobj_get_iterator (struct varobj *var)
>   requested an interator from a non-dynamic varobj"));
>   }
>
> +/* Release and clear VAR's saved item, if any.  */
> +
> +static void
> +varobj_clear_saved_item (struct varobj_dynamic *var)
> +{
> +  if (var->saved_item != NULL)
> +    {
> +      value_free (var->saved_item->value);
> +      xfree (var->saved_item);
> +      var->saved_item = NULL;
> +    }
> +}
>   #endif
>
>   static int
> @@ -801,14 +802,8 @@ update_dynamic_varobj_children (struct varobj *var,
>   				int to)
>   {
>   #if HAVE_PYTHON
> -  struct cleanup *back_to;
>     int i;
>
> -  if (!gdb_python_initialized)
> -    return 0;
> -
> -  back_to = varobj_ensure_python_env (var);
> -
>     *cchanged = 0;
>
>     if (update_children || var->dynamic->child_iter == NULL)
> @@ -816,16 +811,12 @@ update_dynamic_varobj_children (struct varobj *var,
>         varobj_iter_delete (var->dynamic->child_iter);
>         var->dynamic->child_iter = varobj_get_iterator (var);
>
> -      Py_XDECREF (var->dynamic->saved_item);
> -      var->dynamic->saved_item = NULL;
> +      varobj_clear_saved_item (var->dynamic);
>
>         i = 0;
>
>         if (var->dynamic->child_iter == NULL)
> -	{
> -	  do_cleanups (back_to);
> -	  return 0;
> -	}
> +	return 0;
>       }
>     else
>       i = VEC_length (varobj_p, var->children);
> @@ -834,10 +825,10 @@ update_dynamic_varobj_children (struct varobj *var,
>        are more children.  */
>     for (; to < 0 || i < to + 1; ++i)
>       {
> -      PyObject *item;
> +      varobj_item *item;
>
>         /* See if there was a leftover from last time.  */
> -      if (var->dynamic->saved_item)
> +      if (var->dynamic->saved_item != NULL)
>   	{
>   	  item = var->dynamic->saved_item;
>   	  var->dynamic->saved_item = NULL;
> @@ -845,6 +836,10 @@ update_dynamic_varobj_children (struct varobj *var,
>         else
>   	{
>   	  item = varobj_iter_next (var->dynamic->child_iter);
> +	  /* Release vitem->value so its lifetime is not bound to the
> +	     execution of a command.  */
> +	  if (item != NULL && item->value != NULL)
> +	    release_value_or_incref (item->value);
>   	}
>
>         if (item == NULL)
> @@ -857,36 +852,19 @@ update_dynamic_varobj_children (struct varobj *var,
>         /* We don't want to push the extra child on any report list.  */
>         if (to < 0 || i < to)
>   	{
> -	  PyObject *py_v;
> -	  const char *name;
> -	  struct varobj_item varobj_item;
> -	  struct cleanup *inner;
>   	  int can_mention = from < 0 || i >= from;
>
> -	  inner = make_cleanup_py_decref (item);
> -
> -	  if (!PyArg_ParseTuple (item, "sO", &name, &py_v))
> -	    {
> -	      gdbpy_print_stack ();
> -	      error (_("Invalid item from the child list"));
> -	    }
> -
> -	  varobj_item.value = convert_value_from_python (py_v);
> -	  if (varobj_item.value == NULL)
> -	    gdbpy_print_stack ();
> -	  varobj_item.name = xstrdup (name);
> -
>   	  install_dynamic_child (var, can_mention ? changed : NULL,
>   				 can_mention ? type_changed : NULL,
>   				 can_mention ? new : NULL,
>   				 can_mention ? unchanged : NULL,
>   				 can_mention ? cchanged : NULL, i,
> -				 &varobj_item);
> -	  do_cleanups (inner);
> +				 item);
> +
> +	  xfree (item);
>   	}
>         else
>   	{
> -	  Py_XDECREF (var->dynamic->saved_item);
>   	  var->dynamic->saved_item = item;
>
>   	  /* We want to truncate the child list just before this
> @@ -2180,12 +2158,11 @@ free_variable (struct varobj *var)
>
>         Py_XDECREF (var->dynamic->constructor);
>         Py_XDECREF (var->dynamic->pretty_printer);
> -      Py_XDECREF (var->dynamic->child_iter);

If I'm reading this right, this statement was moved to the python 
iterator dtor function. What happens if we do not iterate over all 
children and then delete the varobj? I don't see where 
varobj_delete_iter is called in that case, i.e., isn't this leaked in 
this scenario?

> -      Py_XDECREF (var->dynamic->saved_item);
>         do_cleanups (cleanup);
>       }
>   #endif
>
> +  varobj_clear_saved_item (var->dynamic);
>     value_free (var->value);
>
>     /* Free the expression if this is a root variable.  */
>