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

[Pedro Alves <palves at redhat dot com>]

> 2012-08-28  Yao Qi  <yao@codesourcery.com>
> 
> 	* top.c (history_size): Add 'unsigned'.
> 	(show_commands): Change local variables to 'unsigned'.
> 	(set_history_size_command): Don't check history_size is negative.
> 	Adjust the condition to call unstifle_history and set history_size
> 	to UNIT_MAX.

I'd like to revert "set history size" back to signed.

http://sourceware.org/ml/gdb-patches/2012-08/msg00832.html

All the variables associated with the command are int,
and they need to be, because that's how the readline interfaces
are defined.

As it stands, this introduced signed/unsigned comparisons
and undefined overflows, like in:

void
show_commands (char *args, int from_tty)
{
  /* Index for history commands.  Relative to history_base.  */
  int offset;
...
  /* 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))
...
    }

With history_size set to UINT_MAX, with a (very) large history,
that will overflow the signed "offset".  Making "offset" itself
unsigned is useless, as then we'd overflow on the call to
history_get.

The fact that all the code that uses these interfaces and
variables is "signed", and that "history_size" ends up trimmed
to INT_MAX anyway:

/* Called by do_setshow_command.  */
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)
    {
      /* Ensure that 'show history size' prints 'unlimited'.  */
      history_size = UINT_MAX;
      unstifle_history ();
    }
  else
    stifle_history (history_size);
}

very much reads to me that making this "unsigned" is not
justified.

This patch changes the command back to var_integer.  It's
not a reversion -- I'm doing things differently from what
was done before.  Namely, if a negative value is specified,
the user sees the exact same error the uinteger command throws.
Also, in that case, the command reverts back to the previous
setting, like current code does implicitly, but unlike the original,
that would change the setting to the default on range error.
IOW, there's no user visible change compared to the current code.

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

	* top.c (history_size): Change type to back int.
	(setshow_history_size_var): New global.
	(show_commands): Change local variables back to 'int'.
	(set_history_size_command): Revert to previous setting and error
	out if user set the setting to negative.
	(init_history): Set setshow_history_size_var.
	(init_main): Change back "set/show history size" to integer
	command.
---

 gdb/top.c |   43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 7905b51..bc83496 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -711,7 +711,15 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }

-static unsigned int history_size;
+/* The history size, as set with the "set/show history size" command.
+   This is not the variable registered with the set/show commands
+   though.  */
+static int history_size;
+
+/* Variable associated with "set/show history size" command, as
+   control variable.  Needed for extra "set" validation.  */
+static int setshow_history_size_var;
+
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1381,7 +1389,7 @@ show_commands (char *args, int from_tty)

   /* 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;
+  int hist_len;

   /* Print out some of the commands from the command history.  */
   /* First determine the length of the history list.  */
@@ -1446,15 +1454,21 @@ 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)
+  if (setshow_history_size_var < 0)
     {
-      /* Ensure that 'show history size' prints 'unlimited'.  */
-      history_size = UINT_MAX;
-      unstifle_history ();
+      int var = setshow_history_size_var;
+
+      /* Restore.  */
+      setshow_history_size_var = history_size;
+      error (_("integer %d out of range"), var);
     }
-  else
+
+  /* Commit.  */
+  history_size = setshow_history_size_var;
+
+  if (history_size == INT_MAX)
+    unstifle_history ();
+  else if (history_size >= 0)
     stifle_history (history_size);
 }

@@ -1512,6 +1526,9 @@ init_history (void)
   else if (!history_size)
     history_size = 256;

+  /* Sync the set/show control variable.  */
+  setshow_history_size_var = history_size;
+
   stifle_history (history_size);

   tmpenv = getenv ("GDBHISTFILE");
@@ -1635,13 +1652,13 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);

-  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
+  add_setshow_integer_cmd ("size", no_class, &setshow_history_size_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."),
-			    set_history_size_command,
-			    show_history_size,
-			    &sethistlist, &showhistlist);
+			   set_history_size_command,
+			   show_history_size,
+			   &sethistlist, &showhistlist);

   add_setshow_filename_cmd ("filename", no_class, &history_filename, _("\
 Set the filename in which to record the command history"), _("\

-- 
Pedro Alves