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

Peter Linss
2017-05-25T17:42:19Z

> 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