This is the mail archive of the gdb@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


2011/9/29 Paul Koning <paulkoning@comcast.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?
>

Sorry for too short description, please allow me use one example to
explain why I write this patch, we assume there is a C source like
below:

struct A
{
  int a;
  union {
    int b0;
    int b1;
  };
  int c;
  union {
    int d0;
    int d1;
  };
};

int main()
{
   struct A a;
}

After great gdb break at main() function, we execute below python script:

v = gdb.parse_and_eval("a")
t = v.type
fields = t.fields()
for f in fields:
    print "[%s]" % f.name, v[f.name]

Without this patch, we will see its output may be:

[a] 0
[] {d0 = 0, d1 = 0}
[c] 0
[] {d0 = 0, d1 = 0}

With this patch, we will see its output may be:

[a] 0
[b0] 0
[b1] 0
[c] 0
[d0] 0
[d1] 0

Now, the question is clearly, gdb.Type.fields() miss right handling
for anonymous members of union/struct.

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

"anonymous members in anonymous members", Good point!

I indeed think care about it, I think that embedded anonymous
union/struct are values of another "gdb.Type",
so if the python script writer need to know details of them, they just
should invoke fields() method of another
gdb.Type objects.


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

OK, next version ...

;)

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

My goal indeed is borrowed reference here, however I prefer to
increase the reference count here in next version of this patch.

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

See, the dict is borrowed, so save a refcnt operation here. I will
change this in next version.

Lost setting exception here, it should be a gdb internal error here, I
do not make sure which exception is suitable here, your suggestion is
welcome!

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

En, I think that it is not a problem here, because of dict is a
borrowed Py_None object or NULL pointer, so we do not need to decrease
refcnt here,
and Py_XDECREF() can skip NULL pointer.

Thanks for your time!

Yu

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]