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

Peter Linss
2017-06-14T02:36:56Z

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