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

[Paul Koning <paulkoning at comcast dot net>]

I'm confused by what this patch does.  

It seems that it handles an empty field name by producing None instead of a gdb.Field.  Why do that?  The existing code detects the case where TYPE_FIELD_NAME is null -- if it is, then Field.name is set to None, otherwise it is set to a Python string whose content is the field name.  If it's possible for TYPE_FIELD_NAME to be non-null but a zero length string, would it not be more consistent for that case also to produce a gdb.Field object with name == None?

Looking further down, it looks like you're recursing into the anonymous member instead.  Ok...  Is it possible for anonymous members in turn to have anonymous members?  That case isn't covered.


> gdb.Type.fields() missed handling for anonymous members.
> This patch fix it,
> 
> Signed-off-by: Li Yu <raise.sail@gmail.com>
> 
> gdb/python/:
> 2011-09-29  Li Yu  <raise.sail@gmail.com>
> 
> 	* py-type.c: Add process for anonymous members of struct and union
> 
> ---
> gdb/python/py-type.c |   37 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 35 insertions(+), 2 deletions(-)
> --- gdb-7.3.1.orig/gdb/python/py-type.c	2011-01-27 04:53:45.000000000 +0800
> +++ gdb-7.3.1/gdb/python/py-type.c	2011-09-29 15:15:31.000000000 +0800

Should the patch be against the current instead of against a branch?

> @@ -143,6 +143,7 @@ convert_field (struct type *type, int fi  {
>   PyObject *result = field_new ();
>   PyObject *arg;
> +  char *name = TYPE_FIELD_NAME (type, field);
> 
>   if (!result)
>     return NULL;
> @@ -157,8 +158,13 @@ convert_field (struct type *type, int fi
> 	goto failarg;
>     }
> 
> -  if (TYPE_FIELD_NAME (type, field))
> -    arg = PyString_FromString (TYPE_FIELD_NAME (type, field));
> +  if (name)
> +    {
> +      if (name[0])
> +        arg = PyString_FromString (name);
> +      else
> +        return Py_None;

That doesn't increment the reference count.  You could write the statement Py_RETURN_NONE to do the increment and return in one line.  Or are you returning a borrowed reference to Py_None (while in all other cases this function returns a new reference)?  That seems ugly, at the least that should be clearly documented.  I think it's better for the reference count handling to be uniform: for every return value, the reference is either new or borrowed (and in most cases, new reference appears to be the preferred practice).

> +    }
>   else
>     {
>       arg = Py_None;
> @@ -240,6 +246,33 @@ typy_fields (PyObject *self, PyObject *a
> 	  Py_DECREF (result);
> 	  return NULL;
> 	}
> +
> +      if (dict == Py_None)
> +        {
> +          struct type *f_type = TYPE_FIELD_TYPE(type, i);
> +          int j;
> +
> +          if ((TYPE_CODE(type) != TYPE_CODE_UNION) 
> +            && (TYPE_CODE(type) != TYPE_CODE_STRUCT))
> +            {
> +	      Py_DECREF (result);
> +	      return NULL;

You didn't decrement the reference count on dict.  Also, this raises an exception, but what exception?  I would expect a PyErr_SetString call.

> +            }
> +            
> +          for (j = 0; j < TYPE_NFIELDS(f_type); ++j)
> +            {
> +              PyObject *dict = convert_field (f_type, j);
> +
> +              if (!dict || PyList_Append (result, dict))
> +                {
> +                  Py_XDECREF (dict);

The declaration for "dict" a few lines up hides the outer one, which doesn't have its reference count decremented.

> +                  Py_DECREF (result);
> +                  return NULL;
> +                }
> +            }
> +          continue;
> +        }
> +
>       if (PyList_Append (result, dict))
> 	{
> 	  Py_DECREF (dict);