[PATCH] sim: tweak signed to unsigned local vars

[PATCH] sim: tweak signed to unsigned local vars

[Mike Frysinger]

This tweaks a lot of hardware code to use "unsigned" instead of "int" to
fix gcc warnings about signed/unsigned comparisons.  In these cases, the
code is already working with unsigned variables, so there shouldn't be a
problem converting the local variables from "int".

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
2010-04-10  Mike Frysinger  <vapier@gentoo.org>

	* dv-sockser.c (dv_sockser_init): Change local tmp var to unsigned.
	* hw-ports.c, hw-ports.h (hw_port_encode): Change sizeof_buf arg to
	unsigned.
	* hw-properties.c (hw_add_range_array_property): Change local i var
	to unsigned.
	(hw_add_reg_array_property): Likewise.
	(hw_add_string_array_property): Change local vars sizeof_array and
	string_nr to unsigned.
	(hw_find_string_array_property): Change local vars nr_entries to
	unsigned.
	* hw-tree.c (split_device_specifier): Change local len var to
	unsigned.
	(print_properties): Change local cell_nr var to unsigned.
	* sim-core.c (sim_core_read_buffer): Change local nr_bytes var to
	unsigned.
	(sim_core_write_buffer): Likewise.

 sim/common/dv-sockser.c    |    3 ++-
 sim/common/hw-ports.c      |    2 +-
 sim/common/hw-ports.h      |    2 +-
 sim/common/hw-properties.c |   10 +++++-----
 sim/common/hw-tree.c       |    4 ++--
 sim/common/sim-core.c      |    4 ++--
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/sim/common/dv-sockser.c b/sim/common/dv-sockser.c
index 643ce34..18987f9 100644
--- a/sim/common/dv-sockser.c
+++ b/sim/common/dv-sockser.c
@@ -129,7 +129,8 @@ dv_sockser_init (SIM_DESC sd)
   struct sockaddr_in sockaddr;
   char hostname[100];
   const char *port_str;
-  int tmp,port;
+  unsigned tmp;
+  int port;
 
   if (STATE_ENVIRONMENT (sd) != OPERATING_ENVIRONMENT
       || sockser_addr == NULL)
