Fix usage of to_string() for pretty-printers with children

  Currently when using python pretty-printers that have children in the mi interpreter, the value of the variable object will always return “{...}”, this change will return the result of the pretty-printer’s to_string() method if present. If the pretty-printer has children but doesn’t have a to_string() method, then “{...}” will still be returned.

This allows pretty-printers to optionally return a brief summary of the variable in addition to the expanded list of children and makes the behavior under the mi interpreter match that of the regular interpreter.

This fixes bug 11335.

I also removed the dynamic_varobj_has_child_method function as it wasn't doing anything that hadn’t already been done in the varobj_value_get_print_value function and isn’t used elsewhere.

Tested with Eclipse and the libstdc++-v3 pretty-printer as well as custom pretty-printers for an in-house project and SublimeGDB (SublimeGDB required some additional changes to properly support pretty-printers but those are not related to this change).


gdb/ChangeLog:

	* varobj.c (varobj_value_get_print_value): Call pretty-printer
	to_string method for value if present even when children 
	method is available.
	(dynamic_varobj_has_child_method) Remove unused function.

gdb/doc/ChangeLog:

	* gdb.texinfo  (Variable Objects, Result): Update description of
	value to reflect to_string output.


---

--
  

On 25/05/17 03:33, Peter Linss wrote:

 
 
> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_value_get_print_value): Call pretty-printer
> 	to_string method for value if present even when children 
> 	method is available.
> 	(dynamic_varobj_has_child_method) Remove unused function.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo  (Variable Objects, Result): Update description of
> 	value to reflect to_string output.

Thanks.

>  
> -#if HAVE_PYTHON
> -
> -static int
> -dynamic_varobj_has_child_method (const struct varobj *var)
> -{
> -  PyObject *printer = var->dynamic->pretty_printer;
> -
> -  if (!gdb_python_initialized)
> -    return 0;
> -
> -  gdbpy_enter_varobj enter_py (var);
> -  return PyObject_HasAttr (printer, gdbpy_children_cst);
> -}
> -#endif
> -

In removing the above you are removing the Python environment call
which, among other things, ensures the state of the Python GIL. The
replacement hunk below does not make the same call? Is this
intentional?


>  /* A factory for creating dynamic varobj's iterators.  Returns an
>     iterator object suitable for iterating over VAR's children.  */
>  
> @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value,
>  
>        if (value_formatter)
>  	{
> -	  /* First check to see if we have any children at all.  If so,
> -	     we simply return {...}.  */
> -	  if (dynamic_varobj_has_child_method (var))
> -	    return "{...}";
> -
>  	  if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
>  	    {
>  	      struct value *replacement;
> @@ -2486,6 +2466,13 @@ varobj_value_get_print_value (struct value *value,
>  	      if (replacement)
>  		value = replacement;
>  	    }
> +	  else
> +	    {
> +	      /* If we don't have to_string but we have children,
> +		 we simply return {...}.  */
> +	      if (PyObject_HasAttr (value_formatter, gdbpy_children_cst))
> +		return "{...}";
> +	    }

You've removed a function but replaced it with an if (...) (and the
associated safety calls). I'm not sure this is the right thing to do.

Also, if the to_string call has changed, it might constitute an API
change.

And, alas, all changes need tests, unless obvious, and the outcome of
this test is not obvious (well to me ;) )

Cheers

Phil
  

> From: Peter Linss <peter@elemental.software>
> Date: Wed, 24 May 2017 19:33:41 -0700
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9fb70f6d2a..05e5b868ed 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -29444,8 +29444,9 @@ reliable for a dynamic varobj.  Instead, you must examine the
>  
>  @item value
>  The varobj's scalar value.  For a varobj whose type is some sort of
> -aggregate (e.g., a @code{struct}), or for a dynamic varobj, this value
> -will not be interesting.
> +aggregate (e.g., a @code{struct}) this value will not be interesting.
                                    ^
Please add back the comma removed here.

The documentation part is approved with this gotcha fixed.

Thanks.
  

