Fix up tree-ssa/strn{cat,cpy-2}.c (PR tree-optimization/83075)

On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
> On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
> > Attached is a patch with the comment updated/simplified.
> > The tests do the job they need to do today so I just removed
> > the useless attribute but otherwise left them unchanged.  If
> > you would like to enhance them in some way please feel free.
> 
> Ok for trunk, with a minor nit.  I'll tweak the tests incrementally
> when it is in.

So here is the fix for those testcases.

They didn't test what they meant to test, because they didn't FAIL
without the patch.  That is because the bug was that the -W* option
affected code generation, so with -O2 -Wno-stringop-overflow it didn't
trigger it.
I've changed the tests to test both in a separate noipa function where
it doesn't know about the aliasing and string lengths from the caller,
in that case it does more verifications, including the content of the
whole buffer, and the individual values of the lengths,
and what you did before.

Regtested on x86_64-linux and i686-linux, verified that with the
r255446 tree-ssa-strlen.c change reverted it FAILs.

Ok for trunk?

2017-12-07  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/83075
	* gcc.dg/tree-ssa/strncpy-2.c: Use size_t instead of unsigned, add
	separate function with noipa attribute to also verify behavior when
	optimizers don't know the sizes and aliasing, verify resulting sizes
	and array content.  Add -Wstringop-overflow to dg-options.
	* gcc.dg/tree-ssa/strncat.c: Likewise.



	Jakub

On December 7, 2017 5:55:47 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
>> On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
>> > Attached is a patch with the comment updated/simplified.
>> > The tests do the job they need to do today so I just removed
>> > the useless attribute but otherwise left them unchanged.  If
>> > you would like to enhance them in some way please feel free.
>> 
>> Ok for trunk, with a minor nit.  I'll tweak the tests incrementally
>> when it is in.
>
>So here is the fix for those testcases.
>
>They didn't test what they meant to test, because they didn't FAIL
>without the patch.  That is because the bug was that the -W* option
>affected code generation, so with -O2 -Wno-stringop-overflow it didn't
>trigger it.
>I've changed the tests to test both in a separate noipa function where
>it doesn't know about the aliasing and string lengths from the caller,
>in that case it does more verifications, including the content of the
>whole buffer, and the individual values of the lengths,
>and what you did before.
>
>Regtested on x86_64-linux and i686-linux, verified that with the
>r255446 tree-ssa-strlen.c change reverted it FAILs.
>
>Ok for trunk?

OK. 

Richard. 