diff --git a/sim/common/hw-ports.c b/sim/common/hw-ports.c
index 8f88cb3..5adb072 100644
--- a/sim/common/hw-ports.c
+++ b/sim/common/hw-ports.c
@@ -292,7 +292,7 @@ int
 hw_port_encode (struct hw *me,
 		int port_number,
 		char *buf,
-		int sizeof_buf,
+		unsigned sizeof_buf,
 		port_direction direction)
 {
   const struct hw_port_descriptor *ports = NULL;
diff --git a/sim/common/hw-ports.h b/sim/common/hw-ports.h
index 01a4dcd..f6c8e6b 100644
--- a/sim/common/hw-ports.h
+++ b/sim/common/hw-ports.h
@@ -121,7 +121,7 @@ int hw_port_encode
 (struct hw *me,
  int port_number,
  char *buf,
- int sizeof_buf,
+ unsigned sizeof_buf,
  port_direction direction);
  
 
diff --git a/sim/common/hw-properties.c b/sim/common/hw-properties.c
index 018a84c..d1d6dc5 100644
--- a/sim/common/hw-properties.c
+++ b/sim/common/hw-properties.c
@@ -583,7 +583,7 @@ hw_add_range_array_property (struct hw *me,
 			   * sizeof (unsigned_cell));
   unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
   unsigned_cell *cell;
-  int i;
+  unsigned i;
   
   /* copy the property elements over */
   cell = cells;
@@ -676,7 +676,7 @@ hw_add_reg_array_property (struct hw *me,
 			   * sizeof (unsigned_cell));
   unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
   unsigned_cell *cell;
-  int i;
+  unsigned i;
   
   /* copy the property elements over */
   cell = cells;
@@ -776,8 +776,8 @@ hw_add_string_array_property (struct hw *me,
 			      const string_property_spec *strings,
 			      unsigned nr_strings)
 {
-  int sizeof_array;
-  int string_nr;
+  unsigned sizeof_array;
+  unsigned string_nr;
   char *array;
   char *chp;
   if (nr_strings == 0)
@@ -840,7 +840,7 @@ hw_find_string_array_property (struct hw *me,
       ASSERT (((char*)node->array)[node->sizeof_array - 1] == '\0');
       {
 	const char *chp = node->array;
-	int nr_entries = 0;
+	unsigned nr_entries = 0;
 	/* count the number of strings, keeping an eye out for the one
 	   we're looking for */
 	*string = chp;
diff --git a/sim/common/hw-tree.c b/sim/common/hw-tree.c
index d3530d0..7276b7c 100644
--- a/sim/common/hw-tree.c
+++ b/sim/common/hw-tree.c
@@ -87,7 +87,7 @@ split_device_specifier (struct hw *current,
     {
       struct hw *aliases = hw_tree_find_device (current, "/aliases");
       char alias[32];
-      int len = 0;
+      unsigned len = 0;
       while (device_specifier[len] != '\0'
 	     && device_specifier[len] != '/'
 	     && device_specifier[len] != ':'
@@ -1115,7 +1115,7 @@ print_properties (struct hw *me,
 		if ((property->sizeof_array % sizeof (signed_cell)) == 0)
 		  {
 		    unsigned_cell *w = (unsigned_cell*) property->array;
-		    int cell_nr;
+		    unsigned cell_nr;
 		    for (cell_nr = 0;
 			 cell_nr < (property->sizeof_array / sizeof (unsigned_cell));
 			 cell_nr++)
diff --git a/sim/common/sim-core.c b/sim/common/sim-core.c
index 5476ead..fd982bb 100644
--- a/sim/common/sim-core.c
+++ b/sim/common/sim-core.c
@@ -547,7 +547,7 @@ sim_core_read_buffer (SIM_DESC sd,
 #if (WITH_HW)
     if (mapping->device != NULL)
       {
-	int nr_bytes = len - count;
+	unsigned nr_bytes = len - count;
 	if (raddr + nr_bytes - 1> mapping->bound)
 	  nr_bytes = mapping->bound - raddr + 1;
 	if (sim_hw_io_read_buffer (sd, mapping->device,
@@ -615,7 +615,7 @@ sim_core_write_buffer (SIM_DESC sd,
       if (WITH_CALLBACK_MEMORY
 	  && mapping->device != NULL)
 	{
-	  int nr_bytes = len - count;
+	  unsigned nr_bytes = len - count;
 	  if (raddr + nr_bytes - 1 > mapping->bound)
 	    nr_bytes = mapping->bound - raddr + 1;
 	  if (sim_hw_io_write_buffer (sd, mapping->device,
-- 
1.7.0.2

Re: [PATCH] sim: tweak signed to unsigned local vars

[Joel Brobecker]

> diff --git a/sim/common/dv-sockser.c b/sim/common/dv-sockser.c
> index 643ce34..18987f9 100644
> --- a/sim/common/dv-sockser.c
> +++ b/sim/common/dv-sockser.c
> @@ -129,7 +129,8 @@ dv_sockser_init (SIM_DESC sd)
>    struct sockaddr_in sockaddr;
>    char hostname[100];
>    const char *port_str;
> -  int tmp,port;
> +  unsigned tmp;
> +  int port;

I'm not sure I agree on this one. "tmp" is used to store the result of
a subtraction of 2 pointers. IIRC, the exact type returned is ptrdiff_t,
which is a signed value...

>  hw_port_encode (struct hw *me,
>  		int port_number,
>  		char *buf,
> -		int sizeof_buf,
> +		unsigned sizeof_buf,
>  		port_direction direction)

Would type size_t work, in this case?

> - int sizeof_buf,
> + unsigned sizeof_buf,

Same here?

> @@ -583,7 +583,7 @@ hw_add_range_array_property (struct hw *me,
>  			   * sizeof (unsigned_cell));
>    unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
>    unsigned_cell *cell;
> -  int i;
> +  unsigned i;

I am not sure about this one either. Can you try changing the type
of parameter nr_ranges to int? It looks like the only usage of this
function is in hw-tree, and the variable used is actually an int...

> @@ -676,7 +676,7 @@ hw_add_reg_array_property (struct hw *me,
>  			   * sizeof (unsigned_cell));
>    unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
>    unsigned_cell *cell;
> -  int i;
> +  unsigned i;

Same here.

> @@ -776,8 +776,8 @@ hw_add_string_array_property (struct hw *me,
>  			      const string_property_spec *strings,
>  			      unsigned nr_strings)

Same for nr_strings.

> -  int sizeof_array;
> -  int string_nr;
> +  unsigned sizeof_array;
> +  unsigned string_nr;

Once nr_strings is changed to int, we should no longer need to change
string_nr to unsigned.

Would type size_t for sizeof_array work in your case?  It seems that
the use of int or unsigned is a little random for this type of argument
(check the use of the hw_add_property function, sometimes we pass an int,
sometimes we pass an unsigned).

> @@ -840,7 +840,7 @@ hw_find_string_array_property (struct hw *me,
>        ASSERT (((char*)node->array)[node->sizeof_array - 1] == '\0');
>        {
>  	const char *chp = node->array;
> -	int nr_entries = 0;
> +	unsigned nr_entries = 0;

Same as above.

> @@ -1115,7 +1115,7 @@ print_properties (struct hw *me,
>  		if ((property->sizeof_array % sizeof (signed_cell)) == 0)
>  		  {
>  		    unsigned_cell *w = (unsigned_cell*) property->array;
> -		    int cell_nr;
> +		    unsigned cell_nr;

Use size_t?

-- 
Joel

Re: [PATCH] sim: tweak signed to unsigned local vars

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2093 bytes --]

On Monday 12 April 2010 11:24:13 Joel Brobecker wrote:
> > diff --git a/sim/common/dv-sockser.c b/sim/common/dv-sockser.c
> > index 643ce34..18987f9 100644
> > --- a/sim/common/dv-sockser.c
> > +++ b/sim/common/dv-sockser.c
> > @@ -129,7 +129,8 @@ dv_sockser_init (SIM_DESC sd)
> > 
> >    struct sockaddr_in sockaddr;
> >    char hostname[100];
> >    const char *port_str;
> > 
> > -  int tmp,port;
> > +  unsigned tmp;
> > +  int port;
> 
> I'm not sure I agree on this one. "tmp" is used to store the result of
> a subtraction of 2 pointers. IIRC, the exact type returned is ptrdiff_t,
> which is a signed value...

true, but the math should never yield a negative value.  the compare is 
between a base pointer and a pointer returned from strchr() on the base 
pointer.  so the value should always be >= 0 and it should always fit in 
32bits.

> >  hw_port_encode (struct hw *me,
> >  
> >  		int port_number,
> >  		char *buf,
> > 
> > -		int sizeof_buf,
> > +		unsigned sizeof_buf,
> > 
> >  		port_direction direction)
> 
> Would type size_t work, in this case?

size_t would be usable in pretty much all the places i changed, but i 
consciously did not pick that because "unsigned" is the current convention, 
both with local vars and function arguments.  i didnt want to desync the type 
conventions where some used size_t and some used unsigned especially since 
they're different sizes on 64bit systems.

i certainly wouldnt be opposed to someone mass converting the source to 
size_t, but i dont think i'm going to volunteer for that ;)

> > @@ -583,7 +583,7 @@ hw_add_range_array_property (struct hw *me,
> > 
> >  			   * sizeof (unsigned_cell));
> >    
> >    unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
> >    unsigned_cell *cell;
> > 
> > -  int i;
> > +  unsigned i;
> 
> I am not sure about this one either. Can you try changing the type
> of parameter nr_ranges to int? It looks like the only usage of this
> function is in hw-tree, and the variable used is actually an int...

i'll check these
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: tweak signed to unsigned local vars

[Joel Brobecker]

> > I'm not sure I agree on this one. "tmp" is used to store the result of
> > a subtraction of 2 pointers. IIRC, the exact type returned is ptrdiff_t,
> > which is a signed value...
> 
> true, but the math should never yield a negative value.  the compare is 
> between a base pointer and a pointer returned from strchr() on the base 
> pointer.  so the value should always be >= 0 and it should always fit in 
> 32bits.

I always tend to be very careful with this type of reasoning, probably
because of my past writing safety-critical software (and also how
surprisingly difficult it turned out to be to formally prove that
a piece of code would never overflow).

Even if what you are saying is true, I think that we'll have less
conversion issues if we use the proper types.  But that's just me.
Perhaps others with more C experience than I do will agree with you
that we're fussing over something that actually does not matter.

> size_t would be usable in pretty much all the places i changed, but i 
> consciously did not pick that because "unsigned" is the current convention, 
> both with local vars and function arguments.  i didnt want to desync the type 
> conventions where some used size_t and some used unsigned especially since 
> they're different sizes on 64bit systems.

I actually think, from the little that I have seen, that the code is
actually pretty confused on whether to use signed or unsigned. Again,
perhaps it's my Ada background where using the proper types is super
important (and an invaluable help), but, never mind - if you can make it
work with unsigned, then this is fine with me. You shouldn't have to pay
for the sins of others.

-- 
Joel

Re: [PATCH] sim: tweak signed to unsigned local vars

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1918 bytes --]

On Monday 12 April 2010 12:06:18 Joel Brobecker wrote:
> > > I'm not sure I agree on this one. "tmp" is used to store the result of
> > > a subtraction of 2 pointers. IIRC, the exact type returned is
> > > ptrdiff_t, which is a signed value...
> > 
> > true, but the math should never yield a negative value.  the compare is
> > between a base pointer and a pointer returned from strchr() on the base
> > pointer.  so the value should always be >= 0 and it should always fit in
> > 32bits.
> 
> I always tend to be very careful with this type of reasoning, probably
> because of my past writing safety-critical software (and also how
> surprisingly difficult it turned out to be to formally prove that
> a piece of code would never overflow).
> 
> Even if what you are saying is true, I think that we'll have less
> conversion issues if we use the proper types.  But that's just me.
> Perhaps others with more C experience than I do will agree with you
> that we're fussing over something that actually does not matter.

this is a string passed via command line option.  if it were even remotely 
close to the size required to overflow ("unsigned" -> "unsigned int" -> 32bit 
on all systems where this socket code is going to be used), something is 
terribly wrong elsewhere.  even if we grant it 16bit storage, a 65k string 
specifying a host:port makes no sense, especially when many systems have a 
hard time swallowing a command line over 32k.

i dont have a problem coding defensively when it makes sense, but i cant see 
any such requirements in any of the sim code.

further, while ptrdiff_t may make sense since we're diffing two pointers, the 
value is passed straight into strncpy() which interprets the value as size_t.  
so if we were going to spend the time on this (which i dont think this code 
warrants), a lot more code needs to change than the variable type.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: tweak signed to unsigned local vars

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 723 bytes --]

On Monday 12 April 2010 11:24:13 Joel Brobecker wrote:
> > @@ -583,7 +583,7 @@ hw_add_range_array_property (struct hw *me,
> > 
> >  			   * sizeof (unsigned_cell));
> >    
> >    unsigned_cell *cells = hw_zalloc (me, sizeof_cells);
> >    unsigned_cell *cell;
> > 
> > -  int i;
> > +  unsigned i;
> 
> I am not sure about this one either. Can you try changing the type
> of parameter nr_ranges to int? It looks like the only usage of this
> function is in hw-tree, and the variable used is actually an int...

the relevant funcs involved here seem like they should all be converted to 
unsigned.  in the event of an error, they abort, and only ever return unsigned 
values.  i'll go that route.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: tweak signed to unsigned local vars

[Joel Brobecker]

> i dont have a problem coding defensively when it makes sense, but i
> cant see any such requirements in any of the sim code.

I don't disagree with you, and will stop arguing over this, now.
The use of unsigned will have to do.

-- 
Joel