> On May 25, 2017, at 3:06 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> 
> On 25/05/17 03:33, Peter Linss wrote:
> 
> 
> 
>> gdb/ChangeLog:
>> 
>> 	* varobj.c (varobj_value_get_print_value): Call pretty-printer
>> 	to_string method for value if present even when children 
>> 	method is available.
>> 	(dynamic_varobj_has_child_method) Remove unused function.
>> 
>> gdb/doc/ChangeLog:
>> 
>> 	* gdb.texinfo  (Variable Objects, Result): Update description of
>> 	value to reflect to_string output.
> 
> Thanks.
> 
>> 
>> -#if HAVE_PYTHON
>> -
>> -static int
>> -dynamic_varobj_has_child_method (const struct varobj *var)
>> -{
>> -  PyObject *printer = var->dynamic->pretty_printer;
>> -
>> -  if (!gdb_python_initialized)
>> -    return 0;
>> -
>> -  gdbpy_enter_varobj enter_py (var);
>> -  return PyObject_HasAttr (printer, gdbpy_children_cst);
>> -}
>> -#endif
>> -
> 
> In removing the above you are removing the Python environment call
> which, among other things, ensures the state of the Python GIL. The
> replacement hunk below does not make the same call? Is this
> intentional?

Yes, see below.

> 
> 
>> /* A factory for creating dynamic varobj's iterators.  Returns an
>>    iterator object suitable for iterating over VAR's children.  */
>> 
>> @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value,
>> 
>>       if (value_formatter)
>> 	{
>> -	  /* First check to see if we have any children at all.  If so,
>> -	     we simply return {...}.  */
>> -	  if (dynamic_varobj_has_child_method (var))
>> -	    return "{...}";
>> -
>> 	  if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
>> 	    {
>> 	      struct value *replacement;
>> @@ -2486,6 +2466,13 @@ varobj_value_get_print_value (struct value *value,
>> 	      if (replacement)
>> 		value = replacement;
>> 	    }
>> +	  else
>> +	    {
>> +	      /* If we don't have to_string but we have children,
>> +		 we simply return {...}.  */
>> +	      if (PyObject_HasAttr (value_formatter, gdbpy_children_cst))
>> +		return "{...}";
>> +	    }
> 
> You've removed a function but replaced it with an if (...) (and the
> associated safety calls). I'm not sure this is the right thing to do.

As far as I could tell, everything done within the removed function was already done within the only function that called it.
The gdb_python_initialized test is already performed on line 2400 (after the patch), pretty_printer is already copied form var->dynamic (line 2402), and there’s already a 'gdbpy_enter_varobj enter_py (var)’ on line 2404, which is still in scope.
Aside from the actual call to PyObject_HasAttr(value_formatter, gdbpy_children_cst) everything in the function appeared redundant, and there’s also a call to PyObject_HasAttr(value_formatter, gdbpy_to_string_cst) on line 2408 which I presume is protected by the appropriate safety calls.

If I’m missing something, it would be trivial to leave the dynamic_varobj_has_child_method function and call it instead of PyObject_HasAttr, the necessary change here is shifting the logic so that to_string always gets called if present, and the default return value of “{...}” only gets returned if there isn’t a to_string method on the pretty-printer, everything else is just cleanup.

> 
> Also, if the to_string call has changed, it might constitute an API
> change.

Not sure what you mean here, nothing in the call to to_string has changed, only that it will always be called if it’s present, where previously the call to to_string would be skipped if the pretty-printer has a children method. Note that this is the same behavior when using a pretty printer in the regular interpreter, so pretty-printer authors should already be aware of it.

> 
> And, alas, all changes need tests, unless obvious, and the outcome of
> this test is not obvious (well to me ;) )

I looked for tests involving pretty-printers and didn’t find any, and I have no idea how to integrate a pretty-printer into the testing environment, if there’s something you can point me to I’d be happy to write (or modify) a test as needed. FWIW I did test this both under Eclipse, SublimeGDB, and manually running the mi interpreter (though I, of course, understand the value of automated testing).

The only change this introduces is when inspecting pretty-printed variables in a gdb UI, rather than the current behavior of always seeing “{...}” for the value of variables that have children, if the pretty-printer is returning a summary value in to_string (like an item count for a list) then that value is available to the GUI via the mi interpreter (as it would currently be displayed if the variable was printed in the regular interpreter).

And thanks for the prompt review,

Peter

> 
> Cheers
> 
> Phil
  

