Fix PR gdb/17820

  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.
[Alternatively I can just initialize the variable to 256 in the first
place.  Would that be better?]

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.
---
 gdb/top.c | 53 ++++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)
  

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
  

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
>
  

On 04/28/2015 02:05 AM, Patrick Palka wrote:
> On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote:

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

Indeed, looks like that has already been the case...

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

That's good.  Maybe add the suggestion to
use "set history size BIGINT" in .gdbinit, if compatibility
with previous releases is desired.

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

It may be testable with -x or -ix on the command line too, not
sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though
given "set history size" already behaves different today depending on when
it is called, an alternate way to test the issue that happens to use
the same path today may change in the future, and we may (re)introducing
unnoticed bugs.  So I think we should test that path exactly.  I was
thinking of creating a dir, put a test .gdbinit file there, and point
HOME at that dir.  We'd just skip the test on remote host testing.

Thanks,
Pedro Alves
  

On Wed, Apr 29, 2015 at 6:44 AM, Pedro Alves <palves@redhat.com> wrote:
> On 04/28/2015 02:05 AM, Patrick Palka wrote:
>> On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves <palves@redhat.com> wrote:
>
>>> 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.
>
> Indeed, looks like that has already been the case...
>
>>> 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?
>
> That's good.  Maybe add the suggestion to
> use "set history size BIGINT" in .gdbinit, if compatibility
> with previous releases is desired.
>
>>>>       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?
>
> It may be testable with -x or -ix on the command line too, not
> sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though
> given "set history size" already behaves different today depending on when
> it is called, an alternate way to test the issue that happens to use
> the same path today may change in the future, and we may (re)introducing
> unnoticed bugs.  So I think we should test that path exactly.  I was
> thinking of creating a dir, put a test .gdbinit file there, and point
> HOME at that dir.  We'd just skip the test on remote host testing.

I tried this but the problem is that the testsuite seems to always
pass -nx to invocations of GDB meaning that .gdbinit files are not
read.  Would you know how to work around this?

>
> Thanks,
> Pedro Alves
>
  

On 05/12/2015 12:30 PM, Patrick Palka wrote:
> On Wed, Apr 29, 2015 at 6:44 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 04/28/2015 02:05 AM, Patrick Palka wrote:

>>>> 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?
>>
>> It may be testable with -x or -ix on the command line too, not
>> sure, gdb.base/bp-cmds-execution-x-script.exp is an example, though
>> given "set history size" already behaves different today depending on when
>> it is called, an alternate way to test the issue that happens to use
>> the same path today may change in the future, and we may (re)introducing
>> unnoticed bugs.  So I think we should test that path exactly.  I was
>> thinking of creating a dir, put a test .gdbinit file there, and point
>> HOME at that dir.  We'd just skip the test on remote host testing.
> 
> I tried this but the problem is that the testsuite seems to always
> pass -nx to invocations of GDB meaning that .gdbinit files are not
> read.  Would you know how to work around this?
> 

Thanks for working on this.

Maybe strip -nx out of INTERNAL_GDBFLAGS temporarily.  E.g.,:

 set saved_internal_gdbflags $INTERNAL_GDBFLAGS
 set INTERNAL_GDBFLAGS [string map {"-nx " ""} $INTERNAL_GDBFLAGS]

 ... start gdb here ...

 set INTERNAL_GDBFLAGS $saved_internal_gdbflags

Thanks,
Pedro Alves