Re: [PATCH] Fix usage of to_string() for pretty-printers with children

[Peter Linss <peter@elemental.software>]

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