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

[Andrew Burgess <aburgess@redhat.com>]

* 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.

> 
> 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.

IMHO GDB script is not equivalent to something like C, C++, Python,
etc, where I would certainly expect complete backwards compatibility.

Instead I think we should aim to strike a balance between maintaining
backward compatibility, and keeping a simple, easy to understand
interface.

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.  Maybe the help text could do a better job of cross
referencing the other option, to make discovery easier.

Anyway, just me thoughts.  I'm certainly not going to block the patch
if it is rewritten inline with your suggestion.

> 
> 
> 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?

Thanks,
Andrew