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

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

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

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

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

> +  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);

>   return NULL;
> }

You need to free iter_obj->child in the iterator destructor.

	paul