> On May 25, 2017, at 10:42 AM, Peter Linss <peter@elemental.software> wrote:
> 
>> 
>> On May 25, 2017, at 3:06 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
>> 
>> On 25/05/17 03:33, Peter Linss wrote:
>> 
>> 
>> 
>>> gdb/ChangeLog:
>>> 
>>> 	* varobj.c (varobj_value_get_print_value): Call pretty-printer
>>> 	to_string method for value if present even when children 
>>> 	method is available.
>>> 	(dynamic_varobj_has_child_method) Remove unused function.
>>> 
>>> gdb/doc/ChangeLog:
>>> 
>>> 	* gdb.texinfo  (Variable Objects, Result): Update description of
>>> 	value to reflect to_string output.
>> 
>> Thanks.
>> 
>>> 
>>> -#if HAVE_PYTHON
>>> -
>>> -static int
>>> -dynamic_varobj_has_child_method (const struct varobj *var)
>>> -{
>>> -  PyObject *printer = var->dynamic->pretty_printer;
>>> -
>>> -  if (!gdb_python_initialized)
>>> -    return 0;
>>> -
>>> -  gdbpy_enter_varobj enter_py (var);
>>> -  return PyObject_HasAttr (printer, gdbpy_children_cst);
>>> -}
>>> -#endif
>>> -
>> 
>> In removing the above you are removing the Python environment call
>> which, among other things, ensures the state of the Python GIL. The
>> replacement hunk below does not make the same call? Is this
>> intentional?
> 
> Yes, see below.
> 
>> 
>> 
>>> /* A factory for creating dynamic varobj's iterators.  Returns an
>>>   iterator object suitable for iterating over VAR's children.  */
>>> 
>>> @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value,
>>> 
>>>      if (value_formatter)
>>> 	{
>>> -	  /* First check to see if we have any children at all.  If so,
>>> -	     we simply return {...}.  */
>>> -	  if (dynamic_varobj_has_child_method (var))
>>> -	    return "{...}";
>>> -
>>> 	  if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
>>> 	    {
>>> 	      struct value *replacement;
>>> @@ -2486,6 +2466,13 @@ varobj_value_get_print_value (struct value *value,
>>> 	      if (replacement)
>>> 		value = replacement;
>>> 	    }
>>> +	  else
>>> +	    {
>>> +	      /* If we don't have to_string but we have children,
>>> +		 we simply return {...}.  */
>>> +	      if (PyObject_HasAttr (value_formatter, gdbpy_children_cst))
>>> +		return "{...}";
>>> +	    }
>> 
>> You've removed a function but replaced it with an if (...) (and the
>> associated safety calls). I'm not sure this is the right thing to do.
> 
> As far as I could tell, everything done within the removed function was already done within the only function that called it.
> The gdb_python_initialized test is already performed on line 2400 (after the patch), pretty_printer is already copied form var->dynamic (line 2402), and there’s already a 'gdbpy_enter_varobj enter_py (var)’ on line 2404, which is still in scope.
> Aside from the actual call to PyObject_HasAttr(value_formatter, gdbpy_children_cst) everything in the function appeared redundant, and there’s also a call to PyObject_HasAttr(value_formatter, gdbpy_to_string_cst) on line 2408 which I presume is protected by the appropriate safety calls.
> 
> If I’m missing something, it would be trivial to leave the dynamic_varobj_has_child_method function and call it instead of PyObject_HasAttr, the necessary change here is shifting the logic so that to_string always gets called if present, and the default return value of “{...}” only gets returned if there isn’t a to_string method on the pretty-printer, everything else is just cleanup.
> 
>> 
>> Also, if the to_string call has changed, it might constitute an API
>> change.
> 
> Not sure what you mean here, nothing in the call to to_string has changed, only that it will always be called if it’s present, where previously the call to to_string would be skipped if the pretty-printer has a children method. Note that this is the same behavior when using a pretty printer in the regular interpreter, so pretty-printer authors should already be aware of it.
> 
>> 
>> And, alas, all changes need tests, unless obvious, and the outcome of
>> this test is not obvious (well to me ;) )
> 
> I looked for tests involving pretty-printers and didn’t find any, and I have no idea how to integrate a pretty-printer into the testing environment, if there’s something you can point me to I’d be happy to write (or modify) a test as needed. FWIW I did test this both under Eclipse, SublimeGDB, and manually running the mi interpreter (though I, of course, understand the value of automated testing).
> 
> The only change this introduces is when inspecting pretty-printed variables in a gdb UI, rather than the current behavior of always seeing “{...}” for the value of variables that have children, if the pretty-printer is returning a summary value in to_string (like an item count for a list) then that value is available to the GUI via the mi interpreter (as it would currently be displayed if the variable was printed in the regular interpreter).
> 
> And thanks for the prompt review,
> 
> Peter
> 
>> 
>> Cheers
>> 
>> Phil

Ping.