public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
@ 2015-05-22 11:24 Patrick Palka
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Patrick Palka @ 2015-05-22 11:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: Patrick Palka

The HISTSIZE environment variable is generally expected to be read by
shells, not by applications.  Some distros for example globally export
HISTSIZE in /etc/profile -- with the intention that it only affects
shells -- and by doing so it renders useless GDB's own mechanism for
setting the history size via .gdbinit.  Also, annoyances may arise when
HISTSIZE is not interpreted the same way by the shell and by GDB, e.g.
PR gdb/16999.  That can always be fixed on a shell-by-shell basis but it
may be impossible to be consistent with the behavior of all shells at
once.  Finally it just makes sense to not confound shell environment
variables with application environment variables.

gdb/ChangeLog:

	* NEWS: Add entry.
	* top.c (init_history): Read from GDBHISTSIZE instead of
	HISTSIZE.
	(init_main): Refer to GDBHISTSIZE instead of HISTSIZE.

gdb/doc/ChangeLog:

	* gdb.texinfo (Command History): Replace occurrences of HISTSIZE
	with GDBHISTSIZE.

gdb/testsuite/ChangeLog:

	* gdb.base/gdbinit-history.exp: Replace occurrences of HISTSIZE
	with GDBHISTSIZE.
	* gdb.base/readline.exp: Likewis.
---
 gdb/NEWS                                   |  4 ++++
 gdb/doc/gdb.texinfo                        |  4 ++--
 gdb/testsuite/gdb.base/gdbinit-history.exp |  4 ++--
 gdb/testsuite/gdb.base/readline.exp        | 12 ++++++------
 gdb/top.c                                  |  4 ++--
 5 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 745444b..1be239f 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -48,6 +48,10 @@
 
 * GDB now supports the vector ABI on S/390 GNU/Linux targets.
 
+* The HISTSIZE environment variable is no longer read when determining
+  the size of GDB's command history.  GDB now instead reads the dedicated
+  GDBHISTSIZE environment variable.
+
 * Guile Scripting
 
   ** Memory ports can now be unbuffered.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e38fd31..c21a312 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22599,12 +22599,12 @@ Stop recording command history in a file.
 
 @cindex history size
 @kindex set history size
-@cindex @env{HISTSIZE}, environment variable
+@cindex @env{GDBHISTSIZE}, environment variable
 @item set history size @var{size}
 @itemx set history size unlimited
 Set the number of commands which @value{GDBN} keeps in its history list.
 This defaults to the value of the environment variable
-@code{HISTSIZE}, or to 256 if this variable is not set.  If @var{size}
+@code{GDBHISTSIZE}, or to 256 if this variable is not set.  If @var{size}
 is @code{unlimited}, the number of commands @value{GDBN} keeps in the
 history list is unlimited.
 @end table