>2017-12-07  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/83075
>	* gcc.dg/tree-ssa/strncpy-2.c: Use size_t instead of unsigned, add
>	separate function with noipa attribute to also verify behavior when
>	optimizers don't know the sizes and aliasing, verify resulting sizes
>	and array content.  Add -Wstringop-overflow to dg-options.
>	* gcc.dg/tree-ssa/strncat.c: Likewise.
>
>--- gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c.jj	2017-12-06
>20:11:54.000000000 +0100
>+++ gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c	2017-12-07
>13:31:32.719722416 +0100
>@@ -1,19 +1,35 @@
>-/* PR tree-optimization/83075 - Invalid strncpy optimization
>-   { dg-do run }
>-   { dg-options "-O2 -Wno-stringop-overflow" } */
>+/* PR tree-optimization/83075 - Invalid strncpy optimization */
>+/* { dg-do run } */
>+/* { dg-options "-O2 -Wstringop-overflow" } */
> 
>-int main (void)
>+typedef __SIZE_TYPE__ size_t;
>+
>+__attribute__((noipa)) size_t
>+foo (char *p, char *q, size_t *r)
> {
>-  char a[8] = "";
>+  size_t n0 = __builtin_strlen (p);
>+  __builtin_strncpy (q, p, n0);		/* { dg-warning "specified bound
>depends on the length" } */
>+  size_t n1 = __builtin_strlen (p);
>+  *r = n0;
>+  return n1;
>+}
> 
>+int
>+main ()
>+{
>+  char a[8] = "";
>   __builtin_strcpy (a, "123");
>-
>-  unsigned n0 = __builtin_strlen (a);
>-
>-  __builtin_strncpy (a + 3, a, n0);
>-
>-  unsigned n1 = __builtin_strlen (a);
>-
>+  size_t n0 = __builtin_strlen (a);
>+  __builtin_strncpy (a + 3, a, n0);	/* { dg-warning "specified bound
>depends on the length" } */
>+  size_t n1 = __builtin_strlen (a);
>   if (n1 == n0)
>     __builtin_abort ();
>+  a[6] = '7';
>+  __builtin_strcpy (a, "456");
>+  size_t n2;
>+  if (foo (a, a + 3, &n2) != 7 || n2 != 3)
>+    __builtin_abort ();
>+  if (__builtin_memcmp (a, "4564567", sizeof "4564567"))
>+    __builtin_abort ();
>+  return 0;
> }
>--- gcc/testsuite/gcc.dg/tree-ssa/strncat.c.jj	2017-12-06
>20:11:54.000000000 +0100
>+++ gcc/testsuite/gcc.dg/tree-ssa/strncat.c	2017-12-07
>13:31:09.568008365 +0100
>@@ -1,19 +1,35 @@
>-/* PR tree-optimization/83075 - Invalid strncpy optimization
>-   { dg-do run }
>-   { dg-options "-O2 -Wno-stringop-overflow" } */
>+/* PR tree-optimization/83075 - Invalid strncpy optimization */
>+/* { dg-do run } */
>+/* { dg-options "-O2 -Wstringop-overflow" } */
> 
>-int main (void)
>+typedef __SIZE_TYPE__ size_t;
>+
>+__attribute__((noipa)) size_t
>+foo (char *p, char *q, size_t *r)
> {
>-  char a[8] = "";
>+  size_t n0 = __builtin_strlen (p);
>+  __builtin_strncat (q, p, n0);		/* { dg-warning "specified bound
>depends on the length" } */
>+  size_t n1 = __builtin_strlen (p);
>+  *r = n0;
>+  return n1;
>+}
> 
>+int
>+main ()
>+{
>+  char a[8] = "";
>   __builtin_strcpy (a, "123");
>-
>-  unsigned n0 = __builtin_strlen (a);
>-
>-  __builtin_strncat (a + 3, a, n0);
>-
>-  unsigned n1 = __builtin_strlen (a);
>-
>+  size_t n0 = __builtin_strlen (a);
>+  __builtin_strncat (a + 3, a, n0);	/* { dg-warning "specified bound
>depends on the length" } */
>+  size_t n1 = __builtin_strlen (a);
>   if (n1 == n0)
>     __builtin_abort ();
>+  a[6] = '7';
>+  __builtin_strcpy (a, "456");
>+  size_t n2;
>+  if (foo (a, a + 3, &n2) != 6 || n2 != 3)
>+    __builtin_abort ();
>+  if (__builtin_memcmp (a, "456456\0", sizeof "456456\0"))
>+    __builtin_abort ();
>+  return 0;
> }
>
>
>	Jakub

On 12/07/2017 09:55 AM, Jakub Jelinek wrote:
> On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
>> On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
>>> Attached is a patch with the comment updated/simplified.
>>> The tests do the job they need to do today so I just removed
>>> the useless attribute but otherwise left them unchanged.  If
>>> you would like to enhance them in some way please feel free.
>>
>> Ok for trunk, with a minor nit.  I'll tweak the tests incrementally
>> when it is in.
>
> So here is the fix for those testcases.
>
> They didn't test what they meant to test, because they didn't FAIL
> without the patch.  That is because the bug was that the -W* option
> affected code generation, so with -O2 -Wno-stringop-overflow it didn't
> trigger it.

Doh!  I suppose that underscores that the right way to write test
cases for optimization bugs is to prune warnings out of their output
rather than suppressing them via -Wno-foo.  I've done the former
for this very reason a number of times but clearly fell into the
trap of suppressing the warnings in this instance.  Thanks for
pointing it out!

> I've changed the tests to test both in a separate noipa function where
> it doesn't know about the aliasing and string lengths from the caller,
> in that case it does more verifications, including the content of the
> whole buffer, and the individual values of the lengths,
> and what you did before.

I don't have an opinion on the rest of these changes.  I do want
to make one comment about runtime tests.  I fairly regularly run
tests with cross-compilers on the build machine.  This lets me
verify that compile-only tests pass but it doesn't do anything
for tests that need to run.  In fact, with the current mixture
of all kinds of tests in the same directory, it pretty much rules
out drawing any conclusions from test results in this setup.  So
while I appreciate the additional testing done by the runtime
tests, I think ideally, having compile time only tests would be
the baseline requirement and runtime tests would be a separate
layer that would provide additional validation when possible.

Martin

