Re: [PATCH v2] gdb: split array and string limiting options

[Simon Sobisch <simonsobisch@gnu.org>]

Am 06.01.2022 um 10:33 schrieb Andrew Burgess:
> * Simon Sobisch via Gdb-patches <gdb-patches@sourceware.org> [2022-01-06 08:58:38 +0100]:
> 
>>> From: Andrew Burgess <andrew.burgess@embecosm.com>
>>>
>>> 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