From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7480 invoked by alias); 1 Oct 2011 14:01:39 -0000 Received: (qmail 7472 invoked by uid 22791); 1 Oct 2011 14:01:38 -0000 X-SWARE-Spam-Status: No, hits=-0.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 01 Oct 2011 14:01:23 +0000 Received: by wyf22 with SMTP id 22so2178289wyf.0 for ; Sat, 01 Oct 2011 07:01:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.82.211 with SMTP id o61mr3003703wee.81.1317477681569; Sat, 01 Oct 2011 07:01:21 -0700 (PDT) Received: by 10.216.174.199 with HTTP; Sat, 1 Oct 2011 07:01:21 -0700 (PDT) In-Reply-To: <07654BC2-7955-4858-8AE9-CA9FF895A158@comcast.net> References: <4E8595F6.7080004@gmail.com> <07654BC2-7955-4858-8AE9-CA9FF895A158@comcast.net> Date: Sat, 01 Oct 2011 14:01:00 -0000 Message-ID: Subject: Re: [PATCH v2] gdb/python: add missing handling for anonymous members of struct and union From: Li Yu To: Paul Koning Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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-10/txt/msg00011.txt.bz2 2011/9/30 Paul Koning : > > On Sep 30, 2011, at 6:12 AM, Li Yu wrote: > >> ... >> >> gdb/python/: >> 2011-09-29 =A0Li Yu =A0 >> >> =A0 =A0 =A0 * py-type.c: Add process for anonymous members of struct and= union >> >> py-type.c | =A0 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++= --------- >> 1 file changed, 54 insertions(+), 9 deletions(-) >> >> 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. =A0Perhaps= your mailer is creating trouble. =A0For one thing, it comes across in Unic= ode with non-break spaces in it, and spaces at the start of lines are =A0mi= ssing. =A0I can't apply the patch with the "patch" tool... Sorry for confused patch, it may be triggered by my mis-configured thunderb= ird. > >> 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; >> >> /* A type iterator object. =A0*/ >> -typedef struct { >> +struct __typy_iterator_object >> +{ >> =A0 PyObject_HEAD >> + =A0/* The iterators for support fields of anonymous field */ >> + =A0struct __typy_iterator_object *child; >> + =A0struct __typy_iterator_object *parent; >> =A0 /* The current field index. =A0*/ >> =A0 int field; >> =A0 /* What to return. =A0*/ >> =A0 enum gdbpy_iter_kind kind; >> =A0 /* Pointer back to the original source type object. =A0*/ >> =A0 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. > OK >> >> static PyTypeObject type_iterator_object_type; >> >> @@ -1201,6 +1206,8 @@ typy_make_iter (PyObject *self, enum gdbpy_iter_ki= nd kind) >> =A0 if (typy_iter_obj =3D=3D NULL) >> =A0 =A0 =A0 return NULL; >> >> + =A0typy_iter_obj->child =3D NULL; >> + =A0typy_iter_obj->parent =3D NULL; >> =A0 typy_iter_obj->field =3D 0; >> =A0 typy_iter_obj->kind =3D kind; >> =A0 Py_INCREF (self); >> - =A0struct type *type =3D iter_obj->source->type; >> - =A0int i; >> - =A0PyObject *result; >> - >> - =A0if (iter_obj->field < TYPE_NFIELDS (type)) > > Something strange here. =A0The diff shows this code directly after the ot= her code in typy_make_iter, but that's not where it is. =A0The code is actu= ally in typy_iterator_iternext, which is several functions later in the sou= rce. =A0Is "git diff" malfunctioning or is this a cut & paste glitch? It just is the result of "git diff" without any manual changes, my working system is ubuntu natty, a bug in prebuilt git? ;) Anyway, I will generate right patch at next time. > >> + =A0typy_iterator_object *iter_obj =3D (typy_iterator_object *) self, *= child_iter_obj; >> + =A0struct type *type; >> + =A0PyObject *result, *child_pytype; >> + =A0char *name; >> + >> + =A0while (iter_obj->child) /* deepest anonymous member first */ >> + =A0{ >> + =A0 =A0iter_obj =3D iter_obj->child; >> + =A0} > >> + =A0type =3D iter_obj->source->type; >> + >> +restart: >> + =A0while (iter_obj->field >=3D TYPE_NFIELDS (type)) >> + =A0{ >> + =A0 =A0iter_obj =3D iter_obj->parent; >> + =A0 =A0if (!iter_obj) >> + =A0 =A0 =A0return NULL; >> + =A0 =A0Py_DECREF(iter_obj->child); >> + =A0 =A0iter_obj->child =3D NULL; >> + =A0 =A0type =3D iter_obj->source->type; >> + =A0} >> + > > That whole chunk of code would be simpler and probably easier to understa= nd if you wrote it as a recursion rather than a loop. =A0Something like: > > restart: > =A0if (iter_obj->child) > =A0 =A0{ > =A0 =A0 =A0result =3D typy_iterator_iternext (iter_obj->child); > =A0 =A0 =A0if (result !=3D NULL) > =A0 =A0 =A0 =A0 return result; > =A0 =A0 =A0Py_CLEAR (iter_obj->child); > =A0 =A0} > > One benefit is that the "parent" member is no longer needed if you do tha= t. =A0Then the existing "if (iter_obj->field < TYPE_NFIELDS (type))" would = stay, and the code you have under "abort_clean" is also the "stop iteration= " code. > >> + =A0name =3D TYPE_FIELD_NAME (type, iter_obj->field); >> + =A0if (!name) >> + =A0 =A0goto abort_clean; > > Why is this needed? =A0When is name null as opposed to pointing to an emp= ty string? > >> + >> + =A0if (name[0]) /* mostly cases */ >> =A0 =A0 { >> =A0 =A0 =A0 result =3D make_fielditem (type, iter_obj->field, iter_obj->= kind); >> =A0 =A0 =A0 if (result !=3D NULL) >> - =A0 =A0 iter_obj->field++; >> + =A0 =A0 =A0 =A0iter_obj->field++; >> =A0 =A0 =A0 return result; >> =A0 =A0 } >> >> + =A0/* handing for anonymous members here */ >> + =A0type =3D TYPE_FIELD_TYPE(type, iter_obj->field++); >> + =A0child_pytype =3D type_to_type_object(type); >> + =A0if (!child_pytype) >> + =A0 =A0 goto abort_clean;; > > Extra semicolon. > >> + =A0child_iter_obj =3D (typy_iterator_object*)typy_make_iter (child_pyt= ype, iter_obj->kind); > > You need an error check (child_iter_obj =3D=3D NULL) here. > En, you are right again~ >> + =A0iter_obj->child =3D child_iter_obj; >> + =A0child_iter_obj->parent =3D iter_obj; >> + =A0iter_obj =3D child_iter_obj; >> + =A0goto restart; >> + >> +abort_clean: >> + =A0while (iter_obj->parent) >> + =A0{ >> + =A0 =A0iter_obj =3D iter_obj->parent; >> + =A0 =A0Py_DECREF(iter_obj->child); >> + =A0} > > If you use recursion, all this simply becomes Py_CLEAN (iter_obj->child); > I indeed do not like use recursion in any production code, because of it im= plies some problems, e.g. it may bring more function calls at runtime, which means much security risk and may use more memory, en, this just is my personal taste. Of course, you must have more experience on gdb internals, if you decide to= use recursion here, I will agree with you too and would like write a recursion implementation later. I will leave about one week, see you later ~ >> =A0 return NULL; >> } > > You need to free iter_obj->child in the iterator destructor. > I think that Py_DEFREF(iter_obj->child) should be able to help us reclaim our allocated iterators here. > =A0 =A0 =A0 =A0paul > >