From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97013 invoked by alias); 14 Jun 2017 02:36:56 -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 96999 invoked by uid 89); 14 Jun 2017 02:36:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=You've, Youve, H*m:software, isn 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; Wed, 14 Jun 2017 02:36:53 +0000 Received: from [IPv6:2001:470:879a::dd6f:d986:54e4:f9e0] (unknown [IPv6:2001:470:879a:0:dd6f:d986:54e4:f9e0]) (Authenticated sender: peter@elemental.software) by smtp.elemental.software (Postfix) with ESMTPSA id 3DE88267FE for ; Tue, 13 Jun 2017 19:36:56 -0700 (PDT) From: Peter Linss Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] Fix usage of to_string() for pretty-printers with children Date: Wed, 14 Jun 2017 02:36:00 -0000 References: <9800765F-7128-49EE-A162-428DFA67DF75@elemental.software> To: gdb-patches@sourceware.org In-Reply-To: <9800765F-7128-49EE-A162-428DFA67DF75@elemental.software> Message-Id: <77B1FB88-240E-425E-8692-236108652B6F@elemental.software> X-SW-Source: 2017-06/txt/msg00414.txt.bz2 > On May 25, 2017, at 10:42 AM, Peter Linss wrot= e: >=20 >>=20 >> 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? >=20 > Yes, see below. >=20 >>=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 *valu= e, >>>=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 *valu= e, >>> 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. >=20 > As far as I could tell, everything done within the removed function was a= lready 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=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_chi= ldren_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 line 2408 which I presume is protected by the appropriate safety calls. >=20 > If I=E2=80=99m missing something, it would be trivial to leave the dynami= c_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 g= ets 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 >>=20 >> Also, if the to_string call has changed, it might constitute an API >> change. >=20 > 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 previou= sly the call to to_string would be skipped if the pretty-printer has a chil= dren method. Note that this is the same behavior when using a pretty printe= r in the regular interpreter, so pretty-printer authors should already be a= ware of it. >=20 >>=20 >> And, alas, all changes need tests, unless obvious, and the outcome of >> this test is not obvious (well to me ;) ) >=20 > I looked for tests involving pretty-printers and didn=E2=80=99t find any,= and I have no idea how to integrate a pretty-printer into the testing envi= ronment, if there=E2=80=99s something you can point me to I=E2=80=99d be ha= ppy 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). >=20 > The only change this introduces is when inspecting pretty-printed variabl= es 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= -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). >=20 > And thanks for the prompt review, >=20 > Peter >=20 >>=20 >> Cheers >>=20 >> Phil Ping.