From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [209.51.188.92]) by sourceware.org (Postfix) with ESMTPS id 69BB7385840C for ; Thu, 6 Jan 2022 13:32:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 69BB7385840C Received: from [2001:470:142:3::e] (port=43582 helo=fencepost.gnu.org) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n5SsA-0003TJ-NQ; Thu, 06 Jan 2022 08:32:06 -0500 Received: from ip5f5a8d35.dynamic.kabel-deutschland.de ([95.90.141.53]:56783 helo=[192.168.111.41]) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1n5SsA-00071K-Ui; Thu, 06 Jan 2022 08:32:07 -0500 Message-ID: Date: Thu, 6 Jan 2022 14:32:02 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Content-Language: en-US To: Andrew Burgess Cc: gdb-patches@sourceware.org References: <111e4bb6-d781-ff80-f64b-125ad665f502@web.de> <20220106093322.GD828155@redhat.com> From: Simon Sobisch Subject: Re: [PATCH v2] gdb: split array and string limiting options In-Reply-To: <20220106093322.GD828155@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jan 2022 13:32:09 -0000 Am 06.01.2022 um 10:33 schrieb Andrew Burgess: > * Simon Sobisch via Gdb-patches [2022-01-06 08:58:38 +0100]: > >>> From: Andrew Burgess >>> >>> This commit splits the set/show print elements options into two. We >>> retain set/show print elements for controlling how many elements of an >>> array we print, but a new set/show print characters is added which is >>> used for controlling how many characters of a string are printed. >>> [...] >> Reference to the full patch: >> https://sourceware.org/pipermail/gdb-patches/2021-December/184467.html >> >> >> I really like the general idea and would like to see this added, but I >> think the patch goes too far and will break both old and new usage. > > I don't understand what "new usage" means here. Thanks for checking, that was unclear. With "new usage" I've meant that if someone temporarily sets the new variable in an extension there is no way to get back. The "-1 magic value" I've suggested would also allow a temporary setting outside of "with print characters ". That actually reminds me of a documentation question I had: Does the implementation actually return the amount of characters (especially when the char array which is printed from has multiple multi-byte characters with combining symbols)? >> In my opinion there should be only 1 new test for the new setting and >> the other should be left unchanged while still working, because then >> existing usages will still work, too. >> >> To do so the new parameter cannot be directly used as it is in the patch >> but must be fowarded to a new function or duplicated. >> >> As an example this would mean instead of : >> >> --- src.orig/gdb/c-valprint.c >> +++ src/gdb/c-valprint.c >> @@ -271,7 +271,7 @@ c_value_print_array (struct value *val, >> >> for (temp_len = 0; >> (temp_len < len >> - && temp_len < options->print_max >> + && temp_len < options->print_max_chars >> && extract_unsigned_integer (valaddr + temp_len * eltlen, >> eltlen, byte _order) != 0); >> ++temp_len) >> >> have the following >> >> >> + const int print_max_chars = (options->print_max_chars != -1) ? >> options->print_max_chars : options->print_max; >> for (temp_len = 0; >> (temp_len < len >> - && temp_len < options->print_max >> + && temp_len < print_max_chars >> && extract_unsigned_integer (valaddr + temp_len * eltlen, >> eltlen, byte_order) != 0); >> ++temp_len) > > What follows is just my thoughts on what you propose. This work was > being done for a client of Embecosm's, and Maciej is now driving this > patch, so what he does is up to him. That said... > > I did consider this when I originally wrote the patch. But decided > against it in the end. > > I agree that maintaining backwards compatibility is important. :-) > But I think we should be careful about going over the top to avoid breaking > changes. I think that depends on the actual implementation effort and additional technical dept introduced. > IMHO GDB script is not equivalent to something like C, C++, Python, > etc, where I would certainly expect complete backwards compatibility. Commonly GDB _adds_ new options. This allows to improve GDB scripts to get the new features, but it would already be bad if existing scripts don't work any more. The thing I'm much more interested in here isn't a simple GDB script but GDB extensions (Python, likely also Guile but I haven't worked with that) and MI-Clients. Those extensions can get quite large and breaking those is therefore much worse than breaking a simple GDB script. It is not unlikely that you'll find some that temporarily set elements to a big value or even unlimited to present the user the full string, with the patch applied that's immediately broken as changing this value has no effect at all for the strings. > Instead I think we should aim to strike a balance between maintaining > backward compatibility, and keeping a simple, easy to understand > interface. Agreed. > For me, changing the behaviour of this option didn't seem that crazy, > it's pretty easy to restore the old behaviour by also setting the new > option. ... for which you'd need (as a "simple user") to wait for your MI-client or Python extension's author to change their software and hope you have access to the previous version of GDB to still be able to do your debugging. > Maybe the help text could do a better job of cross > referencing the other option, to make discovery easier. Possibly. With my suggested change the news entry would say that the new setting is undefined (or off = -1) and so the old setting is applied. > Anyway, just me thoughts. I'm certainly not going to block the patch > if it is rewritten inline with your suggestion. I hope it will be - and doing so will also heavily reduce the amount of changes in the patch - as all old testsuite entries can stay as they are. >> This effectively means that until you explicit set the new "print >> characters" the existing "print elements" setting will be used, and if >> you "set print characters off" (or similar, that would internally >> resolve to -1) you get the length depending on "print elements" again. >> >> The parameter type in python for "print elements" is (guessed) >> gdb.PARAM_UINTEGER, the type for "print characters" would be >> gdb.PARAM_INTEGER with -1 meaning "take the other parameter". >> >> >> If those changes are done there is no breakage whatever which could also >> speedup the inclusion s it hopefully lands in GDB 11.2. > > Maybe I'm not understanding how releases work, but I thought 11.2 was > a bug fix release over 11.1, and a future 12 branch would be the new > release in which new features would land. Is that not how things > work? Depends on the project. In GDB Announcements of minor versions the introduction commonly is "brings the following fixes and enhancements". In the past there were also bigger new features (for example related to python) and as long as enhancements don't break backward compatibility at all... But I actually don't know if there is a policy about what goes in (now). > Thanks, > Andrew > Thanks for the initial work on the patch, for sharing your thoughts and request for clarification. I hope Maciej will find the time to work on this and that we may see this "earlier" than GDB 12. Simon