[RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Mark Kettenis]

While looking into tdep/2074, I noticed how crappy the code is that
handles var_integer/var_uinteger/var_zinteger.  I mean, var_zinteger
is supposed to be an 'int' but like Unsigned Integer?  var_integer is
supposed to be a (signed) integer, but when we set it, we use an
`unsigned int'.  The patch below cleans things up, but the source.c
change is debatable.

Unfortunately it also reveals a problem.  I now get:

FAIL: gdb.base/list.exp: show listsize unlimited #6
FAIL: gdb.base/list.exp: show listsize unlimited #7

Analyzing these tests shows that people have been truly confused,
because part of the test considers all values <= 0 to mean unlimited,
but other parts seem to consider 0 to mean that printing should be
suppressed.  Unfortunately the test that checks this is broken and
will always pass.  That may be why we didn't notice the incosistency
of the test before :(.

To me, the intended behaviour seems to be that 0 would suppress output
(0 lines printed), and -1 would print all lines (unlimited lines will
be printed).  But that doesn't map into any of the
var_integer/var_uinteger/var_zinteger cases.

Does anyone see a way out of this mess?  It seems that -1 meaning
unlimited never really worked.  So perhaps we should just drop those
tests and go with my patch.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* command.h (enum var_types): Reorder, adjust comments.
	* cli/cli-setshow.c (do_setshow_command): Handle var_integer,
	var_uinteger and var_zinteger according to their documented
	behaviour.
	* source.c (_initialize_source): Use add_setshow_zinteger_cmd for
	listsize.

Index: command.h
===================================================================
RCS file: /cvs/src/src/gdb/command.h,v
retrieving revision 1.54
diff -u -p -r1.54 command.h
--- command.h 17 Dec 2005 22:33:59 -0000 1.54
+++ command.h 31 Jan 2006 11:10:36 -0000
@@ -1,7 +1,7 @@
 /* Header file for command-reading library command.c.
 
    Copyright (C) 1986, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1999,
-   2000, 2002, 2004 Free Software Foundation, Inc.
+   2000, 2002, 2004, 2006 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -54,24 +54,26 @@ cmd_types;
 /* Types of "set" or "show" command.  */
 typedef enum var_types
   {
-    /* "on" or "off".  *VAR is an integer which is nonzero for on,
-       zero for off.  */
+    /* "on" or "off".  *VAR is an int which is nonzero for on, zero
+       for off.  */
     var_boolean,
 
     /* "on" / "true" / "enable" or "off" / "false" / "disable" or
-       "auto.  *VAR is an ``enum auto_boolean''.  NOTE: In general a
+       "auto.  *VAR is an `enum auto_boolean'.  NOTE: In general a
        custom show command will need to be implemented - one that for
        "auto" prints both the "auto" and the current auto-selected
        value. */
     var_auto_boolean,
 
-    /* Unsigned Integer.  *VAR is an unsigned int.  The user can type 0
-       to mean "unlimited", which is stored in *VAR as UINT_MAX.  */
-    var_uinteger,
-
-    /* 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.  */
+    /* Signed integer.  *VAR is an int.  The user can type 0 to mean
+       "unlimited", which is stored in *VAR as INT_MAX.  */
     var_integer,
+    /* Unsigned integer.  *VAR is an unsigned int.  The user can type
+       0 to mean "unlimited", which is stored in *VAR as UINT_MAX.  */
+    var_uinteger,
+    /* Zeroable integer.  *VAR is an int.  Like var_integer except
+       that zero really means zero.  */
+    var_zinteger,
 
     /* String which the user enters with escapes (e.g. the user types \n and
        it is a real newline in the stored string).
@@ -80,17 +82,14 @@ typedef enum var_types
     /* String which stores what the user types verbatim.
        *VAR is a malloc'd string, or NULL if the string is empty.  */
     var_string_noescape,
-    /* String which stores a filename.  (*VAR) is a malloc'd string,
+    /* String which stores a filename.  *VAR is a malloc'd string.  */
+    var_filename,
+    /* String which stores a filename.  *VAR is a malloc'd string,
        or "" if the string was empty.  */
     var_optional_filename,
-    /* String which stores a filename.  (*VAR) is a malloc'd
-       string.  */
-    var_filename,
-    /* ZeroableInteger.  *VAR is an int.  Like Unsigned Integer except
-       that zero really means zero.  */
-    var_zinteger,
-    /* Enumerated type.  Can only have one of the specified values.  *VAR is a
-       char pointer to the name of the element that we find.  */
+
+    /* Enumerated type.  Can only have one of the specified values.  *VAR
+       is a char pointer to the name of the element that we find.  */
     var_enum
   }
 var_types;
Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.72
diff -u -p -r1.72 source.c
--- source.c 15 Jan 2006 19:09:30 -0000 1.72
+++ source.c 31 Jan 2006 11:10:37 -0000
@@ -1638,7 +1638,7 @@ The matching line number is also stored 
       add_com_alias ("?", "reverse-search", class_files, 0);
     }
 
