diff mbox

RFA: Fix sim scache handling to use unsigned values.

Message ID 87r3gtkbg3.fsf@redhat.com
State New, archived
Headers

Commit Message

Nick Clifton Feb. 3, 2016, 4:25 p.m. UTC 
  Hi Mike,

  A recent bug report on the binutils list pointed out that a construct
  like this:

   for (i = 1; i; i <<= 1)

  can have undefined behaviour if the loop variable is signed and the
  shift operation moves a 1 into the sign bit:

    lists.gnu.org/archive/html/bug-binutils/2016-02/msg00006.html

  A quick scan of the simulator sources found one place where this
  happens, in common/cgen-scache.c:scache_option_handler().  Looking at
  the code here it seemed to me that the simplest fix is to switch to
  using an unsigned int for the parsed value of the scache size.  (A
  negative scache size does not make any sense right ?)  I wondered if
  it would be worth using "unsigned long" instead of "unsigned int",
  since there is no strtou() function, but this seemed like overkill.

  So I created the patch below.  It builds OK, but I am not sure how to
  test it.  Any suggestions ?

Cheers
  Nick

Comments

Mike Frysinger Feb. 3, 2016, 6:55 p.m. UTC | #1 
On 03 Feb 2016 16:25, Nick Clifton wrote:
>   A recent bug report on the binutils list pointed out that a construct
>   like this:
> 
>    for (i = 1; i; i <<= 1)
> 
>   can have undefined behaviour if the loop variable is signed and the
>   shift operation moves a 1 into the sign bit:
> 
>     lists.gnu.org/archive/html/bug-binutils/2016-02/msg00006.html
> 
>   A quick scan of the simulator sources found one place where this
>   happens, in common/cgen-scache.c:scache_option_handler().  Looking at
>   the code here it seemed to me that the simplest fix is to switch to
>   using an unsigned int for the parsed value of the scache size.  (A
>   negative scache size does not make any sense right ?)  I wondered if
>   it would be worth using "unsigned long" instead of "unsigned int",
>   since there is no strtou() function, but this seemed like overkill.
> 
>   So I created the patch below.  It builds OK, but I am not sure how to
>   test it.  Any suggestions ?

seems fine
-mike
diff mbox

Patch

diff --git a/sim/common/cgen-scache.c b/sim/common/cgen-scache.c
index 3a79514..cd1aa11 100644
--- a/sim/common/cgen-scache.c
+++ b/sim/common/cgen-scache.c
@@ -121,24 +121,26 @@  scache_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt,
 	{
 	  if (arg != NULL)
 	    {
-	      int n = strtol (arg, NULL, 0);
+	      unsigned int n = (unsigned int) strtoul (arg, NULL, 0);
 	      if (n < MIN_SCACHE_SIZE)
 		{
-		  sim_io_eprintf (sd, "invalid scache size `%d', must be at least 4", n);
+		  sim_io_eprintf (sd, "invalid scache size `%u', must be at least %u",
+				  n, MIN_SCACHE_SIZE);
 		  return SIM_RC_FAIL;
 		}
 	      /* Ensure it's a multiple of 2.  */
 	      if ((n & (n - 1)) != 0)
 		{
-		  sim_io_eprintf (sd, "scache size `%d' not a multiple of 2\n", n);
-		  {
-		    /* round up to nearest multiple of 2 */
-		    int i;
-		    for (i = 1; i < n; i <<= 1)
-		      continue;
-		    n = i;
-		  }
-		  sim_io_eprintf (sd, "rounding scache size up to %d\n", n);
+		  unsigned int i;
+		  sim_io_eprintf (sd, "scache size `%u' not a multiple of 2\n", n);
+		  /* Round up to nearest multiple of 2.  */
+		  for (i = 1; i && i < n; i <<= 1)
+		    continue;
+		  if (i)
+		    {
+		      n = i;
+		      sim_io_eprintf (sd, "rounding scache size up to %u\n", n);
+		    }
 		}
 	      if (cpu == NULL)
 		STATE_SCACHE_SIZE (sd) = n;