From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3513 invoked by alias); 21 Jan 2014 20:44:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3501 invoked by uid 89); 21 Jan 2014 20:44:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 21 Jan 2014 20:44:01 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0LKhxrO008600 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 21 Jan 2014 15:43:59 -0500 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s0LKhwKL015917 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 21 Jan 2014 15:43:58 -0500 Message-ID: <52DEDC0E.50201@redhat.com> Date: Tue, 21 Jan 2014 20:44:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH 02/11] Generalize varobj iterator References: <1385258996-26047-1-git-send-email-yao@codesourcery.com> <1385258996-26047-3-git-send-email-yao@codesourcery.com> In-Reply-To: <1385258996-26047-3-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-01/txt/msg00815.txt.bz2 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 > Yao Qi > > * 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) : Change its type from "PyObject *" > to "struct varobj_iter *". > : 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 . */ > + > +#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 ("", > + 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 . */ > + > +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 ("", 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); > >