PR 8507 - Remote watchpoint limit really large

PR 8507 - Remote watchpoint limit really large

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

PR 8507 is about this command:

 (gdb) help show remote hardware-watchpoint-limit
 Show the maximum number of target hardware watchpoints.
 Specify a negative limit for unlimited.

And the fact that a negative value (default is -1), shows
through as a large positive value, which is somewhat confusing:

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

Notice that the "show" help explicitly mentioned "Specify a negative
limit for unlimited.".

This patch fixes it to show:

 (gdb) set remote hardware-watchpoint-limit -1
 (gdb) show remote hardware-watchpoint-limit
 The maximum number of target hardware watchpoints is unlimited.

There's no current enum var_types for this integer usage, where
0 really means zero, and negatives mean unlimited, so I've added one.

"set/show remote hardware-breakpoint-limit" has the same issue, so it
gets the same fix.

Comments?

-- 
Pedro Alves

[-- Attachment #2: pr8507.diff --]
[-- Type: text/x-diff, Size: 7129 bytes --]

2008-12-29  Pedro Alves  <pedro@codesourcery.com>

	PR gdb/8507:
	* command.h (enum var_types): Add var_zinteger_nu.
	(add_setshow_zinteger_nu_cmd): Declare.
	* cli/cli-decode.c (add_setshow_zinteger_nu_cmd): New.
	* cli/cli-setshow.c (do_setshow_command): Handle it.
	* remote.c (_initialize_remote): Make the
	hardware-watchpoint-limit and hardware-breakpoint-limit commands
	zinteger_nu commands.

---
 gdb/cli/cli-decode.c  |   21 +++++++++++++++++++++
 gdb/cli/cli-setshow.c |   16 ++++++++++++++++
 gdb/command.h         |   27 ++++++++++++++++++++++++---
 gdb/remote.c          |   16 ++++++++--------
 4 files changed, 69 insertions(+), 11 deletions(-)

Index: src/gdb/command.h
===================================================================
--- src.orig/gdb/command.h	2008-12-29 04:42:35.000000000 +0000
+++ src/gdb/command.h	2008-12-29 05:08:03.000000000 +0000
@@ -71,22 +71,32 @@ typedef enum var_types
        to mean "unlimited", which is stored in *VAR as INT_MAX.  */
     var_integer,
 
+    /* ZeroableInteger.  *VAR is an int.  Like Unsigned Integer except
+       that zero really means zero.  */
+    var_zinteger,
+
+    /* ZeroableIntegerNegativeUnlimited.  *VAR is an int.  Zero really
+       means zero, and negative values mean "unlimited", which is
+       stored as UINT_MAX.  */
+    var_zinteger_nu,
+
     /* String which the user enters with escapes (e.g. the user types \n and
        it is a real newline in the stored string).
        *VAR is a malloc'd string, or NULL if the string is empty.  */
     var_string,
+
     /* 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,
        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.  */
     var_enum
@@ -332,6 +342,17 @@ extern void add_setshow_zinteger_cmd (ch
 				      struct cmd_list_element **set_list,
 				      struct cmd_list_element **show_list);
 
+extern void add_setshow_zinteger_nu_cmd (char *name,
+					 enum command_class class,
+					 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 *);
Index: src/gdb/cli/cli-decode.c
===================================================================
--- src.orig/gdb/cli/cli-decode.c	2008-12-29 04:42:48.000000000 +0000
+++ src/gdb/cli/cli-decode.c	2008-12-29 04:54:51.000000000 +0000
@@ -639,6 +639,27 @@ add_setshow_zinteger_cmd (char *name, en
 			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
+   value.  SET_DOC and SHOW_DOC are the documentation strings.  */
+void
+add_setshow_zinteger_nu_cmd (char *name, enum command_class class,
+			     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_zinteger_nu, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			NULL, NULL);
+}
+
 /* Remove the command named NAME from the command list.  Return the
    list commands which were aliased to the deleted command.  If the
    command had no aliases, return NULL.  The various *HOOKs are set to
Index: src/gdb/cli/cli-setshow.c
===================================================================
--- src.orig/gdb/cli/cli-setshow.c	2008-12-29 04:42:48.000000000 +0000
+++ src/gdb/cli/cli-setshow.c	2008-12-29 05:19:41.000000000 +0000
@@ -213,6 +213,21 @@ do_setshow_command (char *arg, int from_
 	  if (*(unsigned int *) c->var == 0)
 	    *(unsigned int *) c->var = UINT_MAX;
 	  break;
+	case var_zinteger_nu:
+	  {
+	    LONGEST val;
+
+	    if (arg == NULL)
+	      error_no_arg (_("integer to set it to."));
+	    val = parse_and_eval_long (arg);
+	    if (val < 0)
+	      *(int *) c->var = UINT_MAX;
+	    else if (val > INT_MAX)
+	      error (_("integer %s out of range"), plongest (val));
+	    else
+	      *(int *) c->var = val;
+	    break;
+	  }
 	case var_integer:
 	  {
 	    unsigned int val;
@@ -345,6 +360,7 @@ do_setshow_command (char *arg, int from_
 	    }
 	  break;
 	case var_uinteger:
+	case var_zinteger_nu:
 	  if (*(unsigned int *) c->var == UINT_MAX)
 	    {
 	      fputs_filtered ("unlimited", stb->stream);
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-12-29 03:43:20.000000000 +0000
+++ src/gdb/remote.c	2008-12-29 05:10:34.000000000 +0000
@@ -8979,20 +8979,20 @@ further restriction and ``limit'' to ena
 	   _("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_zinteger_nu_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-breakpoint-limit", no_class,
-			    &remote_hw_breakpoint_limit, _("\
+			       NULL, NULL, /* FIXME: i18n: The maximum number of target hardware watchpoints is %s.  */
+			       &remote_set_cmdlist, &remote_show_cmdlist);
+  add_setshow_zinteger_nu_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, _("\

Re: PR 8507 - Remote watchpoint limit really large

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Mon, 29 Dec 2008 05:25:11 +0000
> 
> PR 8507 is about this command:
> 
>  (gdb) help show remote hardware-watchpoint-limit
>  Show the maximum number of target hardware watchpoints.
>  Specify a negative limit for unlimited.
> 
> And the fact that a negative value (default is -1), shows
> through as a large positive value, which is somewhat confusing:
> 
>  (gdb) show remote hardware-watchpoint-limit 
>  The maximum number of target hardware watchpoints is 4294967295.
> 
> Notice that the "show" help explicitly mentioned "Specify a negative
> limit for unlimited.".
> 
> This patch fixes it to show:
> 
>  (gdb) set remote hardware-watchpoint-limit -1
>  (gdb) show remote hardware-watchpoint-limit
>  The maximum number of target hardware watchpoints is unlimited.
> 
> There's no current enum var_types for this integer usage, where
> 0 really means zero, and negatives mean unlimited, so I've added one.

Hmm, I'm not sure I like the name.  How about var_zuinteger, for
zeroable unsigned Integer?

Interesting enough, the existing usage of var_zinteger in alpha-tdep.c
and mips-tdep.c looks like it should really be var_zuinteger too.  I
bet there are more cases.

I'm also not sure your implementation will actually work.  Especially
the statement:

> +	      *(int *) c->var = UINT_MAX;

looks very suspicious to me.  I suspect you are having problems
converting the variables set by this command from 'int' to 'unsigned
int'.  I actually think it would make sense to treat var_uinteger and
var_zuinteger variables as 'int' ans set them to INT_MAX to mean
unlimited.  I don't think the loss of range is a big loss.  For all
practical purposes INT_MAX is just as much infinty as UINT_MAX.

This would probably fix a few bugs as well, since eventually you're
going to compare these to signed integers anyway, and having your
"unsigned" integers in the range [0, INT_MAX] prevents a lot of
accidents from happening there.  (Do you fully understand the C rules
for integer type promotion?).

Mark

Re: PR 8507 - Remote watchpoint limit really large

[Pedro Alves]

On Monday 29 December 2008 12:22:56, Mark Kettenis wrote:
> > From: Pedro Alves <pedro@codesourcery.com>

> > This patch fixes it to show:
> > 
> >  (gdb) set remote hardware-watchpoint-limit -1
> >  (gdb) show remote hardware-watchpoint-limit
> >  The maximum number of target hardware watchpoints is unlimited.
> > 

> > There's no current enum var_types for this integer usage, where
> > 0 really means zero, and negatives mean unlimited, so I've added one.
> 
> Hmm, I'm not sure I like the name.  

I didn't like it either, but it seemed better than the var_nuzinteger
I had started with.

> How about var_zuinteger, for zeroable unsigned Integer?

Hmm, I was trying to convey the idea that negatives mean unlimited,
as the watchpoint/breakpoint limits command explicitly mentions
that.  But, it's just a name, I'm not married to any.

> Interesting enough, the existing usage of var_zinteger in alpha-tdep.c
> and mips-tdep.c looks like it should really be var_zuinteger too.  I
> bet there are more cases.

Yeah, I had noticed those, but I thought I'd propose changing them
after fixing this particular remote command.  Notice that one of those
settings gets broken in a multi-arch GDB that includes both mips and
alpha support, as both -tdep.c files register the same command, but
handle different static variables.  A single setting should be
common-ized, or both settings should be prefixed with mips/alpha
respectively.

> I'm also not sure your implementation will actually work.  Especially
> the statement:
> 
> > +	      *(int *) c->var = UINT_MAX;
> 
> looks very suspicious to me.  I suspect you are having problems
> converting the variables set by this command from 'int' to 'unsigned
> int'.  

Outch.  Talk about dumbness.  Well it worked here, but it's undefined.
I was thinking of setting it to -1 for "unlimited", but for some
reason, ended up not doing it, as it was less typing.  Let's forget I
suggested that.  :-)

