From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26447 invoked by alias); 30 Sep 2011 15:12:43 -0000 Received: (qmail 26439 invoked by uid 22791); 30 Sep 2011 15:12:40 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from qmta14.emeryville.ca.mail.comcast.net (HELO qmta14.emeryville.ca.mail.comcast.net) (76.96.27.212) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 30 Sep 2011 15:12:24 +0000 Received: from omta21.emeryville.ca.mail.comcast.net ([76.96.30.88]) by qmta14.emeryville.ca.mail.comcast.net with comcast id f1hn1h0041u4NiLAE3CHRl; Fri, 30 Sep 2011 15:12:17 +0000 Received: from [10.127.238.91] ([65.206.2.68]) by omta21.emeryville.ca.mail.comcast.net with comcast id f3E51h0091U2a2h8h3E72f; Fri, 30 Sep 2011 15:14:14 +0000 Subject: Re: [PATCH v2] gdb/python: add missing handling for anonymous members of struct and union Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Paul Koning In-Reply-To: <4E8595F6.7080004@gmail.com> Date: Fri, 30 Sep 2011 16:15:00 -0000 Cc: gdb-patches@sourceware.org Content-Transfer-Encoding: quoted-printable Message-Id: <07654BC2-7955-4858-8AE9-CA9FF895A158@comcast.net> References: <4E8595F6.7080004@gmail.com> To: Li Yu 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 X-SW-Source: 2011-09/txt/msg00574.txt.bz2 On Sep 30, 2011, at 6:12 AM, Li Yu wrote: > ... >=20 > gdb/python/: > 2011-09-29 Li Yu >=20 > * py-type.c: Add process for anonymous members of struct and union >=20 > py-type.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++---= ------ > 1 file changed, 54 insertions(+), 9 deletions(-) >=20 > diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c For some strange reason your patch comes across all messed up. Perhaps you= r mailer is creating trouble. For one thing, it comes across in Unicode wi= th non-break spaces in it, and spaces at the start of lines are missing. = I can't apply the patch with the "patch" tool... > index 27f8b38..d0d8a94 100644 > --- a/gdb/python/py-type.c > +++ b/gdb/python/py-type.c > @@ -56,15 +56,20 @@ typedef struct pyty_field_object > static PyTypeObject field_object_type; >=20 > /* A type iterator object. */ > -typedef struct { > +struct __typy_iterator_object > +{ > PyObject_HEAD > + /* The iterators for support fields of anonymous field */ > + struct __typy_iterator_object *child; > + struct __typy_iterator_object *parent; > /* The current field index. */ > int field; > /* What to return. */ > enum gdbpy_iter_kind kind; > /* Pointer back to the original source type object. */ > struct pyty_type_object *source; > -} typy_iterator_object; > +}; > +typedef struct __typy_iterator_object typy_iterator_object; You could just write that as typedef struct __typy_iterator_object { ... } = typy_iterator_object; without the typedef as a separate line. >=20 > static PyTypeObject type_iterator_object_type; >=20 > @@ -1201,6 +1206,8 @@ typy_make_iter (PyObject *self, enum gdbpy_iter_kin= d kind) > if (typy_iter_obj =3D=3D NULL) > return NULL; >=20 > + typy_iter_obj->child =3D NULL; > + typy_iter_obj->parent =3D NULL; > typy_iter_obj->field =3D 0; > typy_iter_obj->kind =3D kind; > Py_INCREF (self); > - struct type *type =3D iter_obj->source->type; > - int i; > - PyObject *result; > -=20=20 > - if (iter_obj->field < TYPE_NFIELDS (type)) Something strange here. The diff shows this code directly after the other = code in typy_make_iter, but that's not where it is. The code is actually i= n typy_iterator_iternext, which is several functions later in the source. = Is "git diff" malfunctioning or is this a cut & paste glitch? > + typy_iterator_object *iter_obj =3D (typy_iterator_object *) self, *chi= ld_iter_obj; > + struct type *type; > + PyObject *result, *child_pytype; > + char *name; > + > + while (iter_obj->child) /* deepest anonymous member first */ > + { > + iter_obj =3D iter_obj->child; > + } > + type =3D iter_obj->source->type; > + > +restart: > + while (iter_obj->field >=3D TYPE_NFIELDS (type)) > + { > + iter_obj =3D iter_obj->parent; > + if (!iter_obj) > + return NULL; > + Py_DECREF(iter_obj->child); > + iter_obj->child =3D NULL; > + type =3D iter_obj->source->type; > + } > + That whole chunk of code would be simpler and probably easier to understand= if you wrote it as a recursion rather than a loop. Something like: restart: if (iter_obj->child) { result =3D typy_iterator_iternext (iter_obj->child); if (result !=3D NULL) return result; Py_CLEAR (iter_obj->child); } One benefit is that the "parent" member is no longer needed if you do that.= Then the existing "if (iter_obj->field < TYPE_NFIELDS (type))" would stay= , and the code you have under "abort_clean" is also the "stop iteration" co= de. > + name =3D TYPE_FIELD_NAME (type, iter_obj->field); > + if (!name) > + goto abort_clean; Why is this needed? When is name null as opposed to pointing to an empty s= tring? > + > + if (name[0]) /* mostly cases */ > { > result =3D make_fielditem (type, iter_obj->field, iter_obj->kind); > if (result !=3D NULL) > - iter_obj->field++; > + iter_obj->field++; > return result; > } >=20 > + /* handing for anonymous members here */ > + type =3D TYPE_FIELD_TYPE(type, iter_obj->field++); > + child_pytype =3D type_to_type_object(type); > + if (!child_pytype) > + goto abort_clean;; Extra semicolon. > + child_iter_obj =3D (typy_iterator_object*)typy_make_iter (child_pytype= , iter_obj->kind); You need an error check (child_iter_obj =3D=3D NULL) here. > + iter_obj->child =3D child_iter_obj; > + child_iter_obj->parent =3D iter_obj; > + iter_obj =3D child_iter_obj; > + goto restart; > + > +abort_clean: > + while (iter_obj->parent) > + { > + iter_obj =3D iter_obj->parent; > + Py_DECREF(iter_obj->child); > + } If you use recursion, all this simply becomes Py_CLEAN (iter_obj->child); > return NULL; > } You need to free iter_obj->child in the iterator destructor. paul