[PATCH] [COMMITTED] Fix PR gdb/17820

[PATCH] [COMMITTED] 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.

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/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/ChangeLog                                      | 14 ++++++
 gdb/testsuite/ChangeLog                            |  7 +++
 gdb/testsuite/gdb.base/gdbinit-history.exp         | 51 +++++++++++++++++++++
 .../gdb.base/gdbinit-history/unlimited/.gdbinit    |  1 +
 .../gdb.base/gdbinit-history/zero/.gdbinit         |  1 +
 gdb/top.c                                          | 53 +++++++++-------------
 6 files changed, 96 insertions(+), 31 deletions(-)
 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/ChangeLog b/gdb/ChangeLog
index 61bc846..082886e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2015-05-13  Patrick Palka  <patrick@parcs.ath.cx>
+
+	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.
+
 2015-05-12  Doug Evans  <dje@google.com>
 
 	* dwarf2read.c (struct file_entry): Tweak comments.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 52140bd..db8f3ce 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2015-05-13  Patrick Palka  <patrick@parcs.ath.cx>
+
+	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.
+
 2015-05-09  Siva Chandra Reddy  <sivachandra@google.com>
 
 	* gdb.python/py-xmethods.cc: Enhance test case.
diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
new file mode 100644
index 0000000..474680a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -0,0 +1,51 @@
+# 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
+
+
+# Check that the history size is properly set to SIZE when reading the .gdbinit
+# file located in HOME.
+
+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
diff --git a/gdb/top.c b/gdb/top.c
index deb4108..74e1e07 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -687,8 +687,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,
@@ -1610,34 +1610,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
@@ -1705,12 +1697,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)
@@ -1859,7 +1849,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.194.gc518059

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Pedro Alves]

Hi Patrick,

I noticed that the buildbots are showing that this new test is failing:

 https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html

~~~
============================
new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
new FAIL: gdb.base/gdbinit-history.exp: show history size
new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
~~~

Logs at:

 http://gdb-build.sergiodj.net/cgit/Fedora-x86_64-m64/.git/tree/?h=master&id=aa0e3e6cdb70d805ec9a3c689861d936ab9c5c90

 (follow the "plain" link on the right right)

Then I noticed that the test is also failing on my
machine (F20).  :-)  I went ahead and wrote a fix.  What do
you think?

-------
From d75f038583bfa6265253e389c1012dc29eabd208 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 May 2015 16:47:23 +0100
Subject: [PATCH] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the
 environment

Some buildslaves are showing that this test is failing.  E.g.,:

 https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html

The issue is that HISTSIZE is set to 1000 in the environment that runs
the tests (that's the default in Fedora, set in /etc/profile).

We can trivially reproduce it with:

 $ HISTSIZE=1000 make check RUNTESTFLAGS="gdbinit-history.exp"
 (...)
 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gdbinit-history.exp ...
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show commands

gdb.log shows:
 ...
 (gdb) set height 0
 (gdb) set width 0
 (gdb) show history size
 The size of the command history is 1000.
 (gdb) FAIL: gdb.base/gdbinit-history.exp: show history size

gdb/testsuite/ChangeLog:
2015-05-15  Pedro Alves  <palves@redhat.com>

	* gdb.base/gdbinit-history.exp (test_gdbinit_history_setting):
	Unset HISTSIZE in the environment while testing and restore
	afterwards.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
index 474680a..7513934 100644
--- a/gdb/testsuite/gdb.base/gdbinit-history.exp
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -29,6 +29,17 @@ proc test_gdbinit_history_setting { home size } {
 
     set old_home $env(HOME)
     set env(HOME) "$srcdir/$subdir/$home"
+
+    # The HISTSIZE environment variable takes precedence over whatever
+    # history size is set in .gdbinit.  Make sure the former is not
+    # set.
+    set have_old_histsize 0
+    if [info exists env(HISTSIZE)] {
+	set old_histsize $env(HISTSIZE)
+	unset env(HISTSIZE)
+	set have_old_histsize 1
+    }
+
     set saved_internal_gdbflags $INTERNAL_GDBFLAGS
     set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
 
@@ -44,6 +55,11 @@ proc test_gdbinit_history_setting { home size } {
     }
 
     set INTERNAL_GDBFLAGS $saved_internal_gdbflags
+
+    if {$have_old_histsize} {
+	set env(HISTSIZE) $old_histsize
+    }
+
     set $env(HOME) $old_home
 }
 
-- 
1.9.3

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Patrick Palka]

