Re: Change "set history size" back to signed (Re: [committed]: [PATCH 1/3] var_integer -> var_uinteger)

[Pedro Alves <palves at redhat dot com>]

On 03/27/2013 02:13 AM, Yao Qi wrote:

> Probably, we can replace local variable 'hist_len' with 'history_length' in function show_commands.

Done.  I was leaving it behind as a placeholder for its comment, but,
looking again, the comment doesn't really add anything -- the code ends up
self explanatory.

> Anyway, the patch looks pretty good.

Thanks.  I've put it in, on mainline and 7.6.

----------------
Subject: Forbid set history size (INT_MAX..UINT_MAX)

The whole readline interface is signed, and works with the 0..INT_MAX
range.

We don't allow setting the size to UINT_MAX directly.  The documented
user visible interface is "use 0 for unlimited".  The UINT_MAX
representation is an implementation detail we could change, e.g., by
keeping a separate flag for "unlimited", which is actually what the
readline interface does (stifled vs non stifled).  Generically
speaking, exposing this detail to clients of the interface may make
our lives complicated when we find the need to extend the range of
some command in the future, and it's better if users
(frontends/scripts) aren't relying on anything but what we tell them
to use for "unlimited".  Making values other than 0 error out is the
way to prevent users from using those ranges inappropriately.  Quite
related, note:

    (gdb) set history size 0xffffffff
    integer 4294967295 out of range

  But,

    (gdb) set history size 0xfffffffe
    (gdb) show history size
    The size of the command history is unlimited.

    (gdb) set history size 0x100000000
    integer 4294967296 out of range

If values over INT_MAX are accepted as unlimited, then there's no good
argument for only accepting [INT_MAX..UINT_MAX) as valid "unlimited"
magic numbers, while not accepting [UINT_MAX..inf).

Making the setting's control variable of different type (unsigned int)
of the rest of the related code (int) adds the need to recall that one
variable among all these is unsigned, and that one need to think about
whether these comparisons are signed or unsigned, along with the
promotion/conversion rules.  Since this is an easy to forget detail,
this patch renames the variable to at least make it more obvious that
this variable is not one of GNU history's public int variables, which
are all signed.  We don't actually need the only code that presently
is affected by this, though, the code that is computing the current
history's length.  We can just use GNU history's history_length
instead:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Variable: int history_length
    The number of entries currently stored in the history list.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/* Return the history entry which is logically at OFFSET in the history array.
   OFFSET is relative to history_base. */
HIST_ENTRY *
history_get (offset)
     int offset;
{
  int local_index;

  local_index = offset - history_base;
  return (local_index >= history_length || local_index < 0 || the_history == 0)
		? (HIST_ENTRY *)NULL
		: the_history[local_index];
}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

At the time this code was added (gdb 4.13 ~1994), 'history_length' was
extern, but not documented in readline's GNU history documents, so I
guess it wasn't considered public then and the loop was the
workaround.

One of the warts of GDB choosing 0 to mean unlimited is that "set
history size 0" behaves differently from 'HISTSIZE=0 gdb'.  The latter
leaves GDB with no history, while the former means "unlimited"...

 $ HISTSIZE=0 ./gdb
 ...
 (gdb) show history size
 The size of the command history is 0.

We shouldn't really change what HISTSIZE=0 means, as bash, etc. also
handle 0 as real zero, and zero it's what really makes sense.

gdb/
2013-03-27  Pedro Alves  <palves@redhat.com>

	* top.c (history_size): Rename to ...
	(history_size_setshow_var): ... this.  Add comment.
	(show_commands): Use readline's 'history_length' instead of
	computing the history length by calling history_get in a loop.
	(set_history_size_command): Error out for sizes over INT_MAX.
	Restore previous history size on invalid size.
	(init_history): If HISTSIZE is negative, leave the history size as
	zero.  Add comments.
	(init_main): Adjust.
---
 gdb/top.c |   83 ++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 52 insertions(+), 31 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 645c898..e2c4c61 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -711,7 +711,10 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static unsigned int history_size;
+/* The variable associated with the "set/show history size"
+   command.  */
+static unsigned int history_size_setshow_var;
+
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1374,21 +1377,7 @@ show_commands (char *args, int from_tty)
      Relative to history_base.  */
   static int num = 0;
 
-  /* The first command in the history which doesn't exist (i.e. one more
-     than the number of the last command).  Relative to history_base.  */
-  unsigned int hist_len;
-
   /* Print out some of the commands from the command history.  */
-  /* First determine the length of the history list.  */
-  hist_len = history_size;
-  for (offset = 0; offset < history_size; offset++)
-    {
-      if (!history_get (history_base + offset))
-	{
-	  hist_len = offset;
-	  break;
-	}
-    }
 
   if (args)
     {
@@ -1402,7 +1391,7 @@ show_commands (char *args, int from_tty)
   /* "show commands" means print the last Hist_print commands.  */
   else
     {
-      num = hist_len - Hist_print;
+      num = history_length - Hist_print;
     }
 
   if (num < 0)
@@ -1410,14 +1399,16 @@ show_commands (char *args, int from_tty)
 
   /* If there are at least Hist_print commands, we want to display the last
      Hist_print rather than, say, the last 6.  */
-  if (hist_len - num < Hist_print)
+  if (history_length - num < Hist_print)
     {
-      num = hist_len - Hist_print;
+      num = history_length - Hist_print;
       if (num < 0)
 	num = 0;
     }
 
-  for (offset = num; offset < num + Hist_print && offset < hist_len; offset++)
+  for (offset = num;
+       offset < num + Hist_print && offset < history_length;
+       offset++)
     {
       printf_filtered ("%5d  %s\n", history_base + offset,
 		       (history_get (history_base + offset))->line);
@@ -1441,16 +1432,30 @@ show_commands (char *args, int from_tty)
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  /* The type of parameter in stifle_history is int, so values from INT_MAX up
-     mean 'unlimited'.  */
-  if (history_size >= INT_MAX)
+  /* 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)
     {
-      /* Ensure that 'show history size' prints 'unlimited'.  */
-      history_size = UINT_MAX;
-      unstifle_history ();
+      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);
+    stifle_history (history_size_setshow_var);
 }
 
 void
@@ -1503,11 +1508,27 @@ init_history (void)
 
   tmpenv = getenv ("HISTSIZE");
   if (tmpenv)
-    history_size = atoi (tmpenv);
-  else if (!history_size)
-    history_size = 256;
+    {
+      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;
+    }
+  /* If the init file hasn't set a size yet, pick the default.  */
+  else if (history_size_setshow_var == 0)
+    history_size_setshow_var = 256;
 
-  stifle_history (history_size);
+  /* Note that unlike "set history size 0", "HISTSIZE=0" really sets
+     the history size to 0...  */
+  stifle_history (history_size_setshow_var);
 
   tmpenv = getenv ("GDBHISTFILE");
   if (tmpenv)
@@ -1630,7 +1651,7 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
+  add_setshow_uinteger_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."),