[PATCH] Fix PR gdb/17820

[PATCH] Fix PR gdb/17820

[Patrick Palka]

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

diff --git a/gdb/top.c b/gdb/top.c
index ddf5415..fb8e9e6 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -695,8 +695,8 @@ show_write_history_p (struct ui_file *file, int from_tty,
 }
 
 /* The variable associated with the "set/show history size"
-   command.  */
-static unsigned int history_size_setshow_var;
+   command.  The value -1 means unlimited, and -2 means undefined.  */
+static int history_size_setshow_var = -2;
 
 static void
 show_history_size (struct ui_file *file, int from_tty,
@@ -1618,34 +1618,26 @@ show_commands (char *args, int from_tty)
     }
 }
 
+/* Update the size of our command history file to HISTORY_SIZE.
+
+   A HISTORY_SIZE of -1 stands for unlimited.  */
+
+static void
+set_readline_history_size (int history_size)
+{
+  gdb_assert (history_size >= -1);
+
+  if (history_size == -1)
+    unstifle_history ();
+  else
+    stifle_history (history_size);
+}
+
 /* Called by do_setshow_command.  */
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  /* Readline's history interface works with 'int', so it can only
-     handle history sizes up to INT_MAX.  The command itself is
-     uinteger, so UINT_MAX means "unlimited", but we only get that if
-     the user does "set history size 0" -- "set history size <UINT_MAX>"
-     throws out-of-range.  */
-  if (history_size_setshow_var > INT_MAX
-      && history_size_setshow_var != UINT_MAX)
-    {
-      unsigned int new_value = history_size_setshow_var;
-
-      /* Restore previous value before throwing.  */
-      if (history_is_stifled ())
-	history_size_setshow_var = history_max_entries;
-      else
-	history_size_setshow_var = UINT_MAX;
-
-      error (_("integer %u out of range"), new_value);
-    }
-
-  /* Commit the new value to readline's history.  */
-  if (history_size_setshow_var == UINT_MAX)
-    unstifle_history ();
-  else
-    stifle_history (history_size_setshow_var);
+  set_readline_history_size (history_size_setshow_var);
 }
 
 void
@@ -1713,12 +1705,10 @@ init_history (void)
       history_size_setshow_var = var;
     }
   /* If the init file hasn't set a size yet, pick the default.  */
-  else if (history_size_setshow_var == 0)
+  else if (history_size_setshow_var == -2)
     history_size_setshow_var = 256;
 
-  /* Note that unlike "set history size 0", "HISTSIZE=0" really sets
-     the history size to 0...  */
-  stifle_history (history_size_setshow_var);
+  set_readline_history_size (history_size_setshow_var);
 
   tmpenv = getenv ("GDBHISTFILE");
   if (tmpenv)
