[PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together.

[PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together.

[Yao Qi]

Hi,
Similar to previous patch, this patch is also to handle case
var_uinteger/var_zuinteger and case var_integer/var_zinteger together.
This change removes some duplicated code, and applies range checking
to the value of var_zuinteger and var_zinteger.

Note that my following patches will check the change of command
option for some purpose, so better to handle these case statements
as together we we can.

gdb:

2012-07-18  Yao Qi  <yao@codesourcery.com>

	* cli/cli-setshow.c (do_setshow_command): Handle case 'var_uinteger'
	and 'var_zuninteger' together.  Handle case 'var_integer' and
	'var_zinteger' together.
---
 gdb/cli/cli-setshow.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 0f854e5..fabc4d7 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -267,20 +267,22 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	  }
 	  break;
 	case var_uinteger:
+	case var_zuinteger:
 	  if (arg == NULL)
 	    error_no_arg (_("integer to set it to."));
 	  *(unsigned int *) c->var = parse_and_eval_long (arg);
-	  if (*(unsigned int *) c->var == 0)
+	  if (c->var_type == var_uinteger && *(unsigned int *) c->var == 0)
 	    *(unsigned int *) c->var = UINT_MAX;
 	  break;
 	case var_integer:
+	case var_zinteger:
 	  {
 	    unsigned int val;
 
 	    if (arg == NULL)
 	      error_no_arg (_("integer to set it to."));
 	    val = parse_and_eval_long (arg);
-	    if (val == 0)
+	    if (val == 0 && c->var_type == var_integer)
 	      *(int *) c->var = INT_MAX;
 	    else if (val >= INT_MAX)
 	      error (_("integer %u out of range"), val);
@@ -288,16 +290,6 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	      *(int *) c->var = val;
 	    break;
 	  }
-	case var_zinteger:
-	  if (arg == NULL)
-	    error_no_arg (_("integer to set it to."));
-	  *(int *) c->var = parse_and_eval_long (arg);
-	  break;
-	case var_zuinteger:
-	  if (arg == NULL)
-	    error_no_arg (_("integer to set it to."));
-	  *(unsigned int *) c->var = parse_and_eval_long (arg);
-	  break;
 	case var_enum:
 	  {
 	    int i;
-- 
1.7.7.6

[committed]: [PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together.

[Yao Qi]

On Wednesday, July 18, 2012 08:51:52 PM Yao Qi wrote:
> gdb:
> 
> 2012-07-18  Yao Qi  <yao@codesourcery.com>
> 
>         * cli/cli-setshow.c (do_setshow_command): Handle case 'var_uinteger'
> and 'var_zuninteger' together.  Handle case 'var_integer' and
> 'var_zinteger' together.

Regression tested on CVS trunk.  Applied.

-- 
Yao (齐尧)

Re: [PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together.

[Khoo Yit Phang]

Hi,

This patch broke "set remote hardware-breakpoint-limit -1" ("integer 4294967295 out of range"), which treats -1 as unlimited. The problem is the following lines:

>    case var_integer:
> +   case var_zinteger:
>      {
>        unsigned int val;


Since val is unsigned, -1 gets casted to UINT_MAX which subsequently fails:

>        else if (val >= INT_MAX)
>          error (_("integer %u out of range"), val);


I tried changing val to LONGEST, but that subsequently broke the "set listsize -1" test in the testsuite (or at least, it fails one of the test cases in testsuite/gdb.base/list.exp).

Actually, it's not clear to me what "set listsize -1" is supposed to do: in one place in the testsuite, it's supposed to be unlimited and "set listsize 0" is supposed to suppress printing, but in another place, "set listsize 0" is supposed to be unlimited. But running "set listsize -1", without my change, also leads to an error ("integer 4294967295 out of range"). The documentation does not make it clear either.

Yit
July 27, 2012


On Jul 18, 2012, at 2:51 PM, Yao Qi wrote:

> Hi,
> Similar to previous patch, this patch is also to handle case
> var_uinteger/var_zuinteger and case var_integer/var_zinteger together.
> This change removes some duplicated code, and applies range checking
> to the value of var_zuinteger and var_zinteger.
> 
> Note that my following patches will check the change of command
> option for some purpose, so better to handle these case statements
> as together we we can.
> 
> gdb:
> 
> 2012-07-18  Yao Qi  <yao@codesourcery.com>
> 
>    * cli/cli-setshow.c (do_setshow_command): Handle case 'var_uinteger'
>    and 'var_zuninteger' together.  Handle case 'var_integer' and
>    'var_zinteger' together.
> ---
> gdb/cli/cli-setshow.c |   16 ++++------------
> 1 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index 0f854e5..fabc4d7 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -267,20 +267,22 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
>      }
>      break;
>    case var_uinteger:
> +   case var_zuinteger:
>      if (arg == NULL)
>        error_no_arg (_("integer to set it to."));
>      *(unsigned int *) c->var = parse_and_eval_long (arg);
> -     if (*(unsigned int *) c->var == 0)
> +     if (c->var_type == var_uinteger && *(unsigned int *) c->var == 0)
>        *(unsigned int *) c->var = UINT_MAX;
>      break;
>    case var_integer:
> +   case var_zinteger:
>      {
>        unsigned int val;
> 
>        if (arg == NULL)
>          error_no_arg (_("integer to set it to."));
>        val = parse_and_eval_long (arg);
> -       if (val == 0)
> +       if (val == 0 && c->var_type == var_integer)
>          *(int *) c->var = INT_MAX;
>        else if (val >= INT_MAX)
>          error (_("integer %u out of range"), val);
> @@ -288,16 +290,6 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
>          *(int *) c->var = val;
>        break;
>      }
> -   case var_zinteger:
> -     if (arg == NULL)
> -       error_no_arg (_("integer to set it to."));
> -     *(int *) c->var = parse_and_eval_long (arg);
> -     break;
> -   case var_zuinteger:
> -     if (arg == NULL)
> -       error_no_arg (_("integer to set it to."));
> -     *(unsigned int *) c->var = parse_and_eval_long (arg);
> -     break;
>    case var_enum:
>      {
>        int i;
> -- 
> 1.7.7.6
> 

Re: [PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together.

[Yao Qi]

On Friday, July 27, 2012 01:40:06 PM Khoo Yit Phang wrote:
> This patch broke "set remote hardware-breakpoint-limit -1" ("integer 
4294967295 out of range"), which treats -1 as unlimited. The problem is the 
following lines:
> >    case var_integer:
> > +   case var_zinteger:
> >      {
> >        unsigned int val;
> 
> Since val is unsigned, -1 gets casted to UINT_MAX which subsequently fails:
> >        else if (val >= INT_MAX)
> >          error (_("integer %u out of range"), val);
> 
> I tried changing val to LONGEST, but that subsequently broke the "set
> listsize -1" test in the testsuite (or at least, it fails one of the test
> cases in testsuite/gdb.base/list.exp).
> 
> Actually, it's not clear to me what "set listsize -1" is supposed to do: in
> one place in the testsuite, it's supposed to be unlimited and "set listsize
> 0" is supposed to suppress printing, but in another place, "set listsize 0"
> is supposed to be unlimited. But running "set listsize -1", without my
> change, also leads to an error ("integer 4294967295 out of range"). The
> documentation does not make it clear either.

The inconsistency is confusing here.  I've started looking at this issue.  We 
did have a discussion on this problem in 2006.  I'll go through the archives, 
and give a reasonable fix to this.

-- 
Yao (齐尧)

[PATCH 3/3] use zuinteger_unlimited for heuristic-fence-post

[Yao Qi]

Hi,
The comment here gives me a feeling that we should
convert this command to zuinteger_unlimited_cmd.

gdb:

2012-08-01  Yao Qi  <yao@codesourcery.com>

	* alpha-tdep.c (_initialize_alpha_tdep): Call
	add_setshow_zuinteger_unlimited_cmd instead of
	add_setshow_zinteger_cmd.
	Remove comments.
	* mips-tdep.c (_initialize_mips_tdep): Likewise.
---
 gdb/alpha-tdep.c |   16 +++++++---------
 gdb/mips-tdep.c  |   16 +++++++---------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c
index 5d3affa..295bdec 100644
--- a/gdb/alpha-tdep.c
+++ b/gdb/alpha-tdep.c
@@ -1874,20 +1874,18 @@ _initialize_alpha_tdep (void)
 
   /* Let the user set the fence post for heuristic_proc_start.  */
 
-  /* We really would like to have both "0" and "unlimited" work, but
-     command.c doesn't deal with that.  So make it a var_zinteger
-     because the user can always use "999999" or some such for unlimited.  */
   /* We need to throw away the frame cache when we set this, since it
      might change our ability to get backtraces.  */
-  add_setshow_zinteger_cmd ("heuristic-fence-post", class_support,
-			    &heuristic_fence_post, _("\
+  add_setshow_zuinteger_unlimited_cmd ("heuristic-fence-post", class_support,
+				       &heuristic_fence_post, _("\
 Set the distance searched for the start of a function."), _("\
 Show the distance searched for the start of a function."), _("\
 If you are debugging a stripped executable, GDB needs to search through the\n\
 program for the start of a function.  This command sets the distance of the\n\
 search.  The only need to set it is when debugging a stripped executable."),
-			    reinit_frame_cache_sfunc,
-			    NULL, /* FIXME: i18n: The distance searched for
-				     the start of a function is \"%d\".  */
-			    &setlist, &showlist);
+				       reinit_frame_cache_sfunc,
+				       NULL, /* FIXME: i18n: The distance
+						searched for the start of a
+						function is \"%d\".  */
+				       &setlist, &showlist);
 }
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 0439989..32b5e12 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8835,20 +8835,18 @@ and is updated automatically from ELF file flags if available."),
 	   _("Show current use of MIPS floating-point coprocessor target."),
 	   &showlist);
 
-  /* We really would like to have both "0" and "unlimited" work, but
-     command.c doesn't deal with that.  So make it a var_zinteger
-     because the user can always use "999999" or some such for unlimited.  */
-  add_setshow_zinteger_cmd ("heuristic-fence-post", class_support,
-			    &heuristic_fence_post, _("\
+  add_setshow_zuinteger_unlimited_cmd ("heuristic-fence-post", class_support,
+				       &heuristic_fence_post, _("\
 Set the distance searched for the start of a function."), _("\
 Show the distance searched for the start of a function."), _("\
 If you are debugging a stripped executable, GDB needs to search through the\n\
 program for the start of a function.  This command sets the distance of the\n\
 search.  The only need to set it is when debugging a stripped executable."),
-			    reinit_frame_cache_sfunc,
-			    NULL, /* FIXME: i18n: The distance searched for
-				     the start of a function is %s.  */
-			    &setlist, &showlist);
+				       reinit_frame_cache_sfunc,
+				       NULL, /* FIXME: i18n: The distance
+						searched for the start of a
+						function is %s.  */
+				       &setlist, &showlist);
 
   /* Allow the user to control whether the upper bits of 64-bit
      addresses should be zeroed.  */
-- 
1.7.7.6

[PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.

[Yao Qi]

Hi,
This patch is to add new var_types 'var_zuinteger_unlimited' and
fix 'set listsize'.

gdb:

2012-08-01  Yao Qi  <yao@codesourcery.com>
	    Mark Kettenis  <kettenis@gnu.org>

	* cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): New.
	* cli/cli-setshow.c (do_setshow_command): Handle case
	'var_zuinteger_unlimited'.
	(do_setshow_command): Check the range for var_uinteger and
	var_zuinteger.
	* command.h (typedef enum var_types): New enum
	'var_zuinteger_unlimited'.
	Declare add_setshow_zuinteger_unlimited_cmd.
	* source.c (_initialize_source): Call add_setshow_zuinteger_unlimited_cmd.

gdb/doc:

2012-08-01  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (List): Describe the meaning of 0 and -1 in
	'set listsize'.

gdb/testsuite:

2012-08-01  Yao Qi  <yao@codesourcery.com>

	* gdb.base/list.exp (set_listsize): Don't set arg to "unlimited"
	when it is less than 0.
---
 gdb/cli/cli-decode.c            |   19 +++++++++++++
 gdb/cli/cli-setshow.c           |   56 ++++++++++++++++++++++++++++++++------
 gdb/command.h                   |   16 +++++++++++
 gdb/doc/gdb.texinfo             |    2 +
 gdb/source.c                    |    8 +++---
 gdb/testsuite/gdb.base/list.exp |    2 +-
 6 files changed, 89 insertions(+), 14 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index c337b43..bd679d8 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -627,6 +627,25 @@ add_setshow_zinteger_cmd (char *name, enum command_class class,
 			NULL, NULL);
 }
 
+void
+add_setshow_zuinteger_unlimited_cmd (char *name,
+				     enum command_class class,
+				     unsigned int *var,
+				     const char *set_doc,
+				     const char *show_doc,
+				     const char *help_doc,
+				     cmd_sfunc_ftype *set_func,
+				     show_value_ftype *show_func,
+				     struct cmd_list_element **set_list,
+				     struct cmd_list_element **show_list)
+{
+  add_setshow_cmd_full (name, class, var_zuinteger_unlimited, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			NULL, NULL);
+}
+
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  CLASS is as in
    add_cmd.  VAR is address of the variable which will contain the
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 7ffb89e..77c836b 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -209,24 +209,34 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	  break;
 	case var_uinteger:
 	case var_zuinteger:
-	  if (arg == NULL)
-	    error_no_arg (_("integer to set it to."));
-	  *(unsigned int *) c->var = parse_and_eval_long (arg);
-	  if (c->var_type == var_uinteger && *(unsigned int *) c->var == 0)
-	    *(unsigned int *) c->var = UINT_MAX;
+	  {
+	    LONGEST val;
+
+	    if (arg == NULL)
+	      error_no_arg (_("integer to set it to."));
+	    val = parse_and_eval_long (arg);
+
+	    if (val < 0 || val > UINT_MAX)
+	      error (_("integer %s out of range"), plongest (val));
+
+	    if (c->var_type == var_uinteger && val == 0)
+	      val = UINT_MAX;
+
+	    *(unsigned int *) c->var = val;
+	  }
 	  break;
 	case var_integer:
 	case var_zinteger:
 	  {
-	    unsigned int val;
+	    LONGEST val;
 
 	    if (arg == NULL)
 	      error_no_arg (_("integer to set it to."));
 	    val = parse_and_eval_long (arg);
 	    if (val == 0 && c->var_type == var_integer)
 	      *(int *) c->var = INT_MAX;
-	    else if (val >= INT_MAX)
-	      error (_("integer %u out of range"), val);
+	    else if (val < INT_MIN || val > INT_MAX)
+	      error (_("integer %s out of range"), plongest (val));
 	    else
 	      *(int *) c->var = val;
 	    break;
@@ -296,6 +306,27 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	    *(const char **) c->var = match;
 	  }
 	  break;
+	case var_zuinteger_unlimited:
+	  {
+	    LONGEST val;
+
+	    if (arg == NULL)
+	      error_no_arg (_("integer to set it to."));
+	    val = parse_and_eval_long (arg);
+
+	    /* Don't allow user to input UINT_MAX, which is reserved for
+	       'unlimited'.  */
+	    if (val >= UINT_MAX)
+	      error (_("integer %s out of range"), plongest (val));
+	    else if (val < -1)
+	      error (_("only -1 is allowed to set as unlimited"));
+
+	    if (val == -1)
+	      val = UINT_MAX;
+
+	    *(unsigned int *) c->var = val;
+	  }
+	  break;
 	default:
 	  error (_("gdb internal error: bad var_type in do_setshow_command"));
 	}
@@ -363,7 +394,14 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	  else
 	    fprintf_filtered (stb, "%d", *(int *) c->var);
 	  break;
-
+	case var_zuinteger_unlimited:
+	  {
+	    if (*(unsigned int *) c->var == UINT_MAX)
+	      fputs_filtered ("unlimited", stb);
+	    else
+	      fprintf_filtered (stb, "%u", *(unsigned int *) c->var);
+	  }
+	  break;
 	default:
 	  error (_("gdb internal error: bad var_type in do_setshow_command"));
 	}
diff --git a/gdb/command.h b/gdb/command.h
index 88895bb..fc09e69 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -99,6 +99,10 @@ typedef enum var_types
     /* ZeroableUnsignedInteger.  *VAR is an unsigned int.  Zero really
        means zero.  */
     var_zuinteger,
+    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an unsigned
+       int, but its range is [0, UINT_MAX - 1].  UINT_MAX stands for
+       unlimited.  */
+    var_zuinteger_unlimited,
     /* Enumerated type.  Can only have one of the specified values.
        *VAR is a char pointer to the name of the element that we
        find.  */
@@ -354,6 +358,18 @@ extern void add_setshow_zuinteger_cmd (char *name,
 				       struct cmd_list_element **set_list,
 				       struct cmd_list_element **show_list);
 
+extern void
+  add_setshow_zuinteger_unlimited_cmd (char *name,
+				       enum command_class class,
+				       unsigned int *var,
+				       const char *set_doc,
+				       const char *show_doc,
+				       const char *help_doc,
+				       cmd_sfunc_ftype *set_func,
+				       show_value_ftype *show_func,
+				       struct cmd_list_element **set_list,
+				       struct cmd_list_element **show_list);
+
 /* Do a "show" command for each thing on a command list.  */
 
 extern void cmd_show_list (struct cmd_list_element *, int, char *);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a4503bf..f481c95 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6705,6 +6705,8 @@ the @code{list} command.  You can change this using @code{set listsize}:
 @item set listsize @var{count}
 Make the @code{list} command display @var{count} source lines (unless
 the @code{list} argument explicitly specifies some other number).
+Setting the @var{count} to -1 means there's no limit and 0 suppress
+printing.
 
 @kindex show listsize
 @item show listsize
diff --git a/gdb/source.c b/gdb/source.c
index 0ff0782..31e104f 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1965,12 +1965,12 @@ The matching line number is also stored as the value of \"$_\"."));
       add_com_alias ("?", "reverse-search", class_files, 0);
     }
 