>
> Regtested on x86_64-linux and i686-linux, verified that with the
> r255446 tree-ssa-strlen.c change reverted it FAILs.
>
> Ok for trunk?
>
> 2017-12-07  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/83075
> 	* gcc.dg/tree-ssa/strncpy-2.c: Use size_t instead of unsigned, add
> 	separate function with noipa attribute to also verify behavior when
> 	optimizers don't know the sizes and aliasing, verify resulting sizes
> 	and array content.  Add -Wstringop-overflow to dg-options.
> 	* gcc.dg/tree-ssa/strncat.c: Likewise.
>
> --- gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c.jj	2017-12-06 20:11:54.000000000 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c	2017-12-07 13:31:32.719722416 +0100
> @@ -1,19 +1,35 @@
> -/* PR tree-optimization/83075 - Invalid strncpy optimization
> -   { dg-do run }
> -   { dg-options "-O2 -Wno-stringop-overflow" } */
> +/* PR tree-optimization/83075 - Invalid strncpy optimization */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wstringop-overflow" } */
>
> -int main (void)
> +typedef __SIZE_TYPE__ size_t;
> +
> +__attribute__((noipa)) size_t
> +foo (char *p, char *q, size_t *r)
>  {
> -  char a[8] = "";
> +  size_t n0 = __builtin_strlen (p);
> +  __builtin_strncpy (q, p, n0);		/* { dg-warning "specified bound depends on the length" } */
> +  size_t n1 = __builtin_strlen (p);
> +  *r = n0;
> +  return n1;
> +}
>
> +int
> +main ()
> +{
> +  char a[8] = "";
>    __builtin_strcpy (a, "123");
> -
> -  unsigned n0 = __builtin_strlen (a);
> -
> -  __builtin_strncpy (a + 3, a, n0);
> -
> -  unsigned n1 = __builtin_strlen (a);
> -
> +  size_t n0 = __builtin_strlen (a);
> +  __builtin_strncpy (a + 3, a, n0);	/* { dg-warning "specified bound depends on the length" } */
> +  size_t n1 = __builtin_strlen (a);
>    if (n1 == n0)
>      __builtin_abort ();
> +  a[6] = '7';
> +  __builtin_strcpy (a, "456");
> +  size_t n2;
> +  if (foo (a, a + 3, &n2) != 7 || n2 != 3)
> +    __builtin_abort ();
> +  if (__builtin_memcmp (a, "4564567", sizeof "4564567"))
> +    __builtin_abort ();
> +  return 0;
>  }
> --- gcc/testsuite/gcc.dg/tree-ssa/strncat.c.jj	2017-12-06 20:11:54.000000000 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/strncat.c	2017-12-07 13:31:09.568008365 +0100
> @@ -1,19 +1,35 @@
> -/* PR tree-optimization/83075 - Invalid strncpy optimization
> -   { dg-do run }
> -   { dg-options "-O2 -Wno-stringop-overflow" } */
> +/* PR tree-optimization/83075 - Invalid strncpy optimization */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -Wstringop-overflow" } */
>
> -int main (void)
> +typedef __SIZE_TYPE__ size_t;
> +
> +__attribute__((noipa)) size_t
> +foo (char *p, char *q, size_t *r)
>  {
> -  char a[8] = "";
> +  size_t n0 = __builtin_strlen (p);
> +  __builtin_strncat (q, p, n0);		/* { dg-warning "specified bound depends on the length" } */
> +  size_t n1 = __builtin_strlen (p);
> +  *r = n0;
> +  return n1;
> +}
>
> +int
> +main ()
> +{
> +  char a[8] = "";
>    __builtin_strcpy (a, "123");
> -
> -  unsigned n0 = __builtin_strlen (a);
> -
> -  __builtin_strncat (a + 3, a, n0);
> -
> -  unsigned n1 = __builtin_strlen (a);
> -
> +  size_t n0 = __builtin_strlen (a);
> +  __builtin_strncat (a + 3, a, n0);	/* { dg-warning "specified bound depends on the length" } */
> +  size_t n1 = __builtin_strlen (a);
>    if (n1 == n0)
>      __builtin_abort ();
> +  a[6] = '7';
> +  __builtin_strcpy (a, "456");
> +  size_t n2;
> +  if (foo (a, a + 3, &n2) != 6 || n2 != 3)
> +    __builtin_abort ();
> +  if (__builtin_memcmp (a, "456456\0", sizeof "456456\0"))
> +    __builtin_abort ();
> +  return 0;
>  }
>
>
> 	Jakub
>

