[PATCH 0/2] Use zuinteger_unlimited

[PATCH 0/2] Use zuinteger_unlimited

[Yao Qi]

These two patches are to change commands to use zuinteger_unlimited,
as these commands are fit to zuinteger_unlimited command.

These two patches were original posted in this series as 2/3 and 3/3,

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

during the review, I decided to post patch 1/3 together with other
changes here,

  [RFC 0/3] Get rid of var_integer in CLI
  http://sourceware.org/ml/gdb-patches/2012-08/msg00364.html

this series was approved by Tom and committed.  It is time to revisit the
patch 2/3 and 3/3 again.  I rebase them on the latest CVS trunk and
update changelog entry a little.

Regression tested on x86_64-linux.  Is it OK?

-- 
1.7.7.6

[PATCH 2/2] 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:

2013-02-15  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 e394605..1a6ceb7 100644
--- a/gdb/alpha-tdep.c
+++ b/gdb/alpha-tdep.c
@@ -1872,20 +1872,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 c7684f8..12bb8ab 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8884,20 +8884,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/2] 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.

Previously, without this patch, negative means unlimited, but after
this patch applied, only -1 means unlimited.  Other negative numbers
are invalid.  However, the doc says -1 is treated as unlimited, as
below,

  set remote hardware-watchpoint-limit limit
  set remote hardware-breakpoint-limit limit
  Restrict gdb to using limit remote hardware breakpoint or watchpoints.
  A limit of -1, the default, is treated as unlimited.

  set remote hardware-watchpoint-length-limit limit
  Restrict gdb to using limit bytes for the maximum length of a remote
  hardware watchpoint. A limit of -1, the default, is treated as
  unlimited.

So patched GDB is still compatible with old GDB on documented
behavior, so we don't have to update doc and mention this change in
NEWS.

gdb:

2013-02-15  Yao Qi  <yao@codesourcery.com>

	* remote.c (remote_hw_watchpoint_limit): Make it 'static' and
	change it to 'unsigned'.
	(remote_hw_watchpoint_length_limit): Likewise.
	(remote_hw_breakpoint_limit): Likewise.
	(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:

2013-02-15  Yao Qi  <yao@codesourcery.com>

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

diff --git a/gdb/remote.c b/gdb/remote.c
index 18fe61d..b30ba37 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8226,17 +8226,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
@@ -8250,8 +8248,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;
     }
@@ -8259,7 +8255,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;
@@ -11551,33 +11547,48 @@ 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, _("\
-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, _("\
+Specify -1 for unlimited."),
+				       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 -1 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_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);
+Specify -1 for unlimited."),
+				       NULL,
+				       NULL, /* FIXME: i18n: The maximum
+						number of target hardware
+						breakpoints is %s.  */
+				       &remote_set_cmdlist,
+				       &remote_show_cmdlist);
 
   add_setshow_uinteger_cmd ("remoteaddresssize", class_obscure,
 			    &remote_address_size, _("\
diff --git a/gdb/testsuite/gdb.base/setshow.exp b/gdb/testsuite/gdb.base/setshow.exp
index eadd1f5..ed35dc9 100644
--- a/gdb/testsuite/gdb.base/setshow.exp
+++ b/gdb/testsuite/gdb.base/setshow.exp
@@ -258,3 +258,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

ping : [PATCH 0/2] Use zuinteger_unlimited

[Yao Qi]

On 02/15/2013 09:27 PM, Yao Qi wrote:
> These two patches are to change commands to use zuinteger_unlimited,
> as these commands are fit to zuinteger_unlimited command.
>
> These two patches were original posted in this series as 2/3 and 3/3,
>
>    [RFC 0/3] New var_types var_zuinteger_unlimited
>    http://sourceware.org/ml/gdb-patches/2012-08/msg00020.html
>
> during the review, I decided to post patch 1/3 together with other
> changes here,
>
>    [RFC 0/3] Get rid of var_integer in CLI
>    http://sourceware.org/ml/gdb-patches/2012-08/msg00364.html
>
> this series was approved by Tom and committed.  It is time to revisit the
> patch 2/3 and 3/3 again.  I rebase them on the latest CVS trunk and
> update changelog entry a little.
>
> Regression tested on x86_64-linux.  Is it OK?
>

Ping.

-- 
Yao (齐尧)

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Pedro Alves]

On 02/15/2013 01:27 PM, Yao Qi wrote:
> -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;

...

> @@ -8259,7 +8255,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)

This made me notice something with var_zuinteger_unlimited.

