From: Paul Koning <paulkoning@comcast.net>
To: raise.sail@gmail.com, gdb@sourceware.org
Subject: Re: [PATCH] gdb/python: add missing handling for anonymous members of struct and union
Date: Thu, 29 Sep 2011 12:51:00 -0000 [thread overview]
Message-ID: <AFD030CE-0F23-48D7-8A66-F17DDBFA89B0@comcast.net> (raw)
In-Reply-To: <09787EF419216C41A903FD14EE5506DD030A334B61@AUSX7MCPC103.AMER.DELL.COM>
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);
next parent reply other threads:[~2011-09-29 12:51 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <4E841E64.8090609@gmail.com>
[not found] ` <09787EF419216C41A903FD14EE5506DD030A334B61@AUSX7MCPC103.AMER.DELL.COM>
2011-09-29 12:51 ` Paul Koning [this message]
[not found] ` <CA+WLrf8CitrMfanhy0S6DcM47b1odfm0ZkX8nJ3ezafpfAoS5g@mail.gmail.com>
2011-09-29 14:58 ` Li Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AFD030CE-0F23-48D7-8A66-F17DDBFA89B0@comcast.net \
--to=paulkoning@comcast.net \
--cc=gdb@sourceware.org \
--cc=raise.sail@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).