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

Pedro Alves
2013-03-26T16:16:54Z

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