What's the point of making it work with unsigned variables,
and UINT_MAX, if the contents of the variable are actually
treated as int everywhere in cli-setshow.c? (and val is
still cut at INT_MAX).  Vis, e.g.,

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

If reads like cli-setshow.c's handling of
var_zuinteger_unlimited was never fully finished?

-- 
Pedro Alves

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Yao Qi]

On 03/04/2013 09:19 PM, Pedro Alves wrote:
> On 02/15/2013 01:27 PM, Yao Qi wrote:
>> -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;
>
> ...
>
>> @@ -8259,7 +8255,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)
>
> This made me notice something with var_zuinteger_unlimited.
>
> What's the point of making it work with unsigned variables,
> and UINT_MAX, if the contents of the variable are actually
> treated as int everywhere in cli-setshow.c? (and val is
> still cut at INT_MAX).  Vis, e.g.,
>

Yes, in cli-setshow.c, the val is treated as signed integer, because -1 
stands for unlimited.  Outside of cli, var_zuinteger_unlimited is about 
a unsigned integer with a unlimited state.  That is reason I change type 
of "remote_hw_watchpoint_limit" from signed to unsigned.  Their default 
value is unlimited (-1), so I initialize them to UINT_MAX.  Originally I 
used "(unsigned int) -1", but UINT_MAX is better as "unlimited" here.

-- 
Yao (齐尧)

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Pedro Alves]

On 03/04/2013 02:37 PM, Yao Qi wrote:
> On 03/04/2013 09:19 PM, Pedro Alves wrote:
>> On 02/15/2013 01:27 PM, Yao Qi wrote:
>>> -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;
>>
>> ...
>>
>>> @@ -8259,7 +8255,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)
>>
>> This made me notice something with var_zuinteger_unlimited.
>>
>> What's the point of making it work with unsigned variables,
>> and UINT_MAX, if the contents of the variable are actually
>> treated as int everywhere in cli-setshow.c? (and val is
>> still cut at INT_MAX).  Vis, e.g.,
>>
> 

(swapping for better answer:)

> That is reason I change type of "remote_hw_watchpoint_limit" from signed to
> unsigned.  Their default value is unlimited (-1), so I initialize them to UINT_MAX.
> Originally I used "(unsigned int) -1", but UINT_MAX is better as "unlimited" here.

That's all clear, and a _consequence_ of var_zuinteger_unlimited
wanting an unsigned variable.

> Yes, in cli-setshow.c, the val is treated as signed integer, because -1 stands for unlimited.
> Outside of cli, var_zuinteger_unlimited is about a unsigned integer with a unlimited
> state.

But then, var_zuinteger_unlimited treats numbers
from INT_MAX to UINT_MAX-1 as invalid, and, while
forbidding any other negative number except -1 at
the same time.

(gdb) set listsize 2147483647        (INT_MAX)
integer 2147483647 out of range
(gdb) set listsize 2147483647+1
only -1 is allowed to set as unlimited
(gdb) set listsize 4294967295U       (UINT_MAX)
integer 4294967295 out of range
(gdb) set listsize 4294967295U-1
integer 4294967294 out of range

Okay, so I notice that's documented in:

    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an unsigned
       int, but its range is [0, INT_MAX].  -1 stands for unlimited.  */
    var_zuinteger_unlimited,

But I ask, what's either:

 - the point of making it internally signed
   with things like:

   if (*(int *) c->var != val)

   and forbidding INT_MAX..UINT_MAX-1.  (Not that I'm
   arguing it should).  The care to only accept -1
   and not any other negative number made me think
   numbers in that range should be accepted.

 - the point of making it externally unsigned if
   it only accepts [0, INT_MAX].  If the variable
   assigned to the command was signed too, then this
   range would be both implicit and explicit, meaning, one
   weird detail less the user of the API needs to know.

BTW, if the variable is unsigned, then I believe accessing it
as signed as cli-setshow.c does is actually undefined behavior.

-- 
Pedro Alves

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Yao Qi]

On 03/05/2013 12:20 AM, Pedro Alves wrote:
> But I ask, what's either:
> 
>   - the point of making it internally signed
>     with things like:
> 
>     if (*(int *) c->var != val)
> 
>     and forbidding INT_MAX..UINT_MAX-1.  (Not that I'm
>     arguing it should).  The care to only accept -1
>     and not any other negative number made me think
>     numbers in that range should be accepted.

Internally, we use -1 for unlimited, so it is signed.  Externally, it 
is unsigned, so we have to forbid the range (INT_MAX, UINT_MAX - 1].

> 
>   - the point of making it externally unsigned if
>     it only accepts [0, INT_MAX].  If the variable

