Re: [PATCH] Fix PR gdb/17820

[Pedro Alves <palves at redhat dot com>]

On 04/26/2015 07:41 PM, Patrick Palka wrote:
> This patch is a comprehensive fix for PR 17820 which reports that
> using "set history size unlimited" inside one's gdbinit file doesn't
> really work.
> 
> There are three small changes in this patch.  The most important change
> this patch makes is to decode the argument of the "size" subcommand
> using add_setshow_zuinteger_unlimited_cmd() instead of using
> add_setshow_uinteger_cmd().  The new decoder takes an int * and maps
> unlimited to -1 whereas the old decoder takes an unsigned int * and maps
> unlimited to UINT_MAX.  Using the new decoder simplifies our handling of
> unlimited and makes it easier to interface with readline which itself
> expects a signed-int history size.
> 
> The second change is the factoring of the [stifle|unstifle]_history logic
> into a common function which is now used by both init_history() and
> set_history_size_command().  This is technically the change that fixes
> the PR itself.
> 
> Thirdly, this patch initializes history_size_setshow_var to -2 to mean
> that the variable has not been set yet.  Now init_history() tests for -2
> instead of 0 to determine whether to give the variable a default value.
> This means that having "set history size 0" in one's gdbinit file will
> actually keep the history size at 0 and not reset it to 256.

Please see also:

  https://sourceware.org/ml/gdb-patches/2013-03/msg00962.html

for background.

Darn.  So around that time, we added support for explicit "unlimited"
to a bunch of commands.  Since "set history size 0" already meant
unlimited before, and since this is the sort of command that users
put in .gdbinit instead of issuing manually, it was reasonable
to assume if we changed the "unlimited" representation, we'd
break user scripts.  So the idea was that we'd let a few years/releases
go by, and then we'd change "size=0" to really mean 0, and we'd
tell users to use "set history size unlimited" instead.

But now we see that "set history size unlimited" in .gdbinit never
really worked.  Bummer.  So this means that users must have kept
using "set history size 0" instead...

So if we change this now, there's no way to have a single
"set history size FOO" setting that means unlimited with
both gdb <= 7.9 and the next gdb release.  :-/  Oh well.  Maybe we should
just tell users to do "set history size BIGINT" instead?

I'd definitely like to make "set history size 0" really
disable history.

So I think that if goes forward, it'd be good to have a NEWS entry.

What do you (and others) think?

> [Alternatively I can just initialize the variable to 256 in the first
> place.  Would that be better?]

-2 is fine with me.

> 
> gdb/ChangeLog:
> 
> 	PR gdb/17820
> 	* top.c (history_size_setshow_var): Change type to signed.
> 	Initialize to -2.  Update documentation.
> 	(set_readline_history_size): Define.
> 	(set_history_size_command): Use it.  Remove logic for handling
> 	out-of-range sizes.
> 	(init_history): Use set_readline_history_size().  Test for a
> 	value of -2 instead of 0 when determining whether to set a
> 	default history size.
> 	(init_main): Decode the argument of the "size" command as a
> 	zuinteger_unlimited.

Looks good to me.

Adding a testcase would be ideal, but I'll not make it a requirement.
I think we should be able to write one making use of GDBFLAGSs.  (and
IWBN to test the  GDBHISTSIZE/HISTSIZE environment variables too, which
we can do with "set env(HISTSIZE)", etc.)

Thanks,
Pedro Alves