-  add_setshow_integer_cmd ("listsize", class_support, &lines_to_list, _("\
+  add_setshow_zinteger_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,
Index: cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.27
diff -u -p -r1.27 cli-setshow.c
--- cli/cli-setshow.c 17 Dec 2005 22:40:17 -0000 1.27
+++ cli/cli-setshow.c 31 Jan 2006 11:10:37 -0000
@@ -200,31 +200,53 @@ do_setshow_command (char *arg, int from_
 	case var_auto_boolean:
 	  *(enum auto_boolean *) c->var = parse_auto_binary_operation (arg);
 	  break;
-	case var_uinteger:
-	  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)
-	    *(unsigned int *) c->var = UINT_MAX;
-	  break;
 	case var_integer:
 	  {
-	    unsigned int val;
+	    LONGEST val;
+
 	    if (arg == NULL)
 	      error_no_arg (_("integer to set it to."));
 	    val = parse_and_eval_long (arg);
+	    if (val < INT_MIN || val > INT_MAX)
+	      error (_("integer %s out of range"),
+		     int_string (val, 10, 1, 0, 0));
+
 	    if (val == 0)
 	      *(int *) c->var = INT_MAX;
-	    else if (val >= INT_MAX)
-	      error (_("integer %u out of range"), val);
 	    else
 	      *(int *) c->var = val;
-	    break;
 	  }
+	  break;
+	case var_uinteger:
+	  {
+	    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"),
+		     int_string (val, 10, 1, 0, 0));
+
+	    if (val == 0)
+	      *(unsigned int *) c->var = UINT_MAX;
+	    else
+	      *(unsigned 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);
+	  {
+	    LONGEST val;
+
+	    if (arg == NULL)
+	      error_no_arg (_("integer to set it to."));
+	    val = parse_and_eval_long (arg);
+	    if (val < INT_MIN || val > INT_MAX)
+	      error (_("integer %s out of range"),
+		     int_string (val, 10, 1, 0, 0));
+
+	    *(int *) c->var = val;
+	  }
 	  break;
 	case var_enum:
 	  {
@@ -332,25 +354,21 @@ do_setshow_command (char *arg, int from_
 	      break;
 	    }
 	  break;
-	case var_uinteger:
-	  if (*(unsigned int *) c->var == UINT_MAX)
-	    {
-	      fputs_filtered ("unlimited", stb->stream);
-	      break;
-	    }
-	  /* else fall through */
-	case var_zinteger:
-	  fprintf_filtered (stb->stream, "%u", *(unsigned int *) c->var);
-	  break;
 	case var_integer:
 	  if (*(int *) c->var == INT_MAX)
-	    {
-	      fputs_filtered ("unlimited", stb->stream);
-	    }
+	    fputs_filtered ("unlimited", stb->stream);
 	  else
 	    fprintf_filtered (stb->stream, "%d", *(int *) c->var);
 	  break;
-
+	case var_uinteger:
+	  if (*(unsigned int *) c->var == UINT_MAX)
+	    fputs_filtered ("unlimited", stb->stream);
+	  else
+	    fprintf_filtered (stb->stream, "%u", *(unsigned int *) c->var);
+	  break;
+	case var_zinteger:
+	  fprintf_filtered (stb->stream, "%d", *(int *) c->var);
+	  break;
 	default:
 	  error (_("gdb internal error: bad var_type in do_setshow_command"));
 	}

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Jim Blandy]

It seems to me there ought to be something like var_limit, which would
accept non-negative numbers or the word "unlimited", and store the
result as an unsigned integer, with UINT_MAX meaning "unlimited".

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Mark Kettenis]

> Date: Tue, 31 Jan 2006 21:53:13 -0800
> From: Jim Blandy <jimb@red-bean.com>
> 
> It seems to me there ought to be something like var_limit, which would
> accept non-negative numbers or the word "unlimited", and store the
> result as an unsigned integer, with UINT_MAX meaning "unlimited".

Actually, it'd make sense if the existing
var_integer/var_uinteger/var_zinteger would accept "unlimited".
That'd make your var_limit unnecessary.

Does anyone see any problems with that?

Mark

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Jim Blandy]

