Re: [PATCH] Fix v9/64-bit strcmp when string ends in multiple zero bytes.

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/30/2014 04:00 PM, David Miller wrote:
> 
> Can I get a quick ACK for the strcmp testcase change?  I also plan to
> commit this also to the active release branches.

The test is a bit "narrow" in scope and I'd like to see you
expand it just a bit.

OK with changes.

> diff --git a/string/test-strcmp.c b/string/test-strcmp.c
> index b395dc7..d1c73a3 100644
> --- a/string/test-strcmp.c
> +++ b/string/test-strcmp.c
> @@ -329,6 +329,35 @@ check (void)
>  		FOR_EACH_IMPL (impl, 0)
>  		check_result (impl, s1 + i1, s2 + i2, exp_result);
>        }
> +
> +  /* Test cases where there are multiple zero bytes after the first.  */
> +
> +  for (size_t i = 0; i < 8; i++)

Cover the full length of the available strings e.g. MIN(l1, l2);

> +    {
> +      int exp_result;
> +
> +      for (CHAR val = 0x01; val < 0x10; val++)

Permute over all char values e.g. [0x1,0xff] or val < 0x100;

> +	{
> +	  for (size_t j = 0; j < i; j++)
> +	    {
> +	      s1[j] = val;
> +	      s2[j] = val;
> +	    }
> +
> +	  s1[i] = 0x00;
> +	  s2[i] = val;
> +
> +	  for (size_t j = i + 1; j < 16; j++)
> +	    {
> +	      s1[j] = 0x00;
> +	      s2[j] = 0x00;
> +	    }

As i only moves forward and j fills with val up to i,
this loop clears more than it should?

Hoist this out of the loop and just initialize both of
the strings to 0x00.

Given that s2 is larger than s1 you can show that s2
is always terminated even though you add an extra val
in there to make the strcmp fail.

> +
> +	  exp_result = SIMPLE_STRCMP (s1, s2);
> +	  FOR_EACH_IMPL (impl, 0)
> +	    check_result (impl, s1, s2, exp_result);
> +	}
> +    }
>  }

OK with those changes and verification that it still
catches the original failure case and hasn't exponentially
blown up the test time :}

Cheers,
Carlos.