@@ -1867,7 +1857,8 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_uinteger_cmd ("size", no_class, &history_size_setshow_var, _("\
+  add_setshow_zuinteger_unlimited_cmd ("size", no_class,
+				       &history_size_setshow_var, _("\
 Set the size of the command history,"), _("\
 Show the size of the command history,"), _("\
 ie. the number of previous commands to keep a record of.\n\
-- 
2.4.0.rc2.31.g7c597ef

Re: [PATCH] Fix PR gdb/17820

[Pedro Alves]

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

Re: [PATCH] Fix PR gdb/17820

[Patrick Palka]

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
>

Re: [PATCH] Fix PR gdb/17820

[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

Re: [PATCH] Fix PR gdb/17820

[Patrick Palka]

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
>

Re: [PATCH] Fix PR gdb/17820

[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

[PATCH] Test the setting of "history size" via $HOME/.gdbinit

[Patrick Palka]

Hi Pedro,

How does this look?

(When it is OK, I will squash this commit with the main commit before
pushing it.)

gdb/testsuite/ChangeLog:

	PR gdb/17820
	* gdb.base/gdbinit-history.exp: New test.
	* gdb.base/gdbinit-history/unlimited/.gdbinit: New file.
	* gdb.base/gdbinit-history/zero/.gdbinit: New file.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp         | 41 ++++++++++++++++++++++
 .../gdb.base/gdbinit-history/unlimited/.gdbinit    |  1 +
 .../gdb.base/gdbinit-history/zero/.gdbinit         |  1 +
 3 files changed, 43 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history.exp
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit

diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
new file mode 100644
index 0000000..194f2df
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -0,0 +1,41 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test the setting of "history size" via $HOME/.gdbinit
+
+proc test_gdbinit_history_setting { home size } {
+    global env
+    global INTERNAL_GDBFLAGS
+    global srcdir
+    global subdir
+
+    set old_home $env(HOME)
+    set env(HOME) "$srcdir/$subdir/$home"
+    set saved_internal_gdbflags $INTERNAL_GDBFLAGS
+    set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
+
+    gdb_exit
+    gdb_start
+
+    gdb_test "show history size" "The size of the command history is $size."
+
+    set INTERNAL_GDBFLAGS $saved_internal_gdbflags
+    set $env(HOME) $old_home
+}
+
+test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited"
+test_gdbinit_history_setting "gdbinit-history/zero" "0"
diff --git a/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit b/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
new file mode 100644
index 0000000..6604d8f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
@@ -0,0 +1 @@
+set history size unlimited
diff --git a/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit b/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit
new file mode 100644
index 0000000..7cd6b24
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit
@@ -0,0 +1 @@
+set history size 0
-- 
2.4.0.194.gc518059

Re: [PATCH] Test the setting of "history size" via $HOME/.gdbinit

[Pedro Alves]

On 05/12/2015 02:07 PM, Patrick Palka wrote:
> Hi Pedro,
> 
> How does this look?
> 
> (When it is OK, I will squash this commit with the main commit before
> pushing it.)
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/17820
> 	* gdb.base/gdbinit-history.exp: New test.
> 	* gdb.base/gdbinit-history/unlimited/.gdbinit: New file.
> 	* gdb.base/gdbinit-history/zero/.gdbinit: New file.
> ---
>  gdb/testsuite/gdb.base/gdbinit-history.exp         | 41 ++++++++++++++++++++++
>  .../gdb.base/gdbinit-history/unlimited/.gdbinit    |  1 +
>  .../gdb.base/gdbinit-history/zero/.gdbinit         |  1 +
>  3 files changed, 43 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/gdbinit-history.exp
>  create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
>  create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit
> 
> diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
> new file mode 100644
> index 0000000..194f2df
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
> @@ -0,0 +1,41 @@
> +# Copyright 2015 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.
> +
> +# Test the setting of "history size" via $HOME/.gdbinit
> +
> +proc test_gdbinit_history_setting { home size } {
> +    global env
> +    global INTERNAL_GDBFLAGS
> +    global srcdir
> +    global subdir
> +
> +    set old_home $env(HOME)
> +    set env(HOME) "$srcdir/$subdir/$home"
> +    set saved_internal_gdbflags $INTERNAL_GDBFLAGS
> +    set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
> +
> +    gdb_exit
> +    gdb_start
> +
> +    gdb_test "show history size" "The size of the command history is $size."
> +
> +    set INTERNAL_GDBFLAGS $saved_internal_gdbflags
> +    set $env(HOME) $old_home
> +}
> +
> +test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited"
> +test_gdbinit_history_setting "gdbinit-history/zero" "0"

Great.

I think that doesn't really cover the bug yet fully though, because
with an unfixed gdb, "show history unlimited" shows "unlimited",
but the readline history still ends up disabled.  So I think we
should try a "show commands" after the "show history size".  For the
unlimited case, we could test that the history contains a few
commands and that the last couple were what we just issued.
And for size==0, make sure that history really is empty.

Something like:

gdb_test "show history size" "The size of the command history is $size."
if {$home == "unlimited"} {
   gdb_test "show comands" " show history size\r\n[ \t]+show commands"
} else {
   gdb_test_no_output "show commands"
}

(FYI: to run a command without having it enter the command history,
prefix it with "server".  That may be useful for testing.)

Thanks,
Pedro Alves

[PATCH] Test the setting of "history size" via $HOME/.gdbinit

[Patrick Palka]

Heh, good point...  Here is an updated patch that tests "show commands" too.

gdb/testsuite/ChangeLog:

	PR gdb/17820
	* gdb.base/gdbinit-history.exp: New test.
	* gdb.base/gdbinit-history/unlimited/.gdbinit: New file.
	* gdb.base/gdbinit-history/zero/.gdbinit: New file.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp         | 47 ++++++++++++++++++++++
 .../gdb.base/gdbinit-history/unlimited/.gdbinit    |  1 +
 .../gdb.base/gdbinit-history/zero/.gdbinit         |  1 +
 3 files changed, 49 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history.exp
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
 create mode 100644 gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit

diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
new file mode 100644
index 0000000..6caeb4b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -0,0 +1,47 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test the setting of "history size" via $HOME/.gdbinit
+
+proc test_gdbinit_history_setting { home size } {
+    global env
+    global INTERNAL_GDBFLAGS
+    global srcdir
+    global subdir
+
+    set old_home $env(HOME)
+    set env(HOME) "$srcdir/$subdir/$home"
+    set saved_internal_gdbflags $INTERNAL_GDBFLAGS
+    set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
+
+    gdb_exit
+    gdb_start
+
+    gdb_test "show history size" "The size of the command history is $size."
+
+    if { $size == "0" } {
+        gdb_test_no_output "show commands"
+    } elseif { $size != "1" } {
+        gdb_test "show commands" "    .  show history size\r\n    .  show commands"
+    }
+
+    set INTERNAL_GDBFLAGS $saved_internal_gdbflags
+    set $env(HOME) $old_home
+}
+
+test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited"
+test_gdbinit_history_setting "gdbinit-history/zero" "0"
diff --git a/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit b/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
new file mode 100644
index 0000000..6604d8f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history/unlimited/.gdbinit
@@ -0,0 +1 @@
+set history size unlimited
diff --git a/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit b/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit
new file mode 100644
index 0000000..7cd6b24
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history/zero/.gdbinit
@@ -0,0 +1 @@
+set history size 0
-- 
2.4.0.194.gc518059

Re: [PATCH] Test the setting of "history size" via $HOME/.gdbinit

[Pedro Alves]

On 05/13/2015 02:09 AM, Patrick Palka wrote:
> Heh, good point...  Here is an updated patch that tests "show commands" too.

Looks good.  Thanks again for working on this.

Thanks,
Pedro Alves