Re: [PATCH] Fix PR gdb/17820

[Patrick Palka <patrick@parcs.ath.cx>]

On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote:
> 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.

I too think the variable should be signed if only because it
simplifies the code.

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

"set history size 0" (in the .gdbinit) doesn't set history to
unlimited either -- it sets it to 256!  So users currently have no
choice but to use "set history size BIGINT" for
approximately-unlimited history.

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

But currently, neither "set history size 0" nor "set history size
unlimited" mean unlimited (in the .gdbinit).  So users today cannot
set an unlimited history size at all.  Changing this now will now at
least make "set history size unlimited".  So whether or not we change
this now, there will be no way to have a single "set history size FOO"
setting that means unlimited across current and future GDB versions.
Am I misunderstanding?

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

I can think of two things to mention.  One is that "set history size
0" now really disables history.  The other is that "set history size
unlimited" now really does not truncate history.  Do you have anything
else in mind?

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

Do you have in mind a test that creates a dummy .gdbinit file to be
read by GDB?  Or is there another way to test this code path?

>
> Thanks,
> Pedro Alves
>