patch for #14363

patch for #14363

[Oliver Buchtala]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

Hello,

this patch can be used to achieve a workaround to bug
http://sourceware.org/bugzilla/show_bug.cgi?id=14363
Besides being a workaround, it also is a new feature, enabling a 
pretty-printer to
control the pretty-printing on deeper recursion levels.

The bug:
Structures/classes which produce cyclic reference lead to an infinite 
recursion on
iterating children during pretty-pretting - leading to endless print and 
stack-overflow on gdb print.

What the patch does:
Before calling the pretty-printer, py-prettyprint.c:print_children() 
informs the pretty-printer about the current recursion level.
This is done by setting a property "level" if this property exists.

Changelog:

2012-08-02  Oliver Buchtala <oliver.buchtala@googlemail.com>

     python/py-prettyprint.c: provide current recursion level to pretty 
printer.


[-- Attachment #2: 0001-Adapt-py-prettyprint-to-inform-pretty-printer-about-.patch --]
[-- Type: text/x-patch, Size: 2193 bytes --]

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(+)

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));
+
   /* If we are printing a map or an array, we want some special
      formatting.  */
   is_map = hint && ! strcmp (hint, "map");
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index bae61c2..e600632 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -328,6 +328,7 @@ extern PyObject *gdbpy_to_string_cst;
 extern PyObject *gdbpy_display_hint_cst;
 extern PyObject *gdbpy_enabled_cst;
 extern PyObject *gdbpy_value_cst;
+extern PyObject *gdbpy_level_cst;
 
 /* Exception types.  */
 extern PyObject *gdbpy_gdb_error;
diff --git a/gdb/python/python.c b/gdb/python/python.c
index c66efe4..ae9e530 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -82,6 +82,7 @@ PyObject *gdbpy_display_hint_cst;
 PyObject *gdbpy_doc_cst;
 PyObject *gdbpy_enabled_cst;
 PyObject *gdbpy_value_cst;
+PyObject *gdbpy_level_cst;
 
 /* The GdbError exception.  */
 PyObject *gdbpy_gdberror_exc;
@@ -1305,6 +1306,7 @@ message == an error message without a stack will be printed."),
   gdbpy_doc_cst = PyString_FromString ("__doc__");
   gdbpy_enabled_cst = PyString_FromString ("enabled");
   gdbpy_value_cst = PyString_FromString ("value");
+  gdbpy_level_cst = PyString_FromString ("level");
 
   /* Release the GIL while gdb runs.  */
   PyThreadState_Swap (NULL);
-- 
1.7.9.5

Re: patch for #14363

[Phil Muldoon]

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

Re: patch for #14363

[Oliver Buchtala]

On 06.08.2012 12:42, Phil Muldoon wrote:
> 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
Thanks,

I see there are lots of deficiencies with my patch. Sorry for that.
You are right in all your points.
I was not aware that recurse == indentation. (misnaming? ;) )

For the second proposal, I need a set data structure for longs.
Do you have such a data structure in gdb somewhere? Or which should I use?
Then I would offer a prototype implementation.

Regards,
Oliver

Re: patch for #14363

[Sergio Durigan Junior]

On Monday, August 06 2012, Phil Muldoon wrote:

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

And also, since I am dealing with these matters as of now, you will also
need a copyright assignment (I don't see your name on gdb/MAINTAINERS).
Please drop an e-mail offlist and I can send you the form.

-- 
Sergio

Re: patch for #14363

[Tom Tromey]

>>>>> "Oliver" == Oliver Buchtala <oliver.buchtala@googlemail.com> writes:

Oliver> this patch can be used to achieve a workaround to bug
Oliver> http://sourceware.org/bugzilla/show_bug.cgi?id=14363 Besides
Oliver> being a workaround, it also is a new feature, enabling a
Oliver> pretty-printer to control the pretty-printing on deeper
Oliver> recursion levels.

I think I'd prefer some kind of "initialization" notification to the
printers.  This could just be an optional method that they implement.

Tom