On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> I noticed that the buildbots are showing that this new test is failing:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> ~~~
> ============================
> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
> new FAIL: gdb.base/gdbinit-history.exp: show history size
> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
> ~~~
>
> Logs at:
>
>  http://gdb-build.sergiodj.net/cgit/Fedora-x86_64-m64/.git/tree/?h=master&id=aa0e3e6cdb70d805ec9a3c689861d936ab9c5c90
>
>  (follow the "plain" link on the right right)
>
> Then I noticed that the test is also failing on my
> machine (F20).  :-)  I went ahead and wrote a fix.  What do
> you think?

Interesting.  I obviously did not encounter this failure even though I
too have HISTSIZE set (in my local rc file however).

>
> -------
> From d75f038583bfa6265253e389c1012dc29eabd208 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 May 2015 16:47:23 +0100
> Subject: [PATCH] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the
>  environment
>
> Some buildslaves are showing that this test is failing.  E.g.,:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> The issue is that HISTSIZE is set to 1000 in the environment that runs
> the tests (that's the default in Fedora, set in /etc/profile).
>
> We can trivially reproduce it with:
>
>  $ HISTSIZE=1000 make check RUNTESTFLAGS="gdbinit-history.exp"
>  (...)
>  Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gdbinit-history.exp ...
>  FAIL: gdb.base/gdbinit-history.exp: show history size
>  FAIL: gdb.base/gdbinit-history.exp: show history size
>  FAIL: gdb.base/gdbinit-history.exp: show commands
>
> gdb.log shows:
>  ...
>  (gdb) set height 0
>  (gdb) set width 0
>  (gdb) show history size
>  The size of the command history is 1000.
>  (gdb) FAIL: gdb.base/gdbinit-history.exp: show history size
>
> gdb/testsuite/ChangeLog:
> 2015-05-15  Pedro Alves  <palves@redhat.com>
>
>         * gdb.base/gdbinit-history.exp (test_gdbinit_history_setting):
>         Unset HISTSIZE in the environment while testing and restore
>         afterwards.
> ---
>  gdb/testsuite/gdb.base/gdbinit-history.exp | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
> index 474680a..7513934 100644
> --- a/gdb/testsuite/gdb.base/gdbinit-history.exp
> +++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
> @@ -29,6 +29,17 @@ proc test_gdbinit_history_setting { home size } {
>
>      set old_home $env(HOME)
>      set env(HOME) "$srcdir/$subdir/$home"
> +
> +    # The HISTSIZE environment variable takes precedence over whatever
> +    # history size is set in .gdbinit.  Make sure the former is not
> +    # set.
> +    set have_old_histsize 0
> +    if [info exists env(HISTSIZE)] {
> +       set old_histsize $env(HISTSIZE)
> +       unset env(HISTSIZE)
> +       set have_old_histsize 1
> +    }
> +
>      set saved_internal_gdbflags $INTERNAL_GDBFLAGS
>      set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
>
> @@ -44,6 +55,11 @@ proc test_gdbinit_history_setting { home size } {
>      }
>
>      set INTERNAL_GDBFLAGS $saved_internal_gdbflags
> +
> +    if {$have_old_histsize} {
> +       set env(HISTSIZE) $old_histsize
> +    }

Why not change this predicate to

     if [info exists old_histsize]

to obviate the need for $have_old_histsize altogether?

> +
>      set $env(HOME) $old_home
>  }
>
> --
> 1.9.3
>
>

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Patrick Palka]

On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> I noticed that the buildbots are showing that this new test is failing:
>
>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>
> ~~~
> ============================
> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
> new FAIL: gdb.base/gdbinit-history.exp: show history size
> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>

Also the tests in this file have duplicate names.. That's undesirable
right?  If so I could make the names unique.  Would such a change fall
under the "obvious" rule?

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Pedro Alves]

On 05/15/2015 06:09 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>> Hi Patrick,
>>
>> I noticed that the buildbots are showing that this new test is failing:
>>
>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>
>> ~~~
>> ============================
>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
> 
> Also the tests in this file have duplicate names.. That's undesirable
> right?  If so I could make the names unique.  

Yes.  I should have spotted that earlier.

> Would such a change fall under the "obvious" rule?

Not sure, depends on how you would fix it. :-)  Apply the test
described in MAINTAINERS.  :-)

There are a couple ways to address that.  In cases like
this test, where we have a function that called multiple
times, the modern way is to use with_test_prefix to wrap the
function call or the function body, which then also covers
FAILs issued from within gdb_start, etc.

