RFC: fix PR 14386

RFC: fix PR 14386

[Tom Tromey]

This fixes PR python/14386.

I'd like to commit this to the trunk and the 7.5 branch; the latter
because it is a reasonably obvious, low-risk, and useful bug fix.

The bug here is that a certain libstdc++ pretty-printer doesn't work in
MI.

What is going on is that the printer in question returns a Python list
object from its 'children' method.  This is perfectly fine.  However,
the varobj code uses PyIter_Check on the returned object, and this
evaluates to false for a list.

I think this is arguably a bug in Python, so I filed:

    http://bugs.python.org/issue15529

Meanwhile, it seems safest not to call PyIter_Check.  We certainly don't
need to -- the CLI doesn't -- because we can just call PyObject_GetIter
and react appropriately if that fails.

Built and tested on x86-64 Fedora 16.
New test case included.

Tom

commit 4074f681d23d3ee12c92bbe128e55f9e0d5a142e
Author: Tom Tromey <tromey@redhat.com>
Date:   Wed Aug 1 10:30:11 2012 -0600

    fix PR python/14386
    
    	PR python/14386:
    	* varobj.c (update_dynamic_varobj_children): Don't call
    	PyIter_Check.
    
    	* gdb.python/py-mi.exp: Add test for printer whose children
    	are a list.
    	* gdb.python/py-prettyprint.c (struct children_as_list): New.
    	(main): New variable children_as_list.
    	* gdb.python/py-prettyprint.py (class pp_children_as_list):
    	New.
    	(register_pretty_printers): Register new printer.

diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp
index a792e44..e7034a1 100644
--- a/gdb/testsuite/gdb.python/py-mi.exp
+++ b/gdb/testsuite/gdb.python/py-mi.exp
@@ -289,6 +289,10 @@ mi_gdb_test "-var-evaluate-expression me" \
 	"\\^done,value=\"<error reading variable: Cannot access memory.>.*\"" \
 	"evaluate me varobj"
 
+# Regression test for python/14836.
+mi_create_dynamic_varobj children_as_list children_as_list \
+    "printer whose children are returned as a list"
+
 # C++ MI tests
 gdb_exit
 if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}-cxx" \
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.c b/gdb/testsuite/gdb.python/py-prettyprint.c
index 1ff9e05..b1a12b1 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.c
+++ b/gdb/testsuite/gdb.python/py-prettyprint.c
@@ -48,6 +48,10 @@ struct hint_error {
   int x;
 };
 