On 12/07/2017 11:03 AM, Martin Sebor wrote:
> On 12/07/2017 09:55 AM, Jakub Jelinek wrote:
>> On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
>>> On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
>>>> Attached is a patch with the comment updated/simplified.
>>>> The tests do the job they need to do today so I just removed
>>>> the useless attribute but otherwise left them unchanged.  If
>>>> you would like to enhance them in some way please feel free.
>>>
>>> Ok for trunk, with a minor nit.  I'll tweak the tests incrementally
>>> when it is in.
>>
>> So here is the fix for those testcases.
>>
>> They didn't test what they meant to test, because they didn't FAIL
>> without the patch.  That is because the bug was that the -W* option
>> affected code generation, so with -O2 -Wno-stringop-overflow it didn't
>> trigger it.
> 
> Doh!  I suppose that underscores that the right way to write test
> cases for optimization bugs is to prune warnings out of their output
> rather than suppressing them via -Wno-foo.  I've done the former
> for this very reason a number of times but clearly fell into the
> trap of suppressing the warnings in this instance.  Thanks for
> pointing it out!
> 
>> I've changed the tests to test both in a separate noipa function where
>> it doesn't know about the aliasing and string lengths from the caller,
>> in that case it does more verifications, including the content of the
>> whole buffer, and the individual values of the lengths,
>> and what you did before.
> 
> I don't have an opinion on the rest of these changes.  I do want
> to make one comment about runtime tests.  I fairly regularly run
> tests with cross-compilers on the build machine.  This lets me
> verify that compile-only tests pass but it doesn't do anything
> for tests that need to run.  In fact, with the current mixture
> of all kinds of tests in the same directory, it pretty much rules
> out drawing any conclusions from test results in this setup.  So
> while I appreciate the additional testing done by the runtime
> tests, I think ideally, having compile time only tests would be
> the baseline requirement and runtime tests would be a separate
> layer that would provide additional validation when possible.
So note if you set up a deeper cross compilation environment (using
sysroot) you can cross-compile down to an executable.  Then you ought to
be able to use qemu's user emulation to run the resulting binary.

Alternately, for many embedded targets if you have a cross compilation
environment (including newlib) you can then use the old ISA simulator to
run execution tests.

We don't do those things much anymore, but we certainly have in the past
and knowing how to do so is valuable.

jeff

On Thu, Dec 07, 2017 at 11:03:10AM -0700, Martin Sebor wrote:
> On 12/07/2017 09:55 AM, Jakub Jelinek wrote:
> > On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
> > > On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
> > > > Attached is a patch with the comment updated/simplified.
> > > > The tests do the job they need to do today so I just removed
> > > > the useless attribute but otherwise left them unchanged.  If
> > > > you would like to enhance them in some way please feel free.
> > > 
> > > Ok for trunk, with a minor nit.  I'll tweak the tests incrementally
> > > when it is in.
> > 
> > So here is the fix for those testcases.
> > 
> > They didn't test what they meant to test, because they didn't FAIL
> > without the patch.  That is because the bug was that the -W* option
> > affected code generation, so with -O2 -Wno-stringop-overflow it didn't
> > trigger it.
> 
> Doh!  I suppose that underscores that the right way to write test
> cases for optimization bugs is to prune warnings out of their output

No, warnings shouldn't be pruned unless really necessary.

It really depends on what you are testing and whether you want to include
-W* options, or -Wno-*, or none of them, or -w.  E.g. the last one is
default in gcc.c-torture/{compile,execute}.

If you need some warnings enabled, then better just add dg-warning
for the options, then you verify you get the right warnings and if it is
dg-do run also right execution, etc.

The reason why you can't have -Wno-foo here is that the test doesn't FAIL
without it without the fix.  That should be a standard procedure for every
submitted patch which adds a test testing a fix in the same patch.
With a vanilla compiler test that
make check<whatever> RUNTESTFLAGS=<whatever.exp>=the_test
FAILs and with the patch applied PASSes.  This takes just a couple of
seconds.  Only after this one should bootstrap/regtest it fully.

> I don't have an opinion on the rest of these changes.  I do want
> to make one comment about runtime tests.  I fairly regularly run
> tests with cross-compilers on the build machine.  This lets me
> verify that compile-only tests pass but it doesn't do anything
> for tests that need to run.  In fact, with the current mixture
> of all kinds of tests in the same directory, it pretty much rules
> out drawing any conclusions from test results in this setup.  So
> while I appreciate the additional testing done by the runtime
> tests, I think ideally, having compile time only tests would be
> the baseline requirement and runtime tests would be a separate
> layer that would provide additional validation when possible.

Depends on what is being tested, properly written valid (without UB) runtime
tests are extremely useful, they test more than compile time only tests can
check and even in the future often they will catch bugs in other parts of
the compiler.  One of the reason why I don't like unit testing too much...

And no, I don't think a compile time test should be added if we are already
adding a runtime test that covers it too and better.

	Jakub