This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: patch for #14363
- From: Phil Muldoon <pmuldoon at redhat dot com>
- To: Oliver Buchtala <oliver dot buchtala at googlemail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 06 Aug 2012 11:42:41 +0100
- Subject: Re: patch for #14363
- References: <501AA900.8020205@googlemail.com>
On 08/02/2012 05:21 PM, Oliver Buchtala wrote:
> From 05a921d423793db93dfead59c9fe12d342f55d75 Mon Sep 17 00:00:00 2001
> From: Oliver Buchtala <oliver.buchtala@googlemail.com>
> Date: Wed, 1 Aug 2012 21:59:43 +0200
> Subject: [PATCH] Adapt py-prettyprint to inform pretty-printer about current
> recursion level.
>
> ---
> gdb/python/py-prettyprint.c | 3 +++
> gdb/python/python-internal.h | 1 +
> gdb/python/python.c | 2 ++
> 3 files changed, 6 insertions(+)
Thanks.
Needs a GNU style ChangeLog, testcase and documentation
additions.
I am having trouble understanding what you are doing with "recurse"
attribute. From valprint.c, in the GDB sources (which calls
apply_val_pretty_printer, among others):
"RECURSE indicates the amount of indentation to supply before
continuation lines; this amount is roughly twice the value of
RECURSE."
So I am not sure that this is the value you want? Anyway, I think you
want the number of recursions for that printer, not the recurse value
being supplied to apply_val_pretty_printer. A testcase would help
here to understand your intention.
> diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
> index 86d4f2c..d5c6b66 100644
> --- a/gdb/python/py-prettyprint.c
> +++ b/gdb/python/py-prettyprint.c
> @@ -476,6 +476,9 @@ print_children (PyObject *printer, const char *hint,
> if (! PyObject_HasAttr (printer, gdbpy_children_cst))
> return;
>
> + if ( PyObject_HasAttr (printer, gdbpy_level_cst))
> + PyObject_SetAttr(printer, gdbpy_level_cst, PyInt_FromLong((long) recurse));
> +
There are a few issues with this patch hunk:
* It is not clear in the documentation, but PyInt_FromLong can raise a
PyErr_NoMemory exception, so you need to NULL check the return and
dispatch the exception appropriately.
* This leaks a reference from PyInt_FromLong. PyObject_SetAttr
increments the reference count of the right side of "=" Python
equivalent (IE foo.bar = baz), which in this case is the integer. So
the transient reference from PyInt_FromLong will last forever. You
you need to assign the results of PyInt_FromLong to a variable and
call Py_DECREF on it after the "PyObject_SetAttr" call.
* PyObject_SetAttr can fail, so the code needs to deal with this
contingency.
* Why PyInt_FromLong over PyLong_FromLong?
Beyond the patch fixes, is it not possible to count the recursion
level by some internal bookkeeping in the printer? I have no problem
with including the attribute if it is helpful, or clearer to solve the
problem, but I am curious if you have tried the alternative, if
possible?
Cheers,
Phil