I just grepped through the sources for 'w_zinteger', 'w_integer' and
'w_uinteger', to catch add_setshow_XXX, and collected the following
table of all integer settings, excluding the "set debug ..."-like
variables.  I was surprised, as I thought there were many more.

Note that:

 integer does 0 -> INT_MAX conversion on a "set".
 uinteger does 0 -> UINT_MAX conversion on a "set".
 zinteger does nothing.

 uu - some smallish integer
 UU - some integer

 (Best viewed without wrapping enabled)

|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| set/show command          | var type | ideal valid range          |  set unlimited? | comments?                                                                 |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| backtrace limit           | integer  | ]0, +uu]                   |               0 |                                                                           |
| remoteaddresssize         | integer  | ]0, bitsof(ULONGEST)       |                 | 0 means use target arch address size, but prints                          |
|                           |          |                            |                 | as "unlimited".  What's an unlimited address BTW?                         |
|                           |          |                            |                 | There's is no way to go back to 0 (use target arch size),                 |
|                           |          |                            |                 | as "set remoteaddresssize 0" sets the internal variable to INT_MAX        |
|                           |          |                            |                 |                                                                           |
| listsize                  | integer  | ]0, +inf]                  |               0 |                                                                           |
| remotetimeout             | integer  | -1, [0, +UU]               |              -1 | -1 means forever (unlimited), 0 is like poll (though                      |
|                           |          |                            |                 | that's unuseful) and GDB lets you say "set ... 0", but                    |
|                           |          |                            |                 | internally sets to INT_MAX, and displays as "unlimited", which is wrong   |
| max-user-call-depth       | integer  | [smallish integer, +inf]   |               0 |                                                                           |
|                           |          |                            |                 |                                                                           |
| historysize               | integer  | [0, +inf]                  | < 0, 0, INT_MAX | setting to < 0 automatically sets to                                      |
|                           |          |                            |                 | INT_MAX (unlimited).  There's no way to specify no-history,               |
|                           |          |                            |                 | as useful as that would be.  This could be a candidate                    |
|                           |          |                            |                 | for the new proposed type.                                                |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| input-radix               | uinteger | [2, +inf]                  |                 | OMG, contrary to claims, "set intput-radix 0" *is* accepted,              |
|                           |          |                            |                 | because of 0->UINT_MAX.  Missed that yesterday somehow, when I fixed "1". |
|                           |          |                            |                 |                                                                           |
| output-radix              | uinteger |                            |                 | OMG, set output-radix 0 is accepted, due to 0->UINT_MAX.                  |
|                           |          |                            |                 | We should only accept 16, 10, 8.                                          |
|                           |          |                            |                 |                                                                           |
| max-symbolic-offset       | uinteger | [0, +inf]                  |               0 | No way to set 0, as in no way to have no symbolic offsets?                |
| width                     | uinteger | [0, +inf]                  |               0 | no wrap == unlimited                                                      |
| height                    | uinteger | [0, +inf]                  |               0 | 0 means "no height"/disable pagination, which                             |
|                           |          |                            |                 | is a kind of "unlimited".                                                 |
| elements                  | uinteger |                            |               0 | help: "set print elements 0 causes there to be no limit."                 |
| repeats                   | uinteger |                            |                 | help: "set print repeats 0 causes all                                     |
|                           |          |                            |                 | elements to be individually printed.", yet GDB displays 0 == "unlimited", |
|                           |          |                            |                 | does this make sense?                                                     |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|
| heuristic-fence-post      | zinteger | [0, some smallish integer] |                 | Comments mention that it would be ideal if 0 and "unlimited"              |
|                           |          |                            |                 | were both valid setttings.                                                |
|                           |          |                            |                 |                                                                           |
| remotebaud                | zinteger |                            |                 | -1 is the default, (uint)-1 will show up in the UI...                     |
| maint dwarf max-cache-age | zinteger |                            |                 | 0 means disabled cache.  There are no negative ages, yet we allow it.     |
| stop-on-solib-events      | zinteger | [0, 1]                     |                 | should be boolean, or better yet, a "catchpoint" instead                  |
| watchdog                  | zinteger | [0, +inf]                  |               0 | 0 means no watchdog, or, "unlimited"                                      |
|                           |          |                            |                 | timeout, depending on how you think about it                              |
|                           |          |                            |                 |                                                                           |
| hardware-watchpoint-limit | zinteger | [0, +inf]                  |             < 0 | 0 is "no support", < 0 unlimited                                          |
| hardware-breakpoint-limit | zinteger | [0, +inf]                  |             < 0 | 0 is "no support", < 0 unlimited                                          |
| com1base, com1irq, com... | zinteger | [0, +inf]                  |                 |                                                                           |
| annotate                  | zinteger | [0, +uu]                   |                 |                                                                           |
|---------------------------+----------+----------------------------+-----------------+---------------------------------------------------------------------------|

There are several interesting issues here:

 - A few integer or uinteger variables translate 0->(U)INT_MAX, and
   show "unlimited" when it doesn't make sense to.

 - Most commands don't want negative values; those that do, want
   it for a special meaning.

 - zinteger variables hold "int", but print as "unsigned int".

   Just found PR8185, which mentions this.

 - There are three zinteger commands that want distinct 0, and
   "unlimited":

    heuristic-fence-post, hardware-watchpoint-limit, hardware-breakpoint-limit

   Two actually specify < 0 as unlimited:

    hardware-watchpoint-limit, hardware-breakpoint-limit

 - There several buggy cases.

 - Most commands ideally would validate set input range, but they
   don't.

> I actually think it would make sense to treat var_uinteger and 
> var_zuinteger variables as 'int' ans set them to INT_MAX to mean
> unlimited.  I don't think the loss of range is a big loss.  For all
> practical purposes INT_MAX is just as much infinty as UINT_MAX.

I see that most (all?) current commands don't care about > INT_MAX,
though, there are cases where it doesn't make seem hard to imagine
wanting it, say for commands similar to "com1base".  It doesn't seem
hard to imagine wanting an unsigned variable without "unlimited".

Here's a draft of the types I think could handle most cases better:

| type       | internal type | print as     | how to set unlimited | internal conversion |
|------------+---------------+--------------+----------------------+---------------------|
| integer    | int           | int          | 0                    | 0 -> INT_MAX        |
| uinteger   | unsigned int  | unsigned int | 0                    | 0 -> UINT_MAX       |
| zinteger   | int           | int          | -                    | -                   |
| zuinteger  | unsigned int  | unsigned int | -                    | -                   |
|------------+---------------+--------------+----------------------+---------------------|
| nuzinteger | int           | int          | < 0                  | <0 -> -1            |

For others not covered, where we want to print something not numeric
and != "unlimited", we'd have to handle this FIXME in cli-setshow.c:

      /* FIXME: cagney/2005-02-10: Need to split this in half: code to
	 convert the value into a string (esentially the above); and
	 code to print the value out.  For the latter there should be
	 MI and CLI specific versions.  */

Alternatively:

| type       | internal type | print as     | how to set unlimited | internal conversion |
|------------+---------------+--------------+----------------------+---------------------|
| integer    | int           | int          | 0                    | 0 -> INT_MAX        |
| uinteger   | unsigned int  | unsigned int | 0                    | 0 -> UINT_MAX       |
| zinteger   | int           | int          | <0                   | <0 -> INT_MAX       |
| zuinteger  | unsigned int  | unsigned int | -                    | -                   |
| zninteger  | int           | int          | -                    | -                   |

(It's likelly that zninteger wouldn't be needed)

I think you were proposing something like:

| type      | internal type | print as | how to set unlimited | internal conversion |
|-----------+---------------+----------+----------------------+---------------------|
| integer   | int           | int      |                    0 | 0 -> INT_MAX        |
| uinteger  | int           | int      |                   <0 | <0 -> INT_MAX       |
| zinteger  | int           | int      |              INT_MAX | -                   |
| zuinteger | int           | int      |                   <0 | <0 -> INT_MAX       |

I'm not sure I understood what you were proposing correctly.  I'm also not sure if this
handles all cases easily or at all; several commands would have to be adjusted.

Something else I just noticed: in integer or uinteger variables, the 0
-> (U)INT_MAX conversion for "unlimited" is only done on a "set",
which has this effect on commands that default to 0:

 (gdb) show remoteaddresssize
 The maximum size of the address (in bits) in a memory packet is 0.
 (gdb) set remoteaddresssize 0
 (gdb) show remoteaddresssize
 The maximum size of the address (in bits) in a memory packet is unlimited.

 What's an "unlimited" address size again?  :-)


Tricky bits here ...

-- 
Pedro Alves

Re: PR 8507 - Remote watchpoint limit really large

[Daniel Jacobowitz]

On Mon, Dec 29, 2008 at 07:12:56PM +0000, Pedro Alves wrote:
> > How about var_zuinteger, for zeroable unsigned Integer?
> 
> Hmm, I was trying to convey the idea that negatives mean unlimited,
> as the watchpoint/breakpoint limits command explicitly mentions
> that.  But, it's just a name, I'm not married to any.

Just because the existing names are abbreviated doesn't mean they all
have to be... how about var_unlimited_uinteger, documented as an
unsigned integer value or the string "unlimited"?  And then we do not
need to overload any number in the UI, just internally.  We could
leave the variable an int, let [0, INT_MAX - 1] be valid, and INT_MAX
mean unlimited.

-- 
Daniel Jacobowitz
CodeSourcery