-  add_setshow_integer_cmd ("listsize", class_support, &lines_to_list, _("\
+  add_setshow_zuinteger_unlimited_cmd ("listsize", class_support,
+				       &lines_to_list, _("\
 Set number of source lines gdb will list by default."), _("\
 Show number of source lines gdb will list by default."), NULL,
-			    NULL,
-			    show_lines_to_list,
-			    &setlist, &showlist);
+				       NULL, show_lines_to_list,
+				       &setlist, &showlist);
 
   add_cmd ("substitute-path", class_files, set_substitute_path_command,
            _("\
diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index 16d25d2..f01b028 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -62,7 +62,7 @@ proc set_listsize { arg } {
     if [gdb_test "set listsize $arg" ".*" "setting listsize to $arg #$set_listsize_count"] {
 	return 0;
     }
-    if { $arg <= 0 } {
+    if { $arg < 0 } {
 	set arg "unlimited";
     }
 
-- 
1.7.7.6

[RFC 0/3] New var_types var_zuinteger_unlimited

[Yao Qi]

On Friday, July 27, 2012 01:40:06 PM Khoo Yit Phang wrote:
> >        else if (val >= INT_MAX)
> >          error (_("integer %u out of range"), val);
> 
> I tried changing val to LONGEST, but that subsequently broke the "set
> listsize -1" test in the testsuite (or at least, it fails one of the test
> cases in testsuite/gdb.base/list.exp).
> 
> Actually, it's not clear to me what "set listsize -1" is supposed to do: in
> one place in the testsuite, it's supposed to be unlimited and "set listsize
> 0" is supposed to suppress printing, but in another place, "set listsize 0"
> is supposed to be unlimited. But running "set listsize -1", without my
> change, also leads to an error ("integer 4294967295 out of range"). The
> documentation does not make it clear either.

When looking at the issue of 'set listsize -1', I found the following
patch and the discussion,

  [RFC] Clean up var_integer/var_uinteger/var_zinteger mess
  http://sourceware.org/ml/gdb-patches/2006-01/msg00488.html

I don't know why this patch wasn't committed, but this patch looks
good to me.  I pick up part of it but adding a new var_types
'var_zuinteger_unlimited', because command 'set listsize' requires
both 0 and unlimited (-1), which doesn't map to any enum var_types
so far.  After I examined the usage of command registrations in GDB,
I find there are more places (patch 2/3 and 3/3) require both 0 and
unlimited, so I decide to add 'var_zuinteger_unlimited'.  Comments are
appreciated.

Regression tested on x86_64-linux for patch 1/3 and 2/3.  Rebuild GDB with
--enable-targets=all for patch 3/3.

[PATCH 2/3] use zuinteger_unlimited for some remote commands

[Yao Qi]

Hi,
These 'set remot XXX-limit' commands have the similar requirements to
'set listsize', so I convert them from zinteger_cmd to
zuinteger_unlimited_cmd.  New tests are added to check these commands
work correctly.

gdb:

2012-08-01  Yao Qi  <yao@codesourcery.com>

	* remote.c: Add 'static' and 'unsigned' to remote_hw_watchpoint_limit,
	remote_hw_watchpoint_length_limit and remote_hw_breakpoint_limit.
	(remote_region_ok_for_hw_watchpoint): Don't check the case 'limit' is less
	than 0.
	(remote_check_watch_resources): Likewise.
	(_initialize_remote): Call add_setshow_zuinteger_unlimited_cmd instead of
	add_setshow_zinteger_cmd.

gdb/testsuite:

2012-08-01  Yao Qi  <yao@codesourcery.com>

	* gdb.base/setshow.exp: Test for setting ahd showing
	hardware-watchpoint-limit, hardware-watchpoint-length-limit and
	hardware-breakpoint-limit.
---
 gdb/remote.c                       |   53 ++++++++++++++++++-----------------
 gdb/testsuite/gdb.base/setshow.exp |   20 +++++++++++++
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6780212..53a1e72 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8109,17 +8109,15 @@ remote_remove_watchpoint (CORE_ADDR addr, int len, int type,
 }
 
 
-int remote_hw_watchpoint_limit = -1;
-int remote_hw_watchpoint_length_limit = -1;
-int remote_hw_breakpoint_limit = -1;
+static unsigned int remote_hw_watchpoint_limit = UINT_MAX;
+static unsigned int remote_hw_watchpoint_length_limit = UINT_MAX;
+static unsigned int remote_hw_breakpoint_limit = UINT_MAX;
 
 static int
 remote_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
 {
   if (remote_hw_watchpoint_length_limit == 0)
     return 0;
-  else if (remote_hw_watchpoint_length_limit < 0)
-    return 1;
   else if (len <= remote_hw_watchpoint_length_limit)
     return 1;
   else
@@ -8133,8 +8131,6 @@ remote_check_watch_resources (int type, int cnt, int ot)
     {
       if (remote_hw_breakpoint_limit == 0)
 	return 0;
-      else if (remote_hw_breakpoint_limit < 0)
-	return 1;
       else if (cnt <= remote_hw_breakpoint_limit)
 	return 1;
     }
@@ -8142,7 +8138,7 @@ remote_check_watch_resources (int type, int cnt, int ot)
     {
       if (remote_hw_watchpoint_limit == 0)
 	return 0;
-      else if (remote_hw_watchpoint_limit < 0)
+      else if (remote_hw_watchpoint_limit == UINT_MAX)
 	return 1;
       else if (ot)
 	return -1;
@@ -11432,33 +11428,38 @@ further restriction and ``limit'' to enable that restriction."),
 	   _("Show the maximum number of bytes per memory-read packet."),
 	   &remote_show_cmdlist);
 
-  add_setshow_zinteger_cmd ("hardware-watchpoint-limit", no_class,
-			    &remote_hw_watchpoint_limit, _("\
+  add_setshow_zuinteger_unlimited_cmd ("hardware-watchpoint-limit", no_class,
+				       &remote_hw_watchpoint_limit, _("\
 Set the maximum number of target hardware watchpoints."), _("\
 Show the maximum number of target hardware watchpoints."), _("\
 Specify a negative limit for unlimited."),
-			    NULL, NULL, /* FIXME: i18n: The maximum
-					   number of target hardware
-					   watchpoints is %s.  */
-			    &remote_set_cmdlist, &remote_show_cmdlist);
-  add_setshow_zinteger_cmd ("hardware-watchpoint-length-limit", no_class,
-			    &remote_hw_watchpoint_length_limit, _("\
+				       NULL, NULL, /* FIXME: i18n: The maximum
+						      number of target hardware
+						      watchpoints is %s.  */
+				       &remote_set_cmdlist,
+				       &remote_show_cmdlist);
+  add_setshow_zuinteger_unlimited_cmd ("hardware-watchpoint-length-limit",
+				       no_class,
+				       &remote_hw_watchpoint_length_limit, _("\
 Set the maximum length (in bytes) of a target hardware watchpoint."), _("\
 Show the maximum length (in bytes) of a target hardware watchpoint."), _("\
 Specify a negative limit for unlimited."),
-			    NULL, NULL, /* FIXME: i18n: The maximum
-                                           length (in bytes) of a target
-                                           hardware watchpoint is %s.  */
-			    &remote_set_cmdlist, &remote_show_cmdlist);
-  add_setshow_zinteger_cmd ("hardware-breakpoint-limit", no_class,
-			    &remote_hw_breakpoint_limit, _("\
+				       NULL, NULL, /* FIXME: i18n: The maximum
+						      length (in bytes) of a
+						      target hardware watchpoint
+						      is %s.  */
+				       &remote_set_cmdlist,
+				       &remote_show_cmdlist);
+  add_setshow_zuinteger_unlimited_cmd ("hardware-breakpoint-limit", no_class,
+				       &remote_hw_breakpoint_limit, _("\
 Set the maximum number of target hardware breakpoints."), _("\
 Show the maximum number of target hardware breakpoints."), _("\
 Specify a negative limit for unlimited."),
-			    NULL, NULL, /* FIXME: i18n: The maximum
-					   number of target hardware
-					   breakpoints is %s.  */
-			    &remote_set_cmdlist, &remote_show_cmdlist);
+				       NULL, NULL, /* FIXME: i18n: The maximum
+						      number of target hardware
+						      breakpoints is %s.  */
+				       &remote_set_cmdlist,
+				       &remote_show_cmdlist);
 
   add_setshow_integer_cmd ("remoteaddresssize", class_obscure,
 			   &remote_address_size, _("\
diff --git a/gdb/testsuite/gdb.base/setshow.exp b/gdb/testsuite/gdb.base/setshow.exp
index 9af5c30..7821024 100644
--- a/gdb/testsuite/gdb.base/setshow.exp
+++ b/gdb/testsuite/gdb.base/setshow.exp
@@ -256,3 +256,23 @@ gdb_test "show verbose" "Verbose printing of informational messages is on..*" "s
 gdb_test_no_output "set verbose off" "set verbose off" 
 #test show verbose off
 gdb_test "show verbose" "Verbosity is off..*" "show verbose (off)" 
+
+foreach remote_bpkt_option {
+    "hardware-watchpoint-limit"
+    "hardware-watchpoint-length-limit"
+    "hardware-breakpoint-limit"
+} {
+    gdb_test "show remote ${remote_bpkt_option}" \
+	"The maximum .* is unlimited.*" \
+	"show remote ${remote_bpkt_option} unlimited 1"
+    gdb_test_no_output "set remote ${remote_bpkt_option} 1"
+
+    gdb_test "set remote ${remote_bpkt_option} -2" \
+	"only -1 is allowed to set as unlimited.*"
+
+    gdb_test_no_output "set remote ${remote_bpkt_option} -1"
+    gdb_test "show remote ${remote_bpkt_option}" \
+	"The maximum .* is unlimited.*" \
+	"show remote ${remote_bpkt_option} unlimited 2"
+}
+
-- 
1.7.7.6

Re: [PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Wed, 1 Aug 2012 21:55:32 +0800
> 
> This patch is to add new var_types 'var_zuinteger_unlimited' and
> fix 'set listsize'.

Thanks.

> +Setting the @var{count} to -1 means there's no limit and 0 suppress
> +printing.

"Setting @var{count}", without "the".  Also, instead of "0 suppress
printing", I suggest "0 means suppress line display" (assuming I
understood correctly what you meant by "printing".

OK with those changes.

Re: [PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.

[Doug Evans]

On Wed, Aug 1, 2012 at 6:55 AM, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> This patch is to add new var_types 'var_zuinteger_unlimited' and
> fix 'set listsize'.
>
> gdb:
>
> 2012-08-01  Yao Qi  <yao@codesourcery.com>
>             Mark Kettenis  <kettenis@gnu.org>
>
>         * cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): New.
>         * cli/cli-setshow.c (do_setshow_command): Handle case
>         'var_zuinteger_unlimited'.
>         (do_setshow_command): Check the range for var_uinteger and
>         var_zuinteger.
>         * command.h (typedef enum var_types): New enum
>         'var_zuinteger_unlimited'.
>         Declare add_setshow_zuinteger_unlimited_cmd.
>         * source.c (_initialize_source): Call add_setshow_zuinteger_unlimited_cmd.

Nit: The changelog entry for source.c should mention "listsize".  And
the entry for do_setshow_command should also mention that it fixes the
setting of var_integer and var_zinteger to allow negative values.

Things are quite a mess, aren't they.
Adding yet another var_foo doesn't appeal to me, but we can't break
anything and being able to set both "0" and "unlimited" (expressed
however) would be nice.

Is there any real use-case that wants to be able to set negative
values and have "0" mean unlimited?  It's never (until this patch)
been the case that one can set negative values to var_integer.  Can we
remove (or at least deprecate) var_integer? [we'd switch all existing
uses to var_uinteger, the only remaining use would be the Python API]

btw, you also might want to update python/py-param.c.
Though py-param.c also doesn't handle var_zuinteger, and I don't mind
not extending the Python API until we know we want to.

Re: [PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.

[Yao Qi]

On Thursday, August 02, 2012 01:33:58 AM Doug Evans wrote:
> Nit: The changelog entry for source.c should mention "listsize".  And
> the entry for do_setshow_command should also mention that it fixes the
> setting of var_integer and var_zinteger to allow negative values.
> 

Thanks, I'll fix changelog entries.

> Things are quite a mess, aren't they.
> Adding yet another var_foo doesn't appeal to me, but we can't break
> anything and being able to set both "0" and "unlimited" (expressed
> however) would be nice.
> 

Agreed.

> Is there any real use-case that wants to be able to set negative
> values and have "0" mean unlimited?  It's never (until this patch)
> been the case that one can set negative values to var_integer.  Can we
> remove (or at least deprecate) var_integer? [we'd switch all existing
> uses to var_uinteger, the only remaining use would be the Python API]

Yes, I was inclined to switch *all* var_integer to var_uinteger, and it is 
still ongoing.  It is independent of this patch series, but my 'command option 
changes notification' patches reply on this series, so I hope this series can 
go in at first.

> 
> btw, you also might want to update python/py-param.c.
> Though py-param.c also doesn't handle var_zuinteger, and I don't mind
> not extending the Python API until we know we want to.

I'll look at python stuff in next step.

-- 
Yao (齐尧)

[PATCH 2/3] var_integer -> var_zuinteger_unlimited

[Yao Qi]

Hi,
With previous patch applied, var_integer are still used in two palces,
'listsize' and 'remotetimeout'.  They are similar, 0 is allowed, and -1
means unlimited.  Existing 'enum var_types' don't cover this kind of
command, so I create a new one 'var_zuinteger_unlimited'.

After this patch, var_integer is only used in python.

gdb:

2012-08-13  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): New.
	* cli/cli-setshow.c (do_set_command): Handle case
	'var_zuinteger_unlimited'.
	(do_show_command): Likewise.
	* cli/cli-cmds.c (init_cmds): Call add_setshow_zuinteger_unlimited_cmd
	for command 'remotetimeout'.
	* command.h (enum var_types): New zuinteger_unlimited.
	* source.c (_initialize_source): Call add_setshow_zuinteger_unlimited_cmd
	for command 'set listsize'.

gdb/doc:

2012-08-13  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo(List): Describe the meaning of 0 and -1 in
	'set listsize'.

2012-08-13  Yao Qi  <yao@codesourcery.com>

	* gdb.base/list.exp (set_listsize): Don't set arg to "unlimited"
	when it is less than 0.
---
 gdb/cli/cli-cmds.c              |    9 +++++----
 gdb/cli/cli-decode.c            |   19 +++++++++++++++++++
 gdb/cli/cli-setshow.c           |   30 +++++++++++++++++++++++++++++-
 gdb/command.h                   |   15 +++++++++++++++
 gdb/doc/gdb.texinfo             |    2 ++
 gdb/source.c                    |    8 ++++----
 gdb/testsuite/gdb.base/list.exp |    2 +-
 7 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 3d70c9e..4b23a0f 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1825,14 +1825,15 @@ is displayed."),
 			    show_remote_debug,
 			    &setdebuglist, &showdebuglist);
 
-  add_setshow_integer_cmd ("remotetimeout", no_class, &remote_timeout, _("\
+  add_setshow_zuinteger_unlimited_cmd ("remotetimeout", no_class,
+				       &remote_timeout, _("\
 Set timeout limit to wait for target to respond."), _("\
 Show timeout limit to wait for target to respond."), _("\
 This value is used to set the time limit for gdb to wait for a response\n\
 from the target."),
-			   NULL,
-			   show_remote_timeout,
-			   &setlist, &showlist);
+				       NULL,
+				       show_remote_timeout,
+				       &setlist, &showlist);
 
   add_prefix_cmd ("debug", no_class, set_debug,
 		  _("Generic command for setting gdb debugging flags"),
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 3c2e152..9e8218c 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -692,6 +692,25 @@ add_setshow_zinteger_cmd (char *name, enum command_class class,
 			NULL, NULL);
 }
 
+void
+add_setshow_zuinteger_unlimited_cmd (char *name,
+				     enum command_class class,
+				     unsigned int *var,
+				     const char *set_doc,
+				     const char *show_doc,
+				     const char *help_doc,
+				     cmd_sfunc_ftype *set_func,
+				     show_value_ftype *show_func,
+				     struct cmd_list_element **set_list,
+				     struct cmd_list_element **show_list)
+{
+  add_setshow_cmd_full (name, class, var_zuinteger_unlimited, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			NULL, NULL);
+}
+
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  CLASS is as in
    add_cmd.  VAR is address of the variable which will contain the
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 89e095a..9d8cb2e 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -379,6 +379,26 @@ do_set_command (char *arg, int from_tty, struct cmd_list_element *c)
 	  }
       }
       break;
+    case var_zuinteger_unlimited:
+      {
+	LONGEST val;
+
+	if (arg == NULL)
+	  error_no_arg (_("integer to set it to."));
+	val = parse_and_eval_long (arg);
+
+	if (val >= INT_MAX)
+	  error (_("integer %s out of range"), plongest (val));
+	else if (val < -1)
+	  error (_("only -1 is allowed to set as unlimited"));
+
+	if (*(int *) c->var != val)
+	  {
+	    *(int *) c->var = val;
+	    option_changed = 1;
+	  }
+      }
+      break;
     default:
       error (_("gdb internal error: bad var_type in do_setshow_command"));
     }
@@ -478,6 +498,7 @@ do_set_command (char *arg, int from_tty, struct cmd_list_element *c)
 	  break;
 	case var_integer:
 	case var_zinteger:
+	case var_zuinteger_unlimited:
 	  {
 	    char s[64];
 
@@ -562,7 +583,14 @@ do_show_command (char *arg, int from_tty, struct cmd_list_element *c)
       else
 	fprintf_filtered (stb, "%d", *(int *) c->var);
       break;
-
+    case var_zuinteger_unlimited:
+      {
+	if (*(int *) c->var == -1)
+	  fputs_filtered ("unlimited", stb);
+	else
+	  fprintf_filtered (stb, "%u", *(int *) c->var);
+      }
+      break;
     default:
       error (_("gdb internal error: bad var_type in do_show_command"));
     }
diff --git a/gdb/command.h b/gdb/command.h
index 88895bb..553bd717 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -99,6 +99,9 @@ typedef enum var_types
     /* ZeroableUnsignedInteger.  *VAR is an unsigned int.  Zero really
        means zero.  */
     var_zuinteger,
+    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an unsigned
+       int, but its range is [0, INT_MAX].  -1 stands for unlimited.  */
+    var_zuinteger_unlimited,
     /* Enumerated type.  Can only have one of the specified values.
        *VAR is a char pointer to the name of the element that we
        find.  */
@@ -354,6 +357,18 @@ extern void add_setshow_zuinteger_cmd (char *name,
 				       struct cmd_list_element **set_list,
 				       struct cmd_list_element **show_list);
 
+extern void
+  add_setshow_zuinteger_unlimited_cmd (char *name,
+				       enum command_class class,
+				       unsigned int *var,
+				       const char *set_doc,
+				       const char *show_doc,
+				       const char *help_doc,
+				       cmd_sfunc_ftype *set_func,
+				       show_value_ftype *show_func,
+				       struct cmd_list_element **set_list,
+				       struct cmd_list_element **show_list);
+
 /* Do a "show" command for each thing on a command list.  */
 
 extern void cmd_show_list (struct cmd_list_element *, int, char *);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5cc5b48..2ccfecd 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6706,6 +6706,8 @@ the @code{list} command.  You can change this using @code{set listsize}:
 @item set listsize @var{count}
 Make the @code{list} command display @var{count} source lines (unless
 the @code{list} argument explicitly specifies some other number).
+Setting @var{count} to -1 means there's no limit and 0 means suppress
+lines display.
 
 @kindex show listsize
 @item show listsize
diff --git a/gdb/source.c b/gdb/source.c
index 0ff0782..31e104f 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1965,12 +1965,12 @@ The matching line number is also stored as the value of \"$_\"."));
       add_com_alias ("?", "reverse-search", class_files, 0);
     }
 
-  add_setshow_integer_cmd ("listsize", class_support, &lines_to_list, _("\
+  add_setshow_zuinteger_unlimited_cmd ("listsize", class_support,
+				       &lines_to_list, _("\
 Set number of source lines gdb will list by default."), _("\
 Show number of source lines gdb will list by default."), NULL,
-			    NULL,
-			    show_lines_to_list,
-			    &setlist, &showlist);
+				       NULL, show_lines_to_list,
+				       &setlist, &showlist);
 
   add_cmd ("substitute-path", class_files, set_substitute_path_command,
            _("\
diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
index 6b5b207..9acb1c3 100644
--- a/gdb/testsuite/gdb.base/list.exp
+++ b/gdb/testsuite/gdb.base/list.exp
@@ -62,7 +62,7 @@ proc set_listsize { arg } {
     if [gdb_test "set listsize $arg" ".*" "setting listsize to $arg #$set_listsize_count"] {
 	return 0;
     }
-    if { $arg <= 0 } {
+    if { $arg < 0 } {
 	set arg "unlimited";
     }
 
-- 
1.7.7.6

[PATCH 1/3] var_integer -> var_uinteger

[Yao Qi]

Hi,
The ngative value makes no sense to these commands, so this patch simply
changes them to var_uinteger.

gdb:

2012-08-13  Yao Qi  <yao@codesourcery.com>

	* cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
	(init_cmds): Call add_setshow_uinteger_cmd for command
	'max-user-call-depth'.
	* cli/cli-script.c (execute_user_command): Add 'unsigned' to the
	declaration of 'max_user_call_depth'.
	* frame.c (backtrace_limit): Add 'unsigned'.
	(_initialize_frame): Call add_setshow_uinteger_cmd for command
	'limit'.
	* remote.c (remoteaddresssize): Add 'unsigned'.
	(remote_address_masked): Change local var 'address_size' to
	'unsigned'.
	(_initialize_remote): Call add_setshow_uinteger_cmd for
	'remoteaddresssize'.
	* top.c (history_size): Add 'unsigned'.
	(show_commands): Change local variables to 'unsigned'.
	(set_history_size_command): Don't check history_size is negative.
---
 gdb/cli/cli-cmds.c   |   10 +++++-----
 gdb/cli/cli-script.c |    2 +-
 gdb/frame.c          |   14 +++++++-------
 gdb/remote.c         |   14 +++++++-------
 gdb/top.c            |   24 +++++++++++-------------
 5 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 2a80803..3d70c9e 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -96,7 +96,7 @@ static void filter_sals (struct symtabs_and_lines *);
 
 \f
 /* Limit the call depth of user-defined commands */
-int max_user_call_depth;
+unsigned int max_user_call_depth;
 
 /* Define all cmd_list_elements.  */
 
@@ -1906,13 +1906,13 @@ With no argument, show definitions of all user defined commands."), &showlist);
   add_com ("apropos", class_support, apropos_command,
 	   _("Search for commands matching a REGEXP"));
 
-  add_setshow_integer_cmd ("max-user-call-depth", no_class,
+  add_setshow_uinteger_cmd ("max-user-call-depth", no_class,
 			   &max_user_call_depth, _("\
 Set the max call depth for non-python user-defined commands."), _("\
 Show the max call depth for non-python user-defined commands."), NULL,
-			   NULL,
-			   show_max_user_call_depth,
-			   &setlist, &showlist);
+			    NULL,
+			    show_max_user_call_depth,
+			    &setlist, &showlist);
 
   add_setshow_boolean_cmd ("trace-commands", no_class, &trace_commands, _("\
 Set tracing of GDB CLI commands."), _("\
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 4b6c416..743c65f 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -314,7 +314,7 @@ execute_user_command (struct cmd_list_element *c, char *args)
   struct cleanup *old_chain;
   enum command_control_type ret;
   static int user_call_depth = 0;
-  extern int max_user_call_depth;
+  extern unsigned int max_user_call_depth;
 
   cmdlines = c->user_commands;
   if (cmdlines == 0)
diff --git a/gdb/frame.c b/gdb/frame.c
index 278269d..9ed49f6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -196,7 +196,7 @@ show_backtrace_past_entry (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int backtrace_limit = INT_MAX;
+static unsigned int backtrace_limit = UINT_MAX;
 static void
 show_backtrace_limit (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
@@ -2488,16 +2488,16 @@ the rest of the stack trace."),
 			   &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_integer_cmd ("limit", class_obscure,
-			   &backtrace_limit, _("\
+  add_setshow_uinteger_cmd ("limit", class_obscure,
+			    &backtrace_limit, _("\
 Set an upper bound on the number of backtrace levels."), _("\
 Show the upper bound on the number of backtrace levels."), _("\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited."),
-			   NULL,
-			   show_backtrace_limit,
-			   &set_backtrace_cmdlist,
-			   &show_backtrace_cmdlist);
+			    NULL,
+			    show_backtrace_limit,
+			    &set_backtrace_cmdlist,
+			    &show_backtrace_cmdlist);
 
   /* Debug this files internals.  */
   add_setshow_zuinteger_cmd ("frame", class_maintenance, &frame_debug,  _("\
diff --git a/gdb/remote.c b/gdb/remote.c
index a974dc1..b7bf29e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -834,7 +834,7 @@ static struct serial *remote_desc = NULL;
    some remote targets this variable is principly provided to
    facilitate backward compatibility.  */
 
-static int remote_address_size;
+static unsigned int remote_address_size;
 
 /* Temporary to track who currently owns the terminal.  See
    remote_terminal_* for more details.  */
@@ -6313,7 +6313,7 @@ hexnumnstr (char *buf, ULONGEST num, int width)
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  int address_size = remote_address_size;
+  unsigned int address_size = remote_address_size;
 
   /* If "remoteaddresssize" was not set, default to target address size.  */
   if (!address_size)
@@ -11460,13 +11460,13 @@ Specify a negative limit for unlimited."),
 					   breakpoints is %s.  */
 			    &remote_set_cmdlist, &remote_show_cmdlist);
 
-  add_setshow_integer_cmd ("remoteaddresssize", class_obscure,
-			   &remote_address_size, _("\
+  add_setshow_uinteger_cmd ("remoteaddresssize", class_obscure,
+			    &remote_address_size, _("\
 Set the maximum size of the address (in bits) in a memory packet."), _("\
 Show the maximum size of the address (in bits) in a memory packet."), NULL,
-			   NULL,
-			   NULL, /* FIXME: i18n: */
-			   &setlist, &showlist);
+			    NULL,
+			    NULL, /* FIXME: i18n: */
+			    &setlist, &showlist);
 
   add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
 			 "X", "binary-download", 1);
diff --git a/gdb/top.c b/gdb/top.c
index 8251d1b..fbd4120 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int history_size;
+static unsigned int history_size;
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1379,7 +1379,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.  */
-  int hist_len;
+  unsigned int hist_len;
 
   /* Print out some of the commands from the command history.  */
   /* First determine the length of the history list.  */
@@ -1444,15 +1444,13 @@ show_commands (char *args, int from_tty)
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  if (history_size == INT_MAX)
+  if (history_size == UINT_MAX)
     unstifle_history ();
-  else if (history_size >= 0)
-    stifle_history (history_size);
+  else if (history_size >= INT_MAX)
+    /* The type of parameter in stifle_history is int, so check the range.  */
+    error (_("History size must be less than %d"), INT_MAX);
   else
-    {
-      history_size = INT_MAX;
-      error (_("History size must be non-negative"));
-    }
+    stifle_history (history_size);
 }
 
 void
@@ -1632,13 +1630,13 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_integer_cmd ("size", no_class, &history_size, _("\
+  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
 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"), _("\
-- 
1.7.7.6

[PATCH 3/3] comments update

[Yao Qi]

gdb:

2012-08-13  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c: Update comment to add_setshow_integer_cmd.
	* command.h (enum var_types): Update comment to var_integer.
---
 gdb/cli/cli-decode.c |    3 ++-
 gdb/command.h        |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 9e8218c..5e93f99 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -632,7 +632,8 @@ add_setshow_optional_filename_cmd (char *name, enum command_class class,
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  CLASS is as in
    add_cmd.  VAR is address of the variable which will contain the
-   value.  SET_DOC and SHOW_DOC are the documentation strings.  */
+   value.  SET_DOC and SHOW_DOC are the documentation strings.  This
+   function is only used in Python API.  Please don't use it elsewhere.  */
 void
 add_setshow_integer_cmd (char *name, enum command_class class,
 			 int *var,
diff --git a/gdb/command.h b/gdb/command.h
index 553bd717..b88bd8b 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -77,7 +77,8 @@ typedef enum var_types
 
     /* Like var_uinteger but signed.  *VAR is an int.  The user can
        type 0 to mean "unlimited", which is stored in *VAR as
-       INT_MAX.  */
+       INT_MAX.  The only remaining use of it is the Python API.
+       Don't use it elsewhere.  */
     var_integer,
 
     /* String which the user enters with escapes (e.g. the user types
-- 
1.7.7.6

[RFC 0/3] Get rid of var_integer in CLI

[Yao Qi]

Hello,
This patch set can be regarded as the preparation for this series,

  [RFC 0/3] New var_types var_zuinteger_unlimited
  http://sourceware.org/ml/gdb-patches/2012-08/msg00020.html

In this version, we try to get rid of 'var_integer' first and it
will not be used except in Python API.  As the meaning of var_integer
is strange, we'd better not use it any more (but we should keep it
there for Python API).  What do you think?

Regression tested on x86_64-linux.

Re: [PATCH 2/3] var_integer -> var_zuinteger_unlimited

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Mon, 13 Aug 2012 23:27:42 +0800
> 
>  @item set listsize @var{count}
>  Make the @code{list} command display @var{count} source lines (unless
>  the @code{list} argument explicitly specifies some other number).
> +Setting @var{count} to -1 means there's no limit and 0 means suppress
> +lines display.                                               ^^^^^^^^
   ^^^^^^^^^^^^^

I think "suppress display of source lines" is better.

OK with that change.

Thanks.

[ping] : [RFC 0/3] Get rid of var_integer in CLI

[Yao Qi]

On 08/13/2012 11:27 PM, Yao Qi wrote:
> Hello,
> This patch set can be regarded as the preparation for this series,
>
>    [RFC 0/3] New var_types var_zuinteger_unlimited
>    http://sourceware.org/ml/gdb-patches/2012-08/msg00020.html
>
> In this version, we try to get rid of 'var_integer' first and it
> will not be used except in Python API.  As the meaning of var_integer
> is strange, we'd better not use it any more (but we should keep it
> there for Python API).  What do you think?
>
> Regression tested on x86_64-linux.
>

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

-- 
Yao

Re: [PATCH 1/3] var_integer -> var_uinteger

[dje]

Yao Qi writes:
 > Hi,
 > The ngative value makes no sense to these commands, so this patch simply
 > changes them to var_uinteger.

Hi.  Most of the patch is ok with me.
Further comments inline.

 > gdb:
 > 
 > 2012-08-13  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
 > 	(init_cmds): Call add_setshow_uinteger_cmd for command
 > 	'max-user-call-depth'.
 > 	* cli/cli-script.c (execute_user_command): Add 'unsigned' to the
 > 	declaration of 'max_user_call_depth'.
 > 	* frame.c (backtrace_limit): Add 'unsigned'.
 > 	(_initialize_frame): Call add_setshow_uinteger_cmd for command
 > 	'limit'.
 > 	* remote.c (remoteaddresssize): Add 'unsigned'.
 > 	(remote_address_masked): Change local var 'address_size' to
 > 	'unsigned'.
 > 	(_initialize_remote): Call add_setshow_uinteger_cmd for
 > 	'remoteaddresssize'.
 > 	* top.c (history_size): Add 'unsigned'.
 > 	(show_commands): Change local variables to 'unsigned'.
 > 	(set_history_size_command): Don't check history_size is negative.
 > [...]
 > diff --git a/gdb/top.c b/gdb/top.c
 > index 8251d1b..fbd4120 100644
 > --- a/gdb/top.c
 > +++ b/gdb/top.c
 > @@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
 >  		    value);
 >  }
 >  
 > -static int history_size;
 > +static unsigned int history_size;
 >  static void
 >  show_history_size (struct ui_file *file, int from_tty,
 >  		   struct cmd_list_element *c, const char *value)
 > @@ -1379,7 +1379,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.  */
 > -  int hist_len;
 > +  unsigned int hist_len;
 >  
 >    /* Print out some of the commands from the command history.  */
 >    /* First determine the length of the history list.  */
 > @@ -1444,15 +1444,13 @@ show_commands (char *args, int from_tty)
 >  static void
 >  set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 >  {
 > -  if (history_size == INT_MAX)
 > +  if (history_size == UINT_MAX)
 >      unstifle_history ();
 > -  else if (history_size >= 0)
 > -    stifle_history (history_size);
 > +  else if (history_size >= INT_MAX)
 > +    /* The type of parameter in stifle_history is int, so check the range.  */
 > +    error (_("History size must be less than %d"), INT_MAX);

I think we don't want to remove resetting "history_size = INT_MAX;"
in the error case here (but see below).
Also, new style rules say that since the clause is multiple lines
it needs to be wrapped in {}.

It's kinda funny that explicitly setting the value to UINT_MAX (instead
of zero) is ok, but setting the value to anywhere between INT_MAX and
UINT_MAX-1 is an error because it's too big. :-)
That makes me think it'd be better to just bite the bullet and say
any value from INT_MAX and up means "unlimited".
They're effectively that anyway.

 >    else
 > -    {
 > -      history_size = INT_MAX;
 > -      error (_("History size must be non-negative"));
 > -    }
 > +    stifle_history (history_size);
 >  }
 >  
 >  void
 > @@ -1632,13 +1630,13 @@ Without an argument, saving is enabled."),
 >  			   show_write_history_p,
 >  			   &sethistlist, &showhistlist);
 >  
 > -  add_setshow_integer_cmd ("size", no_class, &history_size, _("\
 > +  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
 >  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"), _("\
 > -- 
 > 1.7.7.6
 > 

Re: [PATCH 1/3] var_integer -> var_uinteger

[Yao Qi]

On 08/24/2012 02:19 AM, dje@google.com wrote:
>   > -  if (history_size == INT_MAX)
>   > +  if (history_size == UINT_MAX)
>   >      unstifle_history ();
>   > -  else if (history_size >= 0)
>   > -    stifle_history (history_size);
>   > +  else if (history_size >= INT_MAX)
>   > +    /* The type of parameter in stifle_history is int, so check the range.  */
>   > +    error (_("History size must be less than %d"), INT_MAX);
> 
> I think we don't want to remove resetting "history_size = INT_MAX;"
> in the error case here (but see below).

Checking the value is negative should be done in do_set_command, and
patch was posted here,

  [PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.
  http://sourceware.org/ml/gdb-patches/2012-08/msg00021.html

Posting a new one has been on my TODO list, and I'll do it once this
patch goes in.

> Also, new style rules say that since the clause is multiple lines
> it needs to be wrapped in {}.
> 

OK.

> It's kinda funny that explicitly setting the value to UINT_MAX (instead
> of zero) is ok, but setting the value to anywhere between INT_MAX and
> UINT_MAX-1 is an error because it's too big.:-)
> That makes me think it'd be better to just bite the bullet and say
> any value from INT_MAX and up means "unlimited".
> They're effectively that anyway.

Agreed.  Here is a new version.  I'll commit it in three days.

-- 
Yao

gdb:

2012-08-24  Yao Qi  <yao@codesourcery.com>

	* cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
	(init_cmds): Call add_setshow_uinteger_cmd for command
	'max-user-call-depth'.
	* cli/cli-script.c (execute_user_command): Add 'unsigned' to the
	declaration of 'max_user_call_depth'.
	* frame.c (backtrace_limit): Add 'unsigned'.
	(_initialize_frame): Call add_setshow_uinteger_cmd for command
	'limit'.
	* remote.c (remoteaddresssize): Add 'unsigned'.
	(remote_address_masked): Change local var 'address_size' to
	'unsigned'.
	(_initialize_remote): Call add_setshow_uinteger_cmd for
	'remoteaddresssize'.
	* 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.
---
 gdb/cli/cli-cmds.c   |   10 +++++-----
 gdb/cli/cli-script.c |    2 +-
 gdb/frame.c          |   14 +++++++-------
 gdb/remote.c         |   14 +++++++-------
 gdb/top.c            |   23 ++++++++++-------------
 5 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5d8a124..d3473d5 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -96,7 +96,7 @@ static void filter_sals (struct symtabs_and_lines *);
 
 \f
 /* Limit the call depth of user-defined commands */
-int max_user_call_depth;
+unsigned int max_user_call_depth;
 
 /* Define all cmd_list_elements.  */
 
@@ -1907,13 +1907,13 @@ With no argument, show definitions of all user defined commands."), &showlist);
   add_com ("apropos", class_support, apropos_command,
 	   _("Search for commands matching a REGEXP"));
 
-  add_setshow_integer_cmd ("max-user-call-depth", no_class,
+  add_setshow_uinteger_cmd ("max-user-call-depth", no_class,
 			   &max_user_call_depth, _("\
 Set the max call depth for non-python user-defined commands."), _("\
 Show the max call depth for non-python user-defined commands."), NULL,
-			   NULL,
-			   show_max_user_call_depth,
-			   &setlist, &showlist);
+			    NULL,
+			    show_max_user_call_depth,
+			    &setlist, &showlist);
 
   add_setshow_boolean_cmd ("trace-commands", no_class, &trace_commands, _("\
 Set tracing of GDB CLI commands."), _("\
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 4b6c416..743c65f 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -314,7 +314,7 @@ execute_user_command (struct cmd_list_element *c, char *args)
   struct cleanup *old_chain;
   enum command_control_type ret;
   static int user_call_depth = 0;
-  extern int max_user_call_depth;
+  extern unsigned int max_user_call_depth;
 
   cmdlines = c->user_commands;
   if (cmdlines == 0)
diff --git a/gdb/frame.c b/gdb/frame.c
index 278269d..9ed49f6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -196,7 +196,7 @@ show_backtrace_past_entry (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int backtrace_limit = INT_MAX;
+static unsigned int backtrace_limit = UINT_MAX;
 static void
 show_backtrace_limit (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
@@ -2488,16 +2488,16 @@ the rest of the stack trace."),
 			   &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_integer_cmd ("limit", class_obscure,
-			   &backtrace_limit, _("\
+  add_setshow_uinteger_cmd ("limit", class_obscure,
+			    &backtrace_limit, _("\
 Set an upper bound on the number of backtrace levels."), _("\
 Show the upper bound on the number of backtrace levels."), _("\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited."),
-			   NULL,
-			   show_backtrace_limit,
-			   &set_backtrace_cmdlist,
-			   &show_backtrace_cmdlist);
+			    NULL,
+			    show_backtrace_limit,
+			    &set_backtrace_cmdlist,
+			    &show_backtrace_cmdlist);
 
   /* Debug this files internals.  */
   add_setshow_zuinteger_cmd ("frame", class_maintenance, &frame_debug,  _("\
diff --git a/gdb/remote.c b/gdb/remote.c
index 2db2c9d..528f374 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -834,7 +834,7 @@ static struct serial *remote_desc = NULL;
    some remote targets this variable is principly provided to
    facilitate backward compatibility.  */
 
-static int remote_address_size;
+static unsigned int remote_address_size;
 
 /* Temporary to track who currently owns the terminal.  See
    remote_terminal_* for more details.  */
@@ -6314,7 +6314,7 @@ hexnumnstr (char *buf, ULONGEST num, int width)
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  int address_size = remote_address_size;
+  unsigned int address_size = remote_address_size;
 
   /* If "remoteaddresssize" was not set, default to target address size.  */
   if (!address_size)
@@ -11461,13 +11461,13 @@ Specify a negative limit for unlimited."),
 					   breakpoints is %s.  */
 			    &remote_set_cmdlist, &remote_show_cmdlist);
 
-  add_setshow_integer_cmd ("remoteaddresssize", class_obscure,
-			   &remote_address_size, _("\
+  add_setshow_uinteger_cmd ("remoteaddresssize", class_obscure,
+			    &remote_address_size, _("\
 Set the maximum size of the address (in bits) in a memory packet."), _("\
 Show the maximum size of the address (in bits) in a memory packet."), NULL,
-			   NULL,
-			   NULL, /* FIXME: i18n: */
-			   &setlist, &showlist);
+			    NULL,
+			    NULL, /* FIXME: i18n: */
+			    &setlist, &showlist);
 
   add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
 			 "X", "binary-download", 1);
diff --git a/gdb/top.c b/gdb/top.c
index 7084116..2c049bf 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int history_size;
+static unsigned int history_size;
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1380,7 +1380,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.  */
-  int hist_len;
+  unsigned int hist_len;
 
   /* Print out some of the commands from the command history.  */
   /* First determine the length of the history list.  */
@@ -1445,15 +1445,12 @@ show_commands (char *args, int from_tty)
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  if (history_size == INT_MAX)
+  /* The type of parameter in stifle_history is int, so value from INT_MAX and
+     up means "unlimited".  */
+  if (history_size >= INT_MAX)
     unstifle_history ();
-  else if (history_size >= 0)
-    stifle_history (history_size);
   else
-    {
-      history_size = INT_MAX;
-      error (_("History size must be non-negative"));
-    }
+    stifle_history (history_size);
 }
 
 void
@@ -1633,13 +1630,13 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_integer_cmd ("size", no_class, &history_size, _("\
+  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
 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"), _("\
-- 
1.7.7.6

Re: [PATCH 1/3] var_integer -> var_uinteger

[dje]

Yao Qi writes:
 > On 08/24/2012 02:19 AM, dje@google.com wrote:
 > >   > -  if (history_size == INT_MAX)
 > >   > +  if (history_size == UINT_MAX)
 > >   >      unstifle_history ();
 > >   > -  else if (history_size >= 0)
 > >   > -    stifle_history (history_size);
 > >   > +  else if (history_size >= INT_MAX)
 > >   > +    /* The type of parameter in stifle_history is int, so check the range.  */
 > >   > +    error (_("History size must be less than %d"), INT_MAX);
 > > 
 > > I think we don't want to remove resetting "history_size = INT_MAX;"
 > > in the error case here (but see below).
 > 
 > Checking the value is negative should be done in do_set_command, and
 > patch was posted here,
 > 
 >   [PATCH 1/3] var_zuinteger_unlimited and 'set listsize'.
 >   http://sourceware.org/ml/gdb-patches/2012-08/msg00021.html
 > 
 > Posting a new one has been on my TODO list, and I'll do it once this
 > patch goes in.

Ah.
My point was that if the user sets the value to INT_MAX+1, for example, and you flag that as an error, then the value should be reset back to something legal.
But no matter, if we go with all values being legal. :-)

 > @@ -1445,15 +1445,12 @@ show_commands (char *args, int from_tty)
 >  static void
 >  set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 >  {
 > -  if (history_size == INT_MAX)
 > +  /* The type of parameter in stifle_history is int, so value from INT_MAX and
 > +     up means "unlimited".  */
 > +  if (history_size >= INT_MAX)
 >      unstifle_history ();
 > -  else if (history_size >= 0)
 > -    stifle_history (history_size);
 >    else
 > -    {
 > -      history_size = INT_MAX;
 > -      error (_("History size must be non-negative"));
 > -    }
 > +    stifle_history (history_size);
 >  }

One issue that arises is if the user sets the value to, say, INT_MAX+1 we call unstifle_history, but show "show hist size" will print INT_MAX+1 instead of "unlimited".
If we go this route, perhaps any value >= INT_MAX should cause the underlying value to be set to UINT_MAX so that "show" will print "unlimited".

Re: [PATCH 1/3] var_integer -> var_uinteger

[Yao Qi]

On 08/25/2012 01:05 AM, dje@google.com wrote:
>> @@ -1445,15 +1445,12 @@ show_commands (char *args, int from_tty)
>   >  static void
>   >  set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
>   >  {
>   > -  if (history_size == INT_MAX)
>   > +  /* The type of parameter in stifle_history is int, so value from INT_MAX and
>   > +     up means "unlimited".  */
>   > +  if (history_size >= INT_MAX)
>   >      unstifle_history ();
>   > -  else if (history_size >= 0)
>   > -    stifle_history (history_size);
>   >    else
>   > -    {
>   > -      history_size = INT_MAX;
>   > -      error (_("History size must be non-negative"));
>   > -    }
>   > +    stifle_history (history_size);
>   >  }
> 
> One issue that arises is if the user sets the value to, say, INT_MAX+1 we call unstifle_history, but show "show hist size" will print INT_MAX+1 instead of "unlimited".
> If we go this route, perhaps any value >= INT_MAX should cause the underlying value to be set to UINT_MAX so that "show" will print "unlimited".

Good catch! :)  I changed this part as below,

  /* The type of parameter in stifle_history is int, so value from INT_MAX up
     means 'unlimited'.  */
  if (history_size > INT_MAX)
    {
      /* Ensure that 'show history size' print 'unlimited'.  */
      history_size = UINT_MAX;
      unstifle_history ();
    }
  else
    stifle_history (history_size);

-- 
Yao

gdb:

2012-08-27  Yao Qi  <yao@codesourcery.com>

	* cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
	(init_cmds): Call add_setshow_uinteger_cmd for command
	'max-user-call-depth'.
	* cli/cli-script.c (execute_user_command): Add 'unsigned' to the
	declaration of 'max_user_call_depth'.
	* frame.c (backtrace_limit): Add 'unsigned'.
	(_initialize_frame): Call add_setshow_uinteger_cmd for command
	'limit'.
	* remote.c (remoteaddresssize): Add 'unsigned'.
	(remote_address_masked): Change local var 'address_size' to
	'unsigned'.
	(_initialize_remote): Call add_setshow_uinteger_cmd for
	'remoteaddresssize'.
	* 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.
---
 gdb/cli/cli-cmds.c   |   10 +++++-----
 gdb/cli/cli-script.c |    2 +-
 gdb/frame.c          |   14 +++++++-------
 gdb/remote.c         |   14 +++++++-------
 gdb/top.c            |   27 ++++++++++++++-------------
 5 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5d8a124..d3473d5 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -96,7 +96,7 @@ static void filter_sals (struct symtabs_and_lines *);
 
 \f
 /* Limit the call depth of user-defined commands */
-int max_user_call_depth;
+unsigned int max_user_call_depth;
 
 /* Define all cmd_list_elements.  */
 
@@ -1907,13 +1907,13 @@ With no argument, show definitions of all user defined commands."), &showlist);
   add_com ("apropos", class_support, apropos_command,
 	   _("Search for commands matching a REGEXP"));
 
-  add_setshow_integer_cmd ("max-user-call-depth", no_class,
+  add_setshow_uinteger_cmd ("max-user-call-depth", no_class,
 			   &max_user_call_depth, _("\
 Set the max call depth for non-python user-defined commands."), _("\
 Show the max call depth for non-python user-defined commands."), NULL,
-			   NULL,
-			   show_max_user_call_depth,
-			   &setlist, &showlist);
+			    NULL,
+			    show_max_user_call_depth,
+			    &setlist, &showlist);
 
   add_setshow_boolean_cmd ("trace-commands", no_class, &trace_commands, _("\
 Set tracing of GDB CLI commands."), _("\
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 4b6c416..743c65f 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -314,7 +314,7 @@ execute_user_command (struct cmd_list_element *c, char *args)
   struct cleanup *old_chain;
   enum command_control_type ret;
   static int user_call_depth = 0;
-  extern int max_user_call_depth;
+  extern unsigned int max_user_call_depth;
 
   cmdlines = c->user_commands;
   if (cmdlines == 0)
diff --git a/gdb/frame.c b/gdb/frame.c
index 278269d..9ed49f6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -196,7 +196,7 @@ show_backtrace_past_entry (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int backtrace_limit = INT_MAX;
+static unsigned int backtrace_limit = UINT_MAX;
 static void
 show_backtrace_limit (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
@@ -2488,16 +2488,16 @@ the rest of the stack trace."),
 			   &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_integer_cmd ("limit", class_obscure,
-			   &backtrace_limit, _("\
+  add_setshow_uinteger_cmd ("limit", class_obscure,
+			    &backtrace_limit, _("\
 Set an upper bound on the number of backtrace levels."), _("\
 Show the upper bound on the number of backtrace levels."), _("\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited."),
-			   NULL,
-			   show_backtrace_limit,
-			   &set_backtrace_cmdlist,
-			   &show_backtrace_cmdlist);
+			    NULL,
+			    show_backtrace_limit,
+			    &set_backtrace_cmdlist,
+			    &show_backtrace_cmdlist);
 
   /* Debug this files internals.  */
   add_setshow_zuinteger_cmd ("frame", class_maintenance, &frame_debug,  _("\
diff --git a/gdb/remote.c b/gdb/remote.c
index 2db2c9d..528f374 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -834,7 +834,7 @@ static struct serial *remote_desc = NULL;
    some remote targets this variable is principly provided to
    facilitate backward compatibility.  */
 
-static int remote_address_size;
+static unsigned int remote_address_size;
 
 /* Temporary to track who currently owns the terminal.  See
    remote_terminal_* for more details.  */
@@ -6314,7 +6314,7 @@ hexnumnstr (char *buf, ULONGEST num, int width)
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  int address_size = remote_address_size;
+  unsigned int address_size = remote_address_size;
 
   /* If "remoteaddresssize" was not set, default to target address size.  */
   if (!address_size)
@@ -11461,13 +11461,13 @@ Specify a negative limit for unlimited."),
 					   breakpoints is %s.  */
 			    &remote_set_cmdlist, &remote_show_cmdlist);
 
-  add_setshow_integer_cmd ("remoteaddresssize", class_obscure,
-			   &remote_address_size, _("\
+  add_setshow_uinteger_cmd ("remoteaddresssize", class_obscure,
+			    &remote_address_size, _("\
 Set the maximum size of the address (in bits) in a memory packet."), _("\
 Show the maximum size of the address (in bits) in a memory packet."), NULL,
-			   NULL,
-			   NULL, /* FIXME: i18n: */
-			   &setlist, &showlist);
+			    NULL,
+			    NULL, /* FIXME: i18n: */
+			    &setlist, &showlist);
 
   add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
 			 "X", "binary-download", 1);
diff --git a/gdb/top.c b/gdb/top.c
index 7084116..ae2a879 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int history_size;
+static unsigned int history_size;
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1380,7 +1380,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.  */
-  int hist_len;
+  unsigned int hist_len;
 
   /* Print out some of the commands from the command history.  */
   /* First determine the length of the history list.  */
@@ -1445,15 +1445,16 @@ show_commands (char *args, int from_tty)
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  if (history_size == INT_MAX)
-    unstifle_history ();
-  else if (history_size >= 0)
-    stifle_history (history_size);
-  else
+  /* The type of parameter in stifle_history is int, so value from INT_MAX up
+     means 'unlimited'.  */
+  if (history_size > INT_MAX)
     {
-      history_size = INT_MAX;
-      error (_("History size must be non-negative"));
+      /* Ensure that 'show history size' print 'unlimited'.  */
+      history_size = UINT_MAX;
+      unstifle_history ();
     }
+  else
+    stifle_history (history_size);
 }
 
 void
@@ -1633,13 +1634,13 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_integer_cmd ("size", no_class, &history_size, _("\
+  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
 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"), _("\
-- 
1.7.7.6

Re: [PATCH 1/3] var_integer -> var_uinteger

[dje]

Yao Qi writes:
 > On 08/25/2012 01:05 AM, dje@google.com wrote:
 > >> @@ -1445,15 +1445,12 @@ show_commands (char *args, int from_tty)
 > >   >  static void
 > >   >  set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 > >   >  {
 > >   > -  if (history_size == INT_MAX)
 > >   > +  /* The type of parameter in stifle_history is int, so value from INT_MAX and
 > >   > +     up means "unlimited".  */
 > >   > +  if (history_size >= INT_MAX)
 > >   >      unstifle_history ();
 > >   > -  else if (history_size >= 0)
 > >   > -    stifle_history (history_size);
 > >   >    else
 > >   > -    {
 > >   > -      history_size = INT_MAX;
 > >   > -      error (_("History size must be non-negative"));
 > >   > -    }
 > >   > +    stifle_history (history_size);
 > >   >  }
 > > 
 > > One issue that arises is if the user sets the value to, say, INT_MAX+1 we call unstifle_history, but show "show hist size" will print INT_MAX+1 instead of "unlimited".
 > > If we go this route, perhaps any value >= INT_MAX should cause the underlying value to be set to UINT_MAX so that "show" will print "unlimited".
 > 
 > Good catch! :)  I changed this part as below,
 > 
 >   /* The type of parameter in stifle_history is int, so value from INT_MAX up
 >      means 'unlimited'.  */

so values from INT_MAX up
mean 'unlimited'.

 >   if (history_size > INT_MAX)

>= INT_MAX

 >     {
 >       /* Ensure that 'show history size' print 'unlimited'.  */

prints 'unlimited'.

 >       history_size = UINT_MAX;
 >       unstifle_history ();
 >     }
 >   else
 >     stifle_history (history_size);

Ok with those three changes.

[committed]: [PATCH 1/3] var_integer -> var_uinteger

[Yao Qi]

On 08/28/2012 06:14 AM, dje@google.com wrote:
> Ok with those three changes.

Thanks for the the review.  This is what I committed.

-- 
Yao

gdb:

2012-08-28  Yao Qi  <yao@codesourcery.com>

	* cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
	(init_cmds): Call add_setshow_uinteger_cmd for command
	'max-user-call-depth'.
	* cli/cli-script.c (execute_user_command): Add 'unsigned' to the
	declaration of 'max_user_call_depth'.
	* frame.c (backtrace_limit): Add 'unsigned'.
	(_initialize_frame): Call add_setshow_uinteger_cmd for command
	'limit'.
	* remote.c (remoteaddresssize): Add 'unsigned'.
	(remote_address_masked): Change local var 'address_size' to
	'unsigned'.
	(_initialize_remote): Call add_setshow_uinteger_cmd for
	'remoteaddresssize'.
	* 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.
---
 gdb/cli/cli-cmds.c   |   10 +++++-----
 gdb/cli/cli-script.c |    2 +-
 gdb/frame.c          |   14 +++++++-------
 gdb/remote.c         |   14 +++++++-------
 gdb/top.c            |   27 ++++++++++++++-------------
 5 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5d8a124..d3473d5 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -96,7 +96,7 @@ static void filter_sals (struct symtabs_and_lines *);
 
 \f
 /* Limit the call depth of user-defined commands */
-int max_user_call_depth;
+unsigned int max_user_call_depth;
 
 /* Define all cmd_list_elements.  */
 
@@ -1907,13 +1907,13 @@ With no argument, show definitions of all user defined commands."), &showlist);
   add_com ("apropos", class_support, apropos_command,
 	   _("Search for commands matching a REGEXP"));
 
-  add_setshow_integer_cmd ("max-user-call-depth", no_class,
+  add_setshow_uinteger_cmd ("max-user-call-depth", no_class,
 			   &max_user_call_depth, _("\
 Set the max call depth for non-python user-defined commands."), _("\
 Show the max call depth for non-python user-defined commands."), NULL,
-			   NULL,
-			   show_max_user_call_depth,
-			   &setlist, &showlist);
+			    NULL,
+			    show_max_user_call_depth,
+			    &setlist, &showlist);
 
   add_setshow_boolean_cmd ("trace-commands", no_class, &trace_commands, _("\
 Set tracing of GDB CLI commands."), _("\
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 4b6c416..743c65f 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -314,7 +314,7 @@ execute_user_command (struct cmd_list_element *c, char *args)
   struct cleanup *old_chain;
   enum command_control_type ret;
   static int user_call_depth = 0;
-  extern int max_user_call_depth;
+  extern unsigned int max_user_call_depth;
 
   cmdlines = c->user_commands;
   if (cmdlines == 0)
diff --git a/gdb/frame.c b/gdb/frame.c
index 278269d..9ed49f6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -196,7 +196,7 @@ show_backtrace_past_entry (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int backtrace_limit = INT_MAX;
+static unsigned int backtrace_limit = UINT_MAX;
 static void
 show_backtrace_limit (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
@@ -2488,16 +2488,16 @@ the rest of the stack trace."),
 			   &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
-  add_setshow_integer_cmd ("limit", class_obscure,
-			   &backtrace_limit, _("\
+  add_setshow_uinteger_cmd ("limit", class_obscure,
+			    &backtrace_limit, _("\
 Set an upper bound on the number of backtrace levels."), _("\
 Show the upper bound on the number of backtrace levels."), _("\
 No more than the specified number of frames can be displayed or examined.\n\
 Zero is unlimited."),
-			   NULL,
-			   show_backtrace_limit,
-			   &set_backtrace_cmdlist,
-			   &show_backtrace_cmdlist);
+			    NULL,
+			    show_backtrace_limit,
+			    &set_backtrace_cmdlist,
+			    &show_backtrace_cmdlist);
 
   /* Debug this files internals.  */
   add_setshow_zuinteger_cmd ("frame", class_maintenance, &frame_debug,  _("\
diff --git a/gdb/remote.c b/gdb/remote.c
index 2db2c9d..528f374 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -834,7 +834,7 @@ static struct serial *remote_desc = NULL;
    some remote targets this variable is principly provided to
    facilitate backward compatibility.  */
 
-static int remote_address_size;
+static unsigned int remote_address_size;
 
 /* Temporary to track who currently owns the terminal.  See
    remote_terminal_* for more details.  */
@@ -6314,7 +6314,7 @@ hexnumnstr (char *buf, ULONGEST num, int width)
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  int address_size = remote_address_size;
+  unsigned int address_size = remote_address_size;
 
   /* If "remoteaddresssize" was not set, default to target address size.  */
   if (!address_size)
@@ -11461,13 +11461,13 @@ Specify a negative limit for unlimited."),
 					   breakpoints is %s.  */
 			    &remote_set_cmdlist, &remote_show_cmdlist);
 
-  add_setshow_integer_cmd ("remoteaddresssize", class_obscure,
-			   &remote_address_size, _("\
+  add_setshow_uinteger_cmd ("remoteaddresssize", class_obscure,
+			    &remote_address_size, _("\
 Set the maximum size of the address (in bits) in a memory packet."), _("\
 Show the maximum size of the address (in bits) in a memory packet."), NULL,
-			   NULL,
-			   NULL, /* FIXME: i18n: */
-			   &setlist, &showlist);
+			    NULL,
+			    NULL, /* FIXME: i18n: */
+			    &setlist, &showlist);
 
   add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
 			 "X", "binary-download", 1);
diff --git a/gdb/top.c b/gdb/top.c
index 7084116..99fd5ff 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-static int history_size;
+static unsigned int history_size;
 static void
 show_history_size (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
@@ -1380,7 +1380,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.  */
-  int hist_len;
+  unsigned int hist_len;
 
   /* Print out some of the commands from the command history.  */
   /* First determine the length of the history list.  */
@@ -1445,15 +1445,16 @@ show_commands (char *args, int from_tty)
 static void
 set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
 {
-  if (history_size == INT_MAX)
-    unstifle_history ();
-  else if (history_size >= 0)
-    stifle_history (history_size);
-  else
+  /* The type of parameter in stifle_history is int, so values from INT_MAX up
+     mean 'unlimited'.  */
+  if (history_size >= INT_MAX)
     {
-      history_size = INT_MAX;
-      error (_("History size must be non-negative"));
+      /* Ensure that 'show history size' prints 'unlimited'.  */
+      history_size = UINT_MAX;
+      unstifle_history ();
     }
+  else
+    stifle_history (history_size);
 }
 
 void
@@ -1633,13 +1634,13 @@ Without an argument, saving is enabled."),
 			   show_write_history_p,
 			   &sethistlist, &showhistlist);
 
-  add_setshow_integer_cmd ("size", no_class, &history_size, _("\
+  add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
 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"), _("\
-- 
1.7.7.6

Re: [PATCH 2/3] var_integer -> var_zuinteger_unlimited

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> With previous patch applied, var_integer are still used in two palces,
Yao> 'listsize' and 'remotetimeout'.  They are similar, 0 is allowed, and -1
Yao> means unlimited.  Existing 'enum var_types' don't cover this kind of
Yao> command, so I create a new one 'var_zuinteger_unlimited'.

This look good to me.  Ok.

Tom

Re: [PATCH 3/3] comments update

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-08-13  Yao Qi  <yao@codesourcery.com>
Yao> 	* cli/cli-decode.c: Update comment to add_setshow_integer_cmd.
Yao> 	* command.h (enum var_types): Update comment to var_integer.

Ok.

Tom

[committed]: [PATCH 2/3] var_integer -> var_zuinteger_unlimited

[Yao Qi]

On 09/15/2012 02:12 AM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>
> Yao> With previous patch applied, var_integer are still used in two palces,
> Yao> 'listsize' and 'remotetimeout'.  They are similar, 0 is allowed, and -1
> Yao> means unlimited.  Existing 'enum var_types' don't cover this kind of
> Yao> command, so I create a new one 'var_zuinteger_unlimited'.
>
> This look good to me.  Ok.
>
> Tom
>

Tom, thanks for the review.  Committed.

-- 
Yao

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

[Pedro Alves]

> 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

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

[Yao Qi]

On 03/26/2013 03:08 AM, Pedro Alves wrote:
> 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.
> 

Pedro,
I understand that your point of choosing signed command is readline
interface is signed.  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.

> 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 potential overflow needs to be fixed.  I prefer to change "offset"
to unsigned, and check the range when passing "history_base + offset"
to history_get.  Change everything to signed still has the risk of
overflow in "history_get (history_base + offset)".

> 
> 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.

Your patch changes the command back to signed, and at the same time,
keep the command behaves like unsigned.  This makes code a little bit
hard, IMO.  Isn't better to keep the command unsigned (from the
user's perspective) and take care of potential signed overflow
(to fit the internal readline interface)?  How about the patch
below?
 
-- 
Yao (齐尧)

gdb:

2013-03-26  Yao Qi  <yao@codesourcery.com>

	* top.c (show_commands): Change local var 'offset' to
	'unsigned int'.  Check the overflow of 'history_base + offset'
	before pass it to history_get.
---
 gdb/top.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 7905b51..1cedfe9 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1373,7 +1373,7 @@ void
 show_commands (char *args, int from_tty)
 {
   /* Index for history commands.  Relative to history_base.  */
-  int offset;
+  unsigned int offset;
 
   /* Number of the history entry which we are planning to display next.
      Relative to history_base.  */
@@ -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;
-- 
1.7.7.6

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

[Pedro Alves]

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

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

[Yao Qi]

On 03/27/2013 12:16 AM, Pedro Alves wrote:
> 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.
>

Indeed.

@@ -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;

Probably, we can replace local variable 'hist_len' with 'history_length' 
in function show_commands.  Anyway, the patch looks pretty good.

-- 
Yao (齐尧)

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

[Pedro Alves]

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."),