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

Paul Koning
2011-09-30T15:12:43Z

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