This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 03/26/2013 10:21 AM, Yao Qi wrote:

> Pedro,
> I understand that your point of choosing signed command is readline
> interface is signed.  

There's more than one point.

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

- using an unsigned variable for the user setting adds the
  need for an extra level of care, looking out for the
  surprising signed+unsigned comparisons causing unsigned
  conversions / overflows.

- using an unsigned variable led to mapping (INT_MAX..UINT_MAX)
  "unlimited".

  This might make sense at first sight, but it really exposes an
  implementation detail to the user interface that we should
  not expose.

  We don't allow using setting the size to UINT_MAX directly.  The
  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).
 
  We ended up only accepting that range because we shoehorned
  "unsigned" in, and we ended up exposing a worse
  implementation detail than the detail of "the implementation
  limits the history size to INT_MAX, assuming infinite available
  memory".  I hope it's obvious that I'm more worried of these kinds
  of things propagating to new commands over time through copy/paste
  than supporting such a large history file...

> However, my point of choosing unsigned command
> is the characteristic of command "set history size" is unsigned.  I
> don't know why readline interface is signed, but I don't want this
> affect or restrict the design of the command.

Making the implementation of the command signed does not affect
or restrict the design of the command.  At least, not today.
We still don't accept negative numbers.  But, making the command
signed usually makes the code that _uses_ the control variable simpler,
without dangers of the unsigned/signed comparisons and implicit
casts, etc..

@@ -1388,7 +1388,10 @@ show_commands (char *args, int from_tty)
   hist_len = history_size;
   for (offset = 0; offset < history_size; offset++)
     {
-      if (!history_get (history_base + offset))
+      /* Avoid overflow when passing 'history_base + offset' to
+	 history_get.  */
+      if ((offset + history_base > INT_MAX)
+	  || !history_get (history_base + offset))
 	{
 	  hist_len = offset;
 	  break;

This shows one of the points I'm making.  Making the setting's
control variable of different type of the rest of the code
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
 ("hmm? isn't that that comparison always false? hmm, no,
 the left size is is converted to unsigned first.  hmmm,
 what about wraparound in that addition then?")
as evidenced in this and the "set listsize" changes, the need
to consider this detail is easy to forget.

Anyway, I care more about the exported user interface than
the implementation.  If we're staying with unsigned,
then let's rename the variable to at least make it more
obvious that this variable is not one of GNU history's
public variables.

Looking deeper, I don't think we need that loop at all.
We can just read readline's history_length:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.  I'm hoping that "set foo unlimited" might enable
us to reconsider these magic 0s later at some point without
causing much disturbance...

We actually don't need to keep two variables in order to 
be able to revert to previous value on error -- we can just
query readline.

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

	* top.c (history_size): Rename to ...
	(history_size_setshow_var): ... this.  Add comment.
	(show_commands): Change local variable 'hist_len' back to 'int'.
	Set it from readline's 'history_length' instead of 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 | 71 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 7905b51..08419cb 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)
@@ -1381,19 +1384,9 @@ 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 = history_length;
 
   /* 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)
     {
@@ -1446,16 +1439,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
@@ -1508,11 +1515,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)
@@ -1635,7 +1658,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."),
-- 
1.7.11.7



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]