diff --git a/gdb/testsuite/gdb.base/gdbinit-history.exp b/gdb/testsuite/gdb.base/gdbinit-history.exp
index 8adfd68..cbd965a 100644
--- a/gdb/testsuite/gdb.base/gdbinit-history.exp
+++ b/gdb/testsuite/gdb.base/gdbinit-history.exp
@@ -31,10 +31,10 @@ proc test_gdbinit_history_setting { home size } {
 
     set env(HOME) "$srcdir/$subdir/$home"
 
-    # The HISTSIZE environment variable takes precedence over whatever
+    # The GDBHISTSIZE environment variable takes precedence over whatever
     # history size is set in .gdbinit.  Make sure the former is not
     # set.
-    unset -nocomplain env(HISTSIZE)
+    unset -nocomplain env(GDBHISTSIZE)
 
     set saved_internal_gdbflags $INTERNAL_GDBFLAGS
     set INTERNAL_GDBFLAGS [string map {"-nx" ""} $INTERNAL_GDBFLAGS]
diff --git a/gdb/testsuite/gdb.base/readline.exp b/gdb/testsuite/gdb.base/readline.exp
index c444285..f0490a2 100644
--- a/gdb/testsuite/gdb.base/readline.exp
+++ b/gdb/testsuite/gdb.base/readline.exp
@@ -185,11 +185,11 @@ gdb_test_multiple "if 1 > 0\n\033\[A\033\[A\nend" $msg {
 if [info exists env(GDBHISTFILE)] {
     set old_gdbhistfile $env(GDBHISTFILE)
 }
-if [info exists env(HISTSIZE)] {
-    set old_histsize $env(HISTSIZE)
+if [info exists env(GDBHISTSIZE)] {
+    set old_gdbhistsize $env(GDBHISTSIZE)
 }
 set env(GDBHISTFILE) "${srcdir}/${subdir}/gdb_history"
-set env(HISTSIZE) "10"
+set env(GDBHISTSIZE) "10"
 
 gdb_exit
 gdb_start
@@ -207,10 +207,10 @@ if [info exists old_gdbhistfile] {
 } else {
     unset env(GDBHISTFILE)
 }
-if [info exists old_histsize] {
-    set env(HISTSIZE) $old_histsize
+if [info exists old_gdbhistsize] {
+    set env(GDBHISTSIZE) $old_gdbhistsize
 } else {
-    unset env(HISTSIZE)
+    unset env(GDBHISTSIZE)
 }
 set timeout $oldtimeout1
 
diff --git a/gdb/top.c b/gdb/top.c
index 74e1e07..6f0421d 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1680,7 +1680,7 @@ init_history (void)
 {
   char *tmpenv;
 
-  tmpenv = getenv ("HISTSIZE");
+  tmpenv = getenv ("GDBHISTSIZE");
   if (tmpenv)
     {
       int var;
@@ -1856,7 +1856,7 @@ Show the size of the command history,"), _("\
 ie. the number of previous commands to keep a record of.\n\
 If set to \"unlimited\", the number of commands kept in the history\n\
 list is unlimited.  This defaults to the value of the environment\n\
-variable \"HISTSIZE\", or to 256 if this variable is not set."),
+variable \"GDBHISTSIZE\", or to 256 if this variable is not set."),
 			    set_history_size_command,
 			    show_history_size,
 			    &sethistlist, &showhistlist);
-- 
2.4.1.217.g6c1249c

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-05-22 11:24 [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Patrick Palka
@ 2015-05-22 11:24 ` Patrick Palka
  2015-05-22 11:57   ` Patrick Palka
                     ` (2 more replies)
  2015-05-22 12:30 ` [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Eli Zaretskii
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 12+ messages in thread
From: Patrick Palka @ 2015-05-22 11:24 UTC (permalink / raw)
  To: gdb-patches; +Cc: Patrick Palka

When GDB reads a nonsensical value for the GDBHISTSIZE environment
variable, i.e. one that is non-numeric or negative, GDB then sets its
history size to 0.  This behavior is annoying and also inconsistent
with the behavior of bash.

This patch makes the behavior of invalid GDBHISTSIZE consistent with how
bash handles HISTSIZE.  When we encounter a null or out-of-range
GDBHISTSIZE (outside of [0, INT_MAX]) we now set the history size to
unlimited instead of 0.  When we encounter a non-numeric GDBHISTSIZE we
do nothing.

gdb/ChangeLog:

	PR gdb/16999
	* top.c (init_history): For null or out-of-range GDBHISTSIZE,
	set history size to unlimited.  Ignore non-numeric GDBHISTSIZE.

gdb/testsuite/ChangeLog:

	PR gdb/16999
	* gdb.base/gdbhistsize-history.exp: New test.
---
 gdb/NEWS                                       |  4 +-
 gdb/doc/gdb.texinfo                            |  9 ++--
 gdb/testsuite/gdb.base/gdbhistsize-history.exp | 69 ++++++++++++++++++++++++++
 gdb/top.c                                      | 42 ++++++++++------
 4 files changed, 105 insertions(+), 19 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/gdbhistsize-history.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 1be239f..f3cee13 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -50,7 +50,9 @@
 
 * The HISTSIZE environment variable is no longer read when determining
   the size of GDB's command history.  GDB now instead reads the dedicated
-  GDBHISTSIZE environment variable.
+  GDBHISTSIZE environment variable.  Setting GDBHISTSIZE to "-1" or to "" now
+  disables truncation of command history.  Non-numeric values of GDBHISTSIZE
+  are ignored.
 
 * Guile Scripting
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c21a312..b0bd194 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22603,10 +22603,11 @@ Stop recording command history in a file.
 @item set history size @var{size}
 @itemx set history size unlimited
 Set the number of commands which @value{GDBN} keeps in its history list.
-This defaults to the value of the environment variable
-@code{GDBHISTSIZE}, or to 256 if this variable is not set.  If @var{size}
-is @code{unlimited}, the number of commands @value{GDBN} keeps in the
-history list is unlimited.
+This defaults to the value of the environment variable @code{GDBHISTSIZE}, or
+to 256 if this variable is not set.  Non-numeric values of @code{GDBHISTSIZE}
+are ignored.  If @var{size} is @code{unlimited} or if @code{GDBHISTSIZE} is a
+negative number, the number of commands @value{GDBN} keeps in the history list
+is unlimited.
 @end table
 
 History expansion assigns special meaning to the character @kbd{!}.
diff --git a/gdb/testsuite/gdb.base/gdbhistsize-history.exp b/gdb/testsuite/gdb.base/gdbhistsize-history.exp
new file mode 100644
index 0000000..a7b70d1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdbhistsize-history.exp
@@ -0,0 +1,69 @@
+# 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 the GDBHISTSIZE environment variable
+
+
+# Check that the history size is properly set to SIZE when the environment
+# variable ENV_VAR is set to GDBHISTSIZE.
+
+proc test_histsize_history_setting { histsize size { env_var "GDBHISTSIZE" } } {
+    global env
+
+    set have_old_gdbhistsize 0
+    if [info exists env($env_var)] {
+        set have_old_gdbhistsize 1
+        set old_gdbhistsize $env($env_var)
+    }
+    set env($env_var) $histsize
+
+    with_test_prefix "histsize=$histsize" {
+	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"
+	}
+
+	if { $have_old_gdbhistsize } {
+	    set env($env_var) $old_gdbhistsize
+	} else {
+	    unset env($env_var)
+	}
+    }
+}
+
+test_histsize_history_setting "" "unlimited"
+test_histsize_history_setting "0" "0"
+test_histsize_history_setting "20" "20"
+test_histsize_history_setting " 20 " "20"
+test_histsize_history_setting "-5" "unlimited"
+
+# Test defaulting to 256 upon encountering a non-numeric GDBHISTSIZE.
+test_histsize_history_setting "not_an_integer" "256"
+test_histsize_history_setting "10zab" "256"
+
+# A huge number (hopefully larger than INT_MAX)
+test_histsize_history_setting "99999999999999999999999999999999999" "unlimited"
+
+# We no longer read HISTSIZE
+test_histsize_history_setting "50" "256" "HISTSIZE"
diff --git a/gdb/top.c b/gdb/top.c
index 6f0421d..efcb8d0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1683,21 +1683,35 @@ init_history (void)
   tmpenv = getenv ("GDBHISTSIZE");
   if (tmpenv)
     {
-      int var;
-
-      var = atoi (tmpenv);
-      if (var < 0)
-	{
-	  /* Prefer ending up with no history rather than overflowing
-	     readline's history interface, which uses signed 'int'
-	     everywhere.  */
-	  var = 0;
-	}
-
-      history_size_setshow_var = var;
+      long var;
+      char *endptr;
+
+      while (isspace (*tmpenv))
+	tmpenv++;
+
+      var = strtol (tmpenv, &endptr, 10);
+
+      while (isspace (*endptr))
+	endptr++;
+
+      /* If GDBHISTSIZE is non-numeric then ignore it.  If GDBHISTSIZE is the
+	 empty string, a negative number or a huge positive number (larger than
+	 INT_MAX) then set the history size to unlimited.  Otherwise set our
+	 history size to the number we have read.  This behavior is consistent
+	 with how bash handles HISTSIZE.  */
+      if (*endptr != '\0')
+	;
+      else if (*tmpenv == '\0'
+	       || var < 0
+	       || var > INT_MAX)
+	history_size_setshow_var = -1;
+      else
+	history_size_setshow_var = var;
     }
-  /* If the init file hasn't set a size yet, pick the default.  */
-  else if (history_size_setshow_var == -2)
+
+  /* If neither the init file nor GDBHISTSIZE has set a size yet, pick the
+     default.  */
+  if (history_size_setshow_var == -2)
     history_size_setshow_var = 256;
 
   set_readline_history_size (history_size_setshow_var);
-- 
2.4.1.217.g6c1249c

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
@ 2015-05-22 11:57   ` Patrick Palka
  2015-05-29  9:32   ` Pedro Alves
  2015-06-23 12:56   ` Yao Qi
  2 siblings, 0 replies; 12+ messages in thread
From: Patrick Palka @ 2015-05-22 11:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: Patrick Palka

On Fri, May 22, 2015 at 7:23 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> When GDB reads a nonsensical value for the GDBHISTSIZE environment
> variable, i.e. one that is non-numeric or negative, GDB then sets its
> history size to 0.  This behavior is annoying and also inconsistent
> with the behavior of bash.
>
> This patch makes the behavior of invalid GDBHISTSIZE consistent with how
> bash handles HISTSIZE.  When we encounter a null or out-of-range
> GDBHISTSIZE (outside of [0, INT_MAX]) we now set the history size to
> unlimited instead of 0.  When we encounter a non-numeric GDBHISTSIZE we
> do nothing.
>
> gdb/ChangeLog:
>
>         PR gdb/16999
>         * top.c (init_history): For null or out-of-range GDBHISTSIZE,
>         set history size to unlimited.  Ignore non-numeric GDBHISTSIZE.
>

Oops, this ChangeLog entry is not complete.  I will make sure to
complete it before pushing/re-posting.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
  2015-05-22 11:24 [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Patrick Palka
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
@ 2015-05-22 12:30 ` Eli Zaretskii
  2015-06-04 15:30   ` Patrick Palka
  2015-05-29 11:05 ` Pedro Alves
  2015-06-17 18:19 ` Patrick Palka
  3 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2015-05-22 12:30 UTC (permalink / raw)
  To: Patrick Palka; +Cc: gdb-patches, patrick

> From: Patrick Palka <patrick@parcs.ath.cx>
> Cc: Patrick Palka <patrick@parcs.ath.cx>
> Date: Fri, 22 May 2015 07:23:50 -0400
> 
> The HISTSIZE environment variable is generally expected to be read by
> shells, not by applications.  Some distros for example globally export
> HISTSIZE in /etc/profile -- with the intention that it only affects
> shells -- and by doing so it renders useless GDB's own mechanism for
> setting the history size via .gdbinit.  Also, annoyances may arise when
> HISTSIZE is not interpreted the same way by the shell and by GDB, e.g.
> PR gdb/16999.  That can always be fixed on a shell-by-shell basis but it
> may be impossible to be consistent with the behavior of all shells at
> once.  Finally it just makes sense to not confound shell environment
> variables with application environment variables.
> 
> gdb/ChangeLog:
> 
> 	* NEWS: Add entry.
> 	* top.c (init_history): Read from GDBHISTSIZE instead of
> 	HISTSIZE.
> 	(init_main): Refer to GDBHISTSIZE instead of HISTSIZE.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Command History): Replace occurrences of HISTSIZE
> 	with GDBHISTSIZE.

Thanks.

I think we should explain in the manual why we don't use HISTSIZE.
And I wonder why we cannot use HISTSIZE if neither GDBHISTSIZE nor
.gdbinit specify the size, but you probably already discussed that.

Otherwise, the documentation part is OK.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
  2015-05-22 11:57   ` Patrick Palka
@ 2015-05-29  9:32   ` Pedro Alves
  2015-06-17 19:18     ` Patrick Palka
  2015-06-23 12:56   ` Yao Qi
  2 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2015-05-29  9:32 UTC (permalink / raw)
  To: Patrick Palka, gdb-patches

On 05/22/2015 12:23 PM, Patrick Palka wrote:

> +      while (isspace (*tmpenv))
> +	tmpenv++;

...

> +
> +      while (isspace (*endptr))
> +	endptr++;

Use skip_spaces/skip_spaces_const for these.

Otherwise looks good to me.

(I suspect that testing the interaction between setting
the history both from a .gdbinit and $GDBHISTSIZE would
call for merging the gdbhistsize-history.exp / gdbinit-history.exp
to a single file.)

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
  2015-05-22 11:24 [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Patrick Palka
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
  2015-05-22 12:30 ` [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Eli Zaretskii
@ 2015-05-29 11:05 ` Pedro Alves
  2015-06-17 18:19 ` Patrick Palka
  3 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2015-05-29 11:05 UTC (permalink / raw)
  To: Patrick Palka, gdb-patches

On 05/22/2015 12:23 PM, Patrick Palka wrote:
> The HISTSIZE environment variable is generally expected to be read by
> shells, not by applications.  Some distros for example globally export
> HISTSIZE in /etc/profile -- with the intention that it only affects
> shells -- and by doing so it renders useless GDB's own mechanism for
> setting the history size via .gdbinit.  Also, annoyances may arise when
> HISTSIZE is not interpreted the same way by the shell and by GDB, e.g.
> PR gdb/16999.  That can always be fixed on a shell-by-shell basis but it
> may be impossible to be consistent with the behavior of all shells at
> once.  Finally it just makes sense to not confound shell environment
> variables with application environment variables.

This looks good to me.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
  2015-05-22 12:30 ` [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Eli Zaretskii
@ 2015-06-04 15:30   ` Patrick Palka
  2015-06-15 14:54     ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Patrick Palka @ 2015-06-04 15:30 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

On Fri, May 22, 2015 at 8:30 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Patrick Palka <patrick@parcs.ath.cx>
>> Cc: Patrick Palka <patrick@parcs.ath.cx>
>> Date: Fri, 22 May 2015 07:23:50 -0400
>>
>> The HISTSIZE environment variable is generally expected to be read by
>> shells, not by applications.  Some distros for example globally export
>> HISTSIZE in /etc/profile -- with the intention that it only affects
>> shells -- and by doing so it renders useless GDB's own mechanism for
>> setting the history size via .gdbinit.  Also, annoyances may arise when
>> HISTSIZE is not interpreted the same way by the shell and by GDB, e.g.
>> PR gdb/16999.  That can always be fixed on a shell-by-shell basis but it
>> may be impossible to be consistent with the behavior of all shells at
>> once.  Finally it just makes sense to not confound shell environment
>> variables with application environment variables.
>>
>> gdb/ChangeLog:
>>
>>       * NEWS: Add entry.
>>       * top.c (init_history): Read from GDBHISTSIZE instead of
>>       HISTSIZE.
>>       (init_main): Refer to GDBHISTSIZE instead of HISTSIZE.
>>
>> gdb/doc/ChangeLog:
>>
>>       * gdb.texinfo (Command History): Replace occurrences of HISTSIZE
>>       with GDBHISTSIZE.
>
> Thanks.
>
> I think we should explain in the manual why we don't use HISTSIZE.
> And I wonder why we cannot use HISTSIZE if neither GDBHISTSIZE nor
> .gdbinit specify the size, but you probably already discussed that.

Hmm, reading HISTSIZE as a last resort would reduce the inter-version
breakage a little, at least for users who today rely solely on
HISTSIZE (not ~/.gdbinit) to set their history size.  This can be
done, I guess.

>
> Otherwise, the documentation part is OK.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
  2015-06-04 15:30   ` Patrick Palka
@ 2015-06-15 14:54     ` Pedro Alves
  0 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2015-06-15 14:54 UTC (permalink / raw)
  To: Patrick Palka, Eli Zaretskii; +Cc: gdb-patches

On 06/04/2015 04:30 PM, Patrick Palka wrote:

> Hmm, reading HISTSIZE as a last resort would reduce the inter-version
> breakage a little, at least for users who today rely solely on
> HISTSIZE (not ~/.gdbinit) to set their history size.  This can be
> done, I guess.

I'd propose going ahead with the patch that ignores HISTSIZE and
seeing if anyone notices/complains.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE
  2015-05-22 11:24 [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Patrick Palka
                   ` (2 preceding siblings ...)
  2015-05-29 11:05 ` Pedro Alves
@ 2015-06-17 18:19 ` Patrick Palka
  3 siblings, 0 replies; 12+ messages in thread
From: Patrick Palka @ 2015-06-17 18:19 UTC (permalink / raw)
  To: gdb-patches; +Cc: Patrick Palka

On Fri, May 22, 2015 at 7:23 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> The HISTSIZE environment variable is generally expected to be read by
> shells, not by applications.  Some distros for example globally export
> HISTSIZE in /etc/profile -- with the intention that it only affects
> shells -- and by doing so it renders useless GDB's own mechanism for
> setting the history size via .gdbinit.  Also, annoyances may arise when
> HISTSIZE is not interpreted the same way by the shell and by GDB, e.g.
> PR gdb/16999.  That can always be fixed on a shell-by-shell basis but it
> may be impossible to be consistent with the behavior of all shells at
> once.  Finally it just makes sense to not confound shell environment
> variables with application environment variables.
>
> gdb/ChangeLog:
>
>         * NEWS: Add entry.
>         * top.c (init_history): Read from GDBHISTSIZE instead of
>         HISTSIZE.
>         (init_main): Refer to GDBHISTSIZE instead of HISTSIZE.
>
> gdb/doc/ChangeLog:
>
>         * gdb.texinfo (Command History): Replace occurrences of HISTSIZE
>         with GDBHISTSIZE.
>
> gdb/testsuite/ChangeLog:
>
>         * gdb.base/gdbinit-history.exp: Replace occurrences of HISTSIZE
>         with GDBHISTSIZE.
>         * gdb.base/readline.exp: Likewis.

Committed this change (without the part that explains in the manual
why we don't use HISTSIZE).  Eli, what do you think about the
following addendum to the manual?

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9889b69..c1b11f4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22644,6 +22644,15 @@ to 256 if this variable is not set.
Non-numeric values of @env{GDBHISTSIZE}
 are ignored.  If @var{size} is @code{unlimited} or if @env{GDBHISTSIZE} is a
 negative number, the number of commands @value{GDBN} keeps in the history list
 is unlimited.
+
+Note: In @value{GDBN} version 7.8 and earlier, the @env{HISTSIZE} environment
+variable was read instead of the @env{GDBHISTSIZE} environment variable.
+However, reading @env{HISTSIZE} was problematic since this environment variable
+is usually set with the intention to be read only by the user's shell, and
+differences in interpretation of this environment variable may exist between
+@value{GDBN} and the prevailing shell.  So since @value{GDBN} version 7.9 the
+dedicated @env{GDBHISTSIZE} environment variable is instead read for the same
+purpose.
 @end table

 History expansion assigns special meaning to the character @kbd{!}.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-05-29  9:32   ` Pedro Alves
@ 2015-06-17 19:18     ` Patrick Palka
  0 siblings, 0 replies; 12+ messages in thread
From: Patrick Palka @ 2015-06-17 19:18 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Fri, May 29, 2015 at 5:32 AM, Pedro Alves <palves@redhat.com> wrote:
> On 05/22/2015 12:23 PM, Patrick Palka wrote:
>
>> +      while (isspace (*tmpenv))
>> +     tmpenv++;
>
> ...
>
>> +
>> +      while (isspace (*endptr))
>> +     endptr++;
>
> Use skip_spaces/skip_spaces_const for these.
>
> Otherwise looks good to me.
>
> (I suspect that testing the interaction between setting
> the history both from a .gdbinit and $GDBHISTSIZE would
> call for merging the gdbhistsize-history.exp / gdbinit-history.exp
> to a single file.)

Committed.

The test files would not necessarily have to be merged to test the
interaction between $GDBHISTSIZE and .gdbinit.  One may worry about
code duplication by not merging them, but I think there would be code
duplication either way unless you really refactor the tests.  I think
it would be easier to test the interaction inside gdbinit-history.exp
since adding environment variable manipulation to gdbinit-history.exp
is easier than adding .gdbinit file manipulation in
gdbhistsize-history.exp.  In fact, gdbinit-history.exp already has to
manipulate the environment to unset GDBHISTSIZE.  So adding an
interaction test would only require a few lines of code.  I'll do it.

>
> Thanks,
> Pedro Alves
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
  2015-05-22 11:57   ` Patrick Palka
  2015-05-29  9:32   ` Pedro Alves
@ 2015-06-23 12:56   ` Yao Qi
  2015-06-23 14:28     ` Patrick Palka
  2 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2015-06-23 12:56 UTC (permalink / raw)
  To: Patrick Palka; +Cc: gdb-patches

Patrick Palka <patrick@parcs.ath.cx> writes:

Hi Patrick,

> +# A huge number (hopefully larger than INT_MAX)
> +test_histsize_history_setting "99999999999999999999999999999999999"
> "unlimited"

This test fails on some targets (32-bit?) as shown in buildbot,
https://www.sourceware.org/ml/gdb-testers/2015-q2/msg06883.html

 (gdb) show history size
 The size of the command history is 2147483647.
 (gdb) FAIL: gdb.base/gdbhistsize-history.exp: histsize=99999999999999999999999999999999999: show history size

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999]
  2015-06-23 12:56   ` Yao Qi
@ 2015-06-23 14:28     ` Patrick Palka
  0 siblings, 0 replies; 12+ messages in thread
From: Patrick Palka @ 2015-06-23 14:28 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On Tue, Jun 23, 2015 at 8:56 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>
> Hi Patrick,
>
>> +# A huge number (hopefully larger than INT_MAX)
>> +test_histsize_history_setting "99999999999999999999999999999999999"
>> "unlimited"
>
> This test fails on some targets (32-bit?) as shown in buildbot,
> https://www.sourceware.org/ml/gdb-testers/2015-q2/msg06883.html
>
>  (gdb) show history size
>  The size of the command history is 2147483647.
>  (gdb) FAIL: gdb.base/gdbhistsize-history.exp: histsize=99999999999999999999999999999999999: show history size
>
> --
> Yao (齐尧)


Thanks for the heads up.  I failed to account for targets where
INT_MAX == LONG_MAX.  I have a patch to send.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-06-23 14:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-22 11:24 [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Patrick Palka
2015-05-22 11:24 ` [PATCH 2/2] Tweak the handling of $GDBHISTSIZE edge cases [PR gdb/16999] Patrick Palka
2015-05-22 11:57   ` Patrick Palka
2015-05-29  9:32   ` Pedro Alves
2015-06-17 19:18     ` Patrick Palka
2015-06-23 12:56   ` Yao Qi
2015-06-23 14:28     ` Patrick Palka
2015-05-22 12:30 ` [PATCH 1/2] Read $GDBHISTSIZE instead of $HISTSIZE Eli Zaretskii
2015-06-04 15:30   ` Patrick Palka
2015-06-15 14:54     ` Pedro Alves
2015-05-29 11:05 ` Pedro Alves
2015-06-17 18:19 ` Patrick Palka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).