Re: [PATCH] Fix PR gdb/17820

[Pedro Alves <palves@redhat.com>]

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