Thanks,
Pedro Alves

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Patrick Palka]

On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>> Hi Patrick,
>>>
>>> I noticed that the buildbots are showing that this new test is failing:
>>>
>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>
>>> ~~~
>>> ============================
>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>
>> Also the tests in this file have duplicate names.. That's undesirable
>> right?  If so I could make the names unique.
>
> Yes.  I should have spotted that earlier.
>
>> Would such a change fall under the "obvious" rule?
>
> Not sure, depends on how you would fix it. :-)  Apply the test
> described in MAINTAINERS.  :-)

So it will probably not be obvious because of naming preferences.

>
> There are a couple ways to address that.  In cases like
> this test, where we have a function that called multiple
> times, the modern way is to use with_test_prefix to wrap the
> function call or the function body, which then also covers
> FAILs issued from within gdb_start, etc.

Cool.. I will do something like that.

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Pedro Alves]

On 05/15/2015 06:26 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>> Hi Patrick,
>>>>
>>>> I noticed that the buildbots are showing that this new test is failing:
>>>>
>>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>>
>>>> ~~~
>>>> ============================
>>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>>
>>> Also the tests in this file have duplicate names.. That's undesirable
>>> right?  If so I could make the names unique.
>>
>> Yes.  I should have spotted that earlier.
>>
>>> Would such a change fall under the "obvious" rule?
>>
>> Not sure, depends on how you would fix it. :-)  Apply the test
>> described in MAINTAINERS.  :-)
> 
> So it will probably not be obvious because of naming preferences.

Sorry for the trigger-happy pun.  I didn't mean to sound
rude or put you off.  I certainly do not hate your work.  :-)

I was just thinking that someone not familiar with the
testsuite's history might consider obvious to change the test
names one by one, while we avoid that nowadays in some cases
(like described below).

> 
>>
>> There are a couple ways to address that.  In cases like
>> this test, where we have a function that called multiple
>> times, the modern way is to use with_test_prefix to wrap the
>> function call or the function body, which then also covers
>> FAILs issued from within gdb_start, etc.
> 
> Cool.. I will do something like that.

Excellent, thanks.

-- 
Pedro Alves

[PATCH v2] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the environment (Re: [PATCH] [COMMITTED] Fix PR gdb/17820)

[Pedro Alves]

On 05/15/2015 06:03 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>> +    if {$have_old_histsize} {
>> +       set env(HISTSIZE) $old_histsize
>> +    }
> 
> Why not change this predicate to
> 
>      if [info exists old_histsize]
> 
> to obviate the need for $have_old_histsize altogether?

Yeah, "info exists" is ok since this is local scope; I was worrying
that the code ends up copied elsewhere to global context, and
then the "info exists" would be the wrong thing to use, considering
e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
simply saving/restoring the whole env array, like in the updated
patch below, which sidesteps that issue?

[1] https://sourceware.org/ml/gdb-patches/2015-04/msg00261.html
[2] https://sourceware.org/ml/gdb-patches/2015-04/msg00537.html

--------
From b5c09f8bdd4f780d176d110ce61f4eb153ecc9ef Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 May 2015 19:27:07 +0100
Subject: [PATCH] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the
 environment

Some buildslaves are showing that this test is failing.  E.g.,:

 https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html