because we need -1 for unlimited internally.  I don't see anything 
wrong to define unsigned type whose range is [0, INT_MAX].

>     assigned to the command was signed too, then this
>     range would be both implicit and explicit, meaning, one
>     weird detail less the user of the API needs to know.

If I understand you correctly, this is what you want,

extern void
  add_setshow_zuinteger_unlimited_cmd (char *name,
				       enum command_class class,
				       int *var,
                                       ^^^^^^^^
then, IMO, the API is weird, in which the type of parameter VAR 
(singed) is not consistent with the function (zuinteger_unlimited).
Since these CLI stuff provide APIs to other components, it is better to 
review them externally first.

However, I don't insist on that if you believe changing the type of VAR 
from "unsigned int" to "int" is better, I am OK  to change it to:

    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an
       int, but its range is [0, INT_MAX].  -1 stands for
       unlimited and other negative numbers are not allowed.  */

The patch below is regression tested on x86_64-linux.

-- 
Yao (齐尧)

gdb:

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

	* cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): Change
	parameter VAR's type from "unsigned int" to "int".
	* command.h (var_zuinteger_unlimited): Update its comments.
	(add_setshow_zuinteger_unlimited_cmd): Update the declaration.
---
 gdb/cli/cli-decode.c |    2 +-
 gdb/command.h        |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 49ccef3..a8f7747 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -708,7 +708,7 @@ add_setshow_zinteger_cmd (char *name, enum command_class class,
 void
 add_setshow_zuinteger_unlimited_cmd (char *name,
 				     enum command_class class,
-				     unsigned int *var,
+				     int *var,
 				     const char *set_doc,
 				     const char *show_doc,
 				     const char *help_doc,
diff --git a/gdb/command.h b/gdb/command.h
index 17662b4..a25fe04 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -99,8 +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.  */
+    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an int,
+       but its range is [0, INT_MAX].  -1 stands for unlimited and
+       other negative numbers are not allowed.  */
     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
@@ -361,7 +362,7 @@ extern void add_setshow_zuinteger_cmd (char *name,
 extern void
   add_setshow_zuinteger_unlimited_cmd (char *name,
 				       enum command_class class,
-				       unsigned int *var,
+				       int *var,
 				       const char *set_doc,
 				       const char *show_doc,
 				       const char *help_doc,
-- 
1.7.7.6

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Pedro Alves]

On 03/05/2013 07:44 AM, Yao Qi wrote:
> On 03/05/2013 12:20 AM, Pedro Alves wrote:
>> But I ask, what's either:
>>
>>   - the point of making it internally signed
>>     with things like:
>>
>>     if (*(int *) c->var != val)
>>
>>     and forbidding INT_MAX..UINT_MAX-1.  (Not that I'm
>>     arguing it should).  The care to only accept -1
>>     and not any other negative number made me think
>>     numbers in that range should be accepted.
> 
> Internally, we use -1 for unlimited, so it is signed.  Externally, it 
> is unsigned, so we have to forbid the range (INT_MAX, UINT_MAX - 1].

No, we don't _have_ to.  We could as well make it internally
unsigned as well, and only reserve UINT_MAX, thus the range
of values would become [0, UINT_MAX-1], with UINT_MAX meaning
unlimited/"full throttle".  That'd be practically double the
range of values the setting supports.
That'd make a lot more sense, if you wanted to justify making
the variable unsigned.

>>   - the point of making it externally unsigned if
>>     it only accepts [0, INT_MAX].  If the variable
> 
> because we need -1 for unlimited internally.  

You're just reading the code to me, not explaining it.

We don't _need_ -1 for unlimited internally.  We could just as
well treat the variable internally as the correct type of
unsigned int, and handle UINT_MAX as unlimited internally too.
In fact, that'd be the _correct_ way to code it.  Assuming
2's complement _representation_ (unlike signed->unsigned
conversion), as in

    case var_zuinteger_unlimited:
      {
	if (*(int *) c->var == -1)

while the object at c->var is unsigned int, is not valid C.

There's really absolutely no reason at all for doing things
internally and externally differently.

> I don't see anything 
> wrong to define unsigned type whose range is [0, INT_MAX].


> 
>>     assigned to the command was signed too, then this
>>     range would be both implicit and explicit, meaning, one
>>     weird detail less the user of the API needs to know.
> 
> If I understand you correctly, this is what you want,
> 
> extern void
>   add_setshow_zuinteger_unlimited_cmd (char *name,
> 				       enum command_class class,
> 				       int *var,
>                                        ^^^^^^^^
> then, IMO, the API is weird, in which the type of parameter VAR 
> (singed) is not consistent with the function (zuinteger_unlimited).
> Since these CLI stuff provide APIs to other components, it is better to 
> review them externally first.

In turn, the external API was weird in that it has a hole that
is easy to fall into -- the [INT_MAX..UINT_MAX-1] range.  Signed
makes it dead simple -- "The whole set of positive values that fit
in 'int' is valid for regular uses; negative values are special", and
gives the compiler a chance to catch problems.  Sign based clearly
signals to the API user (which includes uses of the variable beyond
the set/show commands themselves) that they can't go beyond INT_MAX.

> 
> However, I don't insist on that if you believe changing the type of VAR 
> from "unsigned int" to "int" is better, I am OK  to change it to:
> 
>     /* ZeroableUnsignedInteger with unlimited value.  *VAR is an
>        int, but its range is [0, INT_MAX].  -1 stands for
>        unlimited and other negative numbers are not allowed.  */
> 

Thanks, that's better.

I like that we no longer assume 2's complement in the API
description, talking about unsigned and -1 (while API clients
actually used UINT_MAX).

> 2013-03-05  Yao Qi  <yao@codesourcery.com>
>
> 	* cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): Change
> 	parameter VAR's type from "unsigned int" to "int".
> 	* command.h (var_zuinteger_unlimited): Update its comments.
> 	(add_setshow_zuinteger_unlimited_cmd): Update the declaration.

This is OK.

-- 
Pedro Alves

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Doug Evans]

On Tue, Mar 5, 2013 at 4:59 AM, Pedro Alves <palves@redhat.com> wrote:
> We could just as
> well treat the variable internally as the correct type of
> unsigned int, and handle UINT_MAX as unlimited internally too.

Part of me is weeping that we didn't take this route.

If uinteger is in the name, anything else just invites problems, if
not tears ... :-)

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Pedro Alves]