+struct children_as_list {
+  int x;
+};
+
 #ifdef __cplusplus
 struct S : public s {
   int zs;
@@ -252,6 +256,7 @@ main ()
   struct ns ns, ns2;
   struct lazystring estring, estring2;
   struct hint_error hint_error;
+  struct children_as_list children_as_list;
 
   nstype.elements = narray;
   nstype.len = 0;
diff --git a/gdb/testsuite/gdb.python/py-prettyprint.py b/gdb/testsuite/gdb.python/py-prettyprint.py
index b02b90f..6e960e6 100644
--- a/gdb/testsuite/gdb.python/py-prettyprint.py
+++ b/gdb/testsuite/gdb.python/py-prettyprint.py
@@ -174,6 +174,18 @@ class pp_hint_error:
     def display_hint (self):
         raise Exception("hint failed")
 
+class pp_children_as_list:
+    "Throw error from display_hint"
+
+    def __init__(self, val):
+        self.val = val
+
+    def to_string(self):
+        return 'children_as_list_val'
+
+    def children (self):
+        return [('one', 1)]
+
 class pp_outer:
     "Print struct outer"
 
@@ -282,6 +294,9 @@ def register_pretty_printers ():
     pretty_printers_dict[re.compile ('^struct hint_error$')]  = pp_hint_error
     pretty_printers_dict[re.compile ('^hint_error$')]  = pp_hint_error
 
+    pretty_printers_dict[re.compile ('^struct children_as_list$')]  = pp_children_as_list
+    pretty_printers_dict[re.compile ('^children_as_list$')]  = pp_children_as_list
+
     pretty_printers_dict[re.compile ('^memory_error$')]  = MemoryErrorString
 
     pretty_printers_dict[re.compile ('^eval_type_s$')] = pp_eval_type
diff --git a/gdb/varobj.c b/gdb/varobj.c
index a75a40d..71b6436 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1114,9 +1114,6 @@ update_dynamic_varobj_children (struct varobj *var,
 
       make_cleanup_py_decref (children);
 
-      if (!PyIter_Check (children))
-	error (_("Returned value is not iterable"));
-
       Py_XDECREF (var->child_iter);
       var->child_iter = PyObject_GetIter (children);
       if (!var->child_iter)

Re: RFC: fix PR 14386

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I think this is arguably a bug in Python, so I filed:
Tom>     http://bugs.python.org/issue15529

Well, I was totally and embarrassingly wrong about that, but the rest of
the message still seems ok.

Tom

Re: RFC: fix PR 14386

[Pedro Alves]

On 08/01/2012 08:29 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> I think this is arguably a bug in Python, so I filed:
> Tom>     http://bugs.python.org/issue15529
> 
> Well, I was totally and embarrassingly wrong about that, but the rest of
> the message still seems ok.

:-)

In light of that, the patch looks obvious even to me.

-- 
Pedro Alves

Re: RFC: fix PR 14386

[Phil Muldoon]

On 08/01/2012 05:42 PM, Tom Tromey wrote:
> This fixes PR python/14386.
> 
> I'd like to commit this to the trunk and the 7.5 branch; the latter
> because it is a reasonably obvious, low-risk, and useful bug fix.
> 
> The bug here is that a certain libstdc++ pretty-printer doesn't work in
> MI.

Looks great to me.  The only comment I have has nothing to do with your
patch, but I will comment here for context.

It seems we wind-up printing two errors if the value returned to
"children" is not an iterator.  The "type error" exception that is
printed with gdbpy_print_stack, and then the explicit error call we call
right after.  Not sure if this is because of an MI detail needing an
"error" call, but it seems counter-intuitive to print two error
messages for one exception.  Maybe in a future patch we could extract
the exception message from the exception and pass that to the GDB
error call, or skip the error call completely.

Cheers,

Phil

Re: RFC: fix PR 14386

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> It seems we wind-up printing two errors if the value returned to
Phil> "children" is not an iterator.  The "type error" exception that is
Phil> printed with gdbpy_print_stack, and then the explicit error call we call
Phil> right after.  Not sure if this is because of an MI detail needing an
Phil> "error" call, but it seems counter-intuitive to print two error
Phil> messages for one exception.  Maybe in a future patch we could extract
Phil> the exception message from the exception and pass that to the GDB
Phil> error call, or skip the error call completely.

There's PR 12174, which is about this.  The basic problem is that the
exception conversion process is not loss-less.  This is fixable by
adding a bit of state to gdb exceptions, and a little hacking here and
there; but nobody has attempted it comprehensively yet.

Tom

Re: RFC: fix PR 14386

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I'd like to commit this to the trunk and the 7.5 branch; the latter
Tom> because it is a reasonably obvious, low-risk, and useful bug fix.

After Jan's note today, I decided against putting this into 7.5.
It is not a regression.
I think the other fixes I have proposed for 7.5 are for regressions,
aside from the missing cleanup -- but that is already in.

Tom

Re: RFC: fix PR 14386

[Joel Brobecker]

> Tom> I'd like to commit this to the trunk and the 7.5 branch; the latter
> Tom> because it is a reasonably obvious, low-risk, and useful bug fix.
> 
> After Jan's note today, I decided against putting this into 7.5.
> It is not a regression.

If it is deemed extra safe, and useful, I don't see a problem.
But I am fine either way.

-- 
Joel

Re: RFC: fix PR 14386

[Tom Tromey]

Joel> If it is deemed extra safe, and useful, I don't see a problem.
Joel> But I am fine either way.

I think it is safe, but I think people other than me ought to say so.

Tom

Re: RFC: fix PR 14386

[Joel Brobecker]

> Joel> If it is deemed extra safe, and useful, I don't see a problem.
> Joel> But I am fine either way.
> 
> I think it is safe, but I think people other than me ought to say so.

Pedro said it seemed obvious even to him. I agree it seems correct
as well. I would put it in.

-- 
Joel

Re: RFC: fix PR 14386

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Pedro said it seemed obvious even to him. I agree it seems correct
Joel> as well. I would put it in.

Ok, I checked it in.

Tom