On 2/1/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> Actually, it'd make sense if the existing
> var_integer/var_uinteger/var_zinteger would accept "unlimited".
> That'd make your var_limit unnecessary.
>
> Does anyone see any problems with that?

Well, I presume that sometimes (often) those are used for limits of
something, and sometimes they're genuine integers.  Surely there's
something in GDB where a negative value would make sense.  I don't
like the idea of accepting "unlimited" for a quantity that isn't a
limit on anything.

Then, of course, there's aix-thread.c which is using zinteger for a boolean.

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Mark Kettenis]

> X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on 
> 	elgar.sibelius.xs4all.nl
> X-Spam-Level: 
> X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=no 
> 	version=3.1.0
> DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
>         s=beta; d=gmail.com;
>         h=received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
>         b=TFTIQB+EK02V1vfXwI4QJr/gwBrh3UD5JTREmxEUO/A7bevYVtY02UcXlTjdbxMdWVK0UFIL+7lGFhq9OI4I4ZDiTkjxogv8dHMT4YB0XjPaJwFosrXjv5aSAOTv7blG57qzR2FfqT+q/8wjS018LVkizr+i29iXHZipUDHhXDc=
> Date: Wed, 1 Feb 2006 10:49:22 -0800
> From: Jim Blandy <jimb@red-bean.com>
> Sender: jimblandy@gmail.com
> Cc: gdb-patches@sourceware.org
> Content-Disposition: inline
> X-XS4ALL-DNSBL-Checked: mxdrop24.xs4all.nl checked 64.233.162.193 against DNS blacklists
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: 0 () 
> X-XS4ALL-Spam: NO
> Envelope-To: mark.kettenis@xs4all.nl
> X-MIME-Autoconverted: from quoted-printable to 8bit by mxdrop24.xs4all.nl id k11InHwe077021
> X-UIDL: 1138819766._smtp.mxdrop24.77065,S=2619
> 
> On 2/1/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > Actually, it'd make sense if the existing
> > var_integer/var_uinteger/var_zinteger would accept "unlimited".
> > That'd make your var_limit unnecessary.
> >
> > Does anyone see any problems with that?
> 
> Well, I presume that sometimes (often) those are used for limits of
> something, and sometimes they're genuine integers.  Surely there's
> something in GDB where a negative value would make sense.  I don't
> like the idea of accepting "unlimited" for a quantity that isn't a
> limit on anything.

Well, the current code already prints "unlimited" for
var_integer/var_uinteger.  I'd say it is only logical that they can be
set to "unlimited" too.

Guess your var_zinteger is your "genuine" integer; we probably
shouldn't accept "unlimited" for those.

> Then, of course, there's aix-thread.c which is using zinteger for a boolean.

Of course.  That's the proper type for boolean expressions ;-).

Mark

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Jim Blandy]

On 2/1/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > Then, of course, there's aix-thread.c which is using zinteger for a boolean.
>
> Of course.  That's the proper type for boolean expressions ;-).

Get back in your cave.  :)

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Daniel Jacobowitz]

On Wed, Feb 01, 2006 at 10:49:22AM -0800, Jim Blandy wrote:
> On 2/1/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > Actually, it'd make sense if the existing
> > var_integer/var_uinteger/var_zinteger would accept "unlimited".
> > That'd make your var_limit unnecessary.
> >
> > Does anyone see any problems with that?
> 
> Well, I presume that sometimes (often) those are used for limits of
> something, and sometimes they're genuine integers.  Surely there's
> something in GDB where a negative value would make sense.  I don't
> like the idea of accepting "unlimited" for a quantity that isn't a
> limit on anything.
> 
> Then, of course, there's aix-thread.c which is using zinteger for a boolean.

There's lots of (mis- ?) uses of all the var_ types all over GDB.  I
think that (A) we ought to clean them all up, and (B) it's going to
require checking them all to do it.  Definitely some of the current
uses don't make sense - for instance:

(gdb) show remote hardware-breakpoint-limit 
The maximum number of target hardware breakpoints is 4294967295.

A bunch of others ought to be booleans.  A bunch of others are actual
integer values - sometimes signed would make sense, sometimes it
wouldn't, e.g. ser-go32.c.

I'd like to be able to set the ones that are limits to "unlimited".
Caveats are preserving the existing "set to zero" behavior where it
makes sense to do so, updating the manual, and not being able to set
non-limits to unlimited.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Clean up var_integer/var_uinteger/var_zinteger mess

[Joel Brobecker]

> Then, of course, there's aix-thread.c which is using zinteger for a
> boolean.

Outch! I'll take care of that as soon as I have a minute (and some
CPU cycles :-).

-- 
Joel