The issue is that HISTSIZE is set to 1000 in the environment that runs
the tests (that's the default in Fedora, set in /etc/profile).

We can trivially reproduce it with:

 $ HISTSIZE=1000 make check RUNTESTFLAGS="gdbinit-history.exp"
 (...)
 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gdbinit-history.exp ...
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show commands

gdb.log shows:
 ...
 (gdb) set height 0
 (gdb) set width 0
 (gdb) show history size
 The size of the command history is 1000.
 (gdb) FAIL: gdb.base/gdbinit-history.exp: show history size

gdb/testsuite/ChangeLog:
2015-05-15  Pedro Alves  <palves@redhat.com>

	* gdb.base/gdbinit-history.exp (test_gdbinit_history_setting):
	Save the whole env array instead of just HOME.  Unset HISTSIZE in
	the environment while testing.  Restore whole environment
	afterwards.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
index 474680a..aba15b4 100644
--- a/gdb/testsuite/gdb.base/gdbinit-history.exp
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -27,8 +27,15 @@ proc test_gdbinit_history_setting { home size } {
     global srcdir
     global subdir
 
-    set old_home $env(HOME)
+    array set old_env [array get env]
+
     set env(HOME) "$srcdir/$subdir/$home"
+
+    # The HISTSIZE environment variable takes precedence over whatever
+    # history size is set in .gdbinit.  Make sure the former is not
+    # set.
+    unset -nocomplain env(HISTSIZE)
+
     set saved_internal_gdbflags $INTERNAL_GDBFLAGS
     set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
 
@@ -44,7 +51,8 @@ proc test_gdbinit_history_setting { home size } {
     }
 
     set INTERNAL_GDBFLAGS $saved_internal_gdbflags
-    set $env(HOME) $old_home
+
+    array set env [array get old_env]
 }
 
 test_gdbinit_history_setting "gdbinit-history/unlimited" "unlimited"
-- 
1.9.3

Re: [PATCH] [COMMITTED] Fix PR gdb/17820

[Patrick Palka]

On Fri, May 15, 2015 at 1:48 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:26 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 1:17 PM, Pedro Alves <palves@redhat.com> wrote:
>>> On 05/15/2015 06:09 PM, Patrick Palka wrote:
>>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>>> Hi Patrick,
>>>>>
>>>>> I noticed that the buildbots are showing that this new test is failing:
>>>>>
>>>>>  https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html
>>>>>
>>>>> ~~~
>>>>> ============================
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show commands <<2>>
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size
>>>>> new FAIL: gdb.base/gdbinit-history.exp: show history size <<2>>
>>>>
>>>> Also the tests in this file have duplicate names.. That's undesirable
>>>> right?  If so I could make the names unique.
>>>
>>> Yes.  I should have spotted that earlier.
>>>
>>>> Would such a change fall under the "obvious" rule?
>>>
>>> Not sure, depends on how you would fix it. :-)  Apply the test
>>> described in MAINTAINERS.  :-)
>>
>> So it will probably not be obvious because of naming preferences.
>
> Sorry for the trigger-happy pun.  I didn't mean to sound
> rude or put you off.  I certainly do not hate your work.  :-)

No problem, I had not inferred any rude intentions.

>
> I was just thinking that someone not familiar with the
> testsuite's history might consider obvious to change the test
> names one by one, while we avoid that nowadays in some cases
> (like described below).

Yeah, I was planning on exactly that!

>
>>
>>>
>>> There are a couple ways to address that.  In cases like
>>> this test, where we have a function that called multiple
>>> times, the modern way is to use with_test_prefix to wrap the
>>> function call or the function body, which then also covers
>>> FAILs issued from within gdb_start, etc.
>>
>> Cool.. I will do something like that.
>
> Excellent, thanks.
>
> --
> Pedro Alves
>

Re: [PATCH v2] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the environment (Re: [PATCH] [COMMITTED] Fix PR gdb/17820)

[Patrick Palka]

On Fri, May 15, 2015 at 2:33 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:03 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>> +    if {$have_old_histsize} {
>>> +       set env(HISTSIZE) $old_histsize
>>> +    }
>>
>> Why not change this predicate to
>>
>>      if [info exists old_histsize]
>>
>> to obviate the need for $have_old_histsize altogether?
>
> Yeah, "info exists" is ok since this is local scope; I was worrying
> that the code ends up copied elsewhere to global context, and
> then the "info exists" would be the wrong thing to use, considering
> e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
> simply saving/restoring the whole env array, like in the updated
> patch below, which sidesteps that issue?

What a neat language (TCL).

Both the original approach (with $have_old_histsize) and this one look
fine by me.

Re: [PATCH v2] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the environment (Re: [PATCH] [COMMITTED] Fix PR gdb/17820)

[Pedro Alves]

On 05/15/2015 10:42 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 2:33 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/15/2015 06:03 PM, Patrick Palka wrote:
>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>> +    if {$have_old_histsize} {
>>>> +       set env(HISTSIZE) $old_histsize
>>>> +    }
>>>
>>> Why not change this predicate to
>>>
>>>      if [info exists old_histsize]
>>>
>>> to obviate the need for $have_old_histsize altogether?
>>
>> Yeah, "info exists" is ok since this is local scope; I was worrying
>> that the code ends up copied elsewhere to global context, and
>> then the "info exists" would be the wrong thing to use, considering
>> e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
>> simply saving/restoring the whole env array, like in the updated
>> patch below, which sidesteps that issue?
> 
> What a neat language (TCL).

:-)

(It's more an issue with DejaGnu sourcing all test files into
the same context, than language.)

> 
> Both the original approach (with $have_old_histsize) and this one look
> fine by me.

I've pushed this one in now.

Thanks,
Pedro Alves