On 03/05/2013 05:33 PM, Doug Evans wrote:
> On Tue, Mar 5, 2013 at 4:59 AM, Pedro Alves <palves@redhat.com> wrote:
>> We could just as
>> well treat the variable internally as the correct type of
>> unsigned int, and handle UINT_MAX as unlimited internally too.
> 
> Part of me is weeping that we didn't take this route.

In a way, I suppose I don't like it, because it ties a
magic _positive_ number as upper bound in the user/api
interface.  And that also invites problems and tears
down the road.  :-)

IOW, the magic number UINT_MAX actually depends on
host int width (32/64-bit, etc., exotic, but they exist),
so e.g., in theory python scripts may have trouble with that.
Also, if at some point we want to extend the range of
these variables to 64-bit, the magic number has to change,
as then UINT_MAX is midway through the valid range...
Signed negative "-1", "-2", etc. don't have that issue.
They mean the same whatever the width.

(And if we want another magic number in addition to "unlimited"?
Say, we need "disabled", while "0" still meaning "0".
Reserving and writting "UINT_MAX-1" gets much uglier then...
Maybe we'll end up with a struct instead of packing things
into a single number.)

That's a load of theory though.  We could also just declare
the command is bound to a uint32_t variable, and document it as
such.

But still, OTOH, this same reasoning percolates all the
way to the remote protocol -- numbers in the remote protocol
don't actually have an upper bound / width hard coded.  So there's
a possible compatibility issue here.  Once UINT_MAX for one target
is deemed "not big enough", should we add new, different packets?
Or should GDB be adjusted to cope internally, while the visible
interfaces (cli/python/RSP) remain the same, only that they support
larger values?

The other approach (signed, -1) avoids actually thinking too
much about all these issues, and pondering on their
significance, hence my preference.  :-)

> If uinteger is in the name, anything else just invites problems, if
> not tears ... :-)

To me, it indicates that for "normal" values, this command only
really accepts positive values.  Negatives are almost an
implementation detail.  We could make it fully an implementation
detail, if "set foo unlimited" was supported (I do think it should),
and, if that isn't actually problematic for frontends/python (is it?),
though I could imagine the negative values being more convenient in
scripts than having to check for special non-numbers.

With all that said -- it's fine with me if people remove the u.

-- 
Pedro Alves

Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

[Doug Evans]

I think we need to take the set of var_foo options, and choose
something that works on the whole.

var_uinteger has values [1,UINT_MAX-1] plus 0 mapping to UINT_MAX.
var_zuinteger has values [0,UINT_MAX].
And I don't see a plan to change those to int.

Fortunately, I don't see var_zuinteger_unlimited in py-param.c.
What if we rename it to var_zpinteger_unlimited or some such.
[p for positive]
[I'd still keep it out of the python API until there's a compelling
need for it.]