From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49831 invoked by alias); 25 May 2017 17:42:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 49806 invoked by uid 89); 25 May 2017 17:42:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=didn=e2, didn, inspecting, You've?= X-HELO: smtp.elemental.software Received: from smtp.elemental.software (HELO smtp.elemental.software) (45.33.60.119) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 May 2017 17:42:17 +0000 Received: from [IPv6:2001:470:879a::d847:c47a:9f25:c1a] (unknown [IPv6:2001:470:879a:0:d847:c47a:9f25:c1a]) (Authenticated sender: peter@elemental.software) by smtp.elemental.software (Postfix) with ESMTPSA id ECE5926609; Thu, 25 May 2017 10:42:18 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] Fix usage of to_string() for pretty-printers with children From: Peter Linss In-Reply-To: Date: Thu, 25 May 2017 17:42:00 -0000 Cc: gdb-patches@sourceware.org Content-Transfer-Encoding: quoted-printable Message-Id: <9800765F-7128-49EE-A162-428DFA67DF75@elemental.software> References: To: Phil Muldoon X-SW-Source: 2017-05/txt/msg00549.txt.bz2 > On May 25, 2017, at 3:06 AM, Phil Muldoon wrote: >=20 > On 25/05/17 03:33, Peter Linss wrote: >=20 >=20 >=20 >> gdb/ChangeLog: >>=20 >> * varobj.c (varobj_value_get_print_value): Call pretty-printer >> to_string method for value if present even when children=20 >> method is available. >> (dynamic_varobj_has_child_method) Remove unused function. >>=20 >> gdb/doc/ChangeLog: >>=20 >> * gdb.texinfo (Variable Objects, Result): Update description of >> value to reflect to_string output. >=20 > Thanks. >=20 >>=20 >> -#if HAVE_PYTHON >> - >> -static int >> -dynamic_varobj_has_child_method (const struct varobj *var) >> -{ >> - PyObject *printer =3D var->dynamic->pretty_printer; >> - >> - if (!gdb_python_initialized) >> - return 0; >> - >> - gdbpy_enter_varobj enter_py (var); >> - return PyObject_HasAttr (printer, gdbpy_children_cst); >> -} >> -#endif >> - >=20 > 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. >=20 >=20 >> /* A factory for creating dynamic varobj's iterators. Returns an >> iterator object suitable for iterating over VAR's children. */ >>=20 >> @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value, >>=20 >> 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 =3D 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 "{...}"; >> + } >=20 > 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 alr= eady done within the only function that called it. The gdb_python_initialized test is already performed on line 2400 (after th= e patch), pretty_printer is already copied form var->dynamic (line 2402), a= nd there=E2=80=99s already a 'gdbpy_enter_varobj enter_py (var)=E2=80=99 on= line 2404, which is still in scope. Aside from the actual call to PyObject_HasAttr(value_formatter, gdbpy_child= ren_cst) everything in the function appeared redundant, and there=E2=80=99s= also a call to PyObject_HasAttr(value_formatter, gdbpy_to_string_cst) on l= ine 2408 which I presume is protected by the appropriate safety calls. If I=E2=80=99m missing something, it would be trivial to leave the dynamic_= varobj_has_child_method function and call it instead of PyObject_HasAttr, t= he necessary change here is shifting the logic so that to_string always get= s called if present, and the default return value of =E2=80=9C{...}=E2=80= =9D only gets returned if there isn=E2=80=99t a to_string method on the pre= tty-printer, everything else is just cleanup. >=20 > 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=E2=80=99s present, where previousl= y the call to to_string would be skipped if the pretty-printer has a childr= en method. Note that this is the same behavior when using a pretty printer = in the regular interpreter, so pretty-printer authors should already be awa= re of it. >=20 > 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=E2=80=99t find any, a= nd I have no idea how to integrate a pretty-printer into the testing enviro= nment, if there=E2=80=99s something you can point me to I=E2=80=99d be happ= y to write (or modify) a test as needed. FWIW I did test this both under Ec= lipse, SublimeGDB, and manually running the mi interpreter (though I, of co= urse, 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 =E2=80=9C{.= ..}=E2=80=9D for the value of variables that have children, if the pretty-p= rinter 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 in= terpreter). And thanks for the prompt review, Peter >=20 > Cheers >=20 > Phil