[PATCH 03/11] Iterate over 'struct varobj_item' instead of PyObject
Keith Seitz
keiths@redhat.com
Tue Jan 21 20:44:00 GMT 2014
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. */
>
More information about the Gdb-patches
mailing list