Re: [PATCH v2] gdb/python: add missing handling for anonymous members of struct and union

[Li Yu <raise.sail@gmail.com>]

2011/9/30 Paul Koning <paulkoning@comcast.net>:
>
> On Sep 30, 2011, at 6:12 AM, Li Yu wrote:
>
>> ...
>>
>> gdb/python/:
>> 2011-09-29  Li Yu  <raise.sail@gmail.com>
>>
>>       * py-type.c: Add process for anonymous members of struct and union
>>
>> py-type.c |   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.  Perhaps your mailer is creating trouble.  For one thing, it comes across in Unicode with non-break spaces in it, and spaces at the start of lines are  missing.  I can't apply the patch with the "patch" tool...

Sorry for confused patch, it may be triggered by my mis-configured thunderbird.

>
>> 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.  */
>> -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.
>

OK

>>
>> static PyTypeObject type_iterator_object_type;
>>
>> @@ -1201,6 +1206,8 @@ typy_make_iter (PyObject *self, enum gdbpy_iter_kind kind)
>>   if (typy_iter_obj == NULL)
>>       return NULL;
>>
>> +  typy_iter_obj->child = NULL;
>> +  typy_iter_obj->parent = NULL;
>>   typy_iter_obj->field = 0;
>>   typy_iter_obj->kind = kind;
>>   Py_INCREF (self);
>> -  struct type *type = iter_obj->source->type;
>> -  int i;
>> -  PyObject *result;
>> -
>> -  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 in typy_iterator_iternext, which is several functions later in the source.  Is "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.

>
>> +  typy_iterator_object *iter_obj = (typy_iterator_object *) self, *child_iter_obj;
>> +  struct type *type;
>> +  PyObject *result, *child_pytype;
>> +  char *name;
>> +
>> +  while (iter_obj->child) /* deepest anonymous member first */
>> +  {
>> +    iter_obj = iter_obj->child;
>> +  }
>
>> +  type = iter_obj->source->type;
>> +
>> +restart:
>> +  while (iter_obj->field >= TYPE_NFIELDS (type))
>> +  {
>> +    iter_obj = iter_obj->parent;
>> +    if (!iter_obj)
>> +      return NULL;
>> +    Py_DECREF(iter_obj->child);
>> +    iter_obj->child = NULL;
>> +    type = 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 = typy_iterator_iternext (iter_obj->child);
>      if (result != 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" code.
>
>> +  name = 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 string?
>
>> +
>> +  if (name[0]) /* mostly cases */
>>     {
>>       result = make_fielditem (type, iter_obj->field, iter_obj->kind);
>>       if (result != NULL)
>> -     iter_obj->field++;
>> +        iter_obj->field++;
>>       return result;
>>     }
>>
>> +  /* handing for anonymous members here */
>> +  type = TYPE_FIELD_TYPE(type, iter_obj->field++);
>> +  child_pytype = type_to_type_object(type);
>> +  if (!child_pytype)
>> +     goto abort_clean;;
>
> Extra semicolon.
>
>> +  child_iter_obj = (typy_iterator_object*)typy_make_iter (child_pytype, iter_obj->kind);
>
> You need an error check (child_iter_obj == NULL) here.
>

En, you are right again~

>> +  iter_obj->child = child_iter_obj;
>> +  child_iter_obj->parent = iter_obj;
>> +  iter_obj = child_iter_obj;
>> +  goto restart;
>> +
>> +abort_clean:
>> +  while (iter_obj->parent)
>> +  {
>> +    iter_obj = iter_obj->parent;
>> +    Py_DECREF(iter_obj->child);
>> +  }
>
> 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 implies
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 ~

>>   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.

>        paul
>
>