Re: PATCH to fix bogus warning with -Wstringop-truncation -g (PR tree-optimization/84228)

[Martin Sebor <msebor at gmail dot com>]

On 02/06/2018 06:23 AM, Marek Polacek wrote:
> On Tue, Feb 06, 2018 at 01:57:36PM +0100, Jakub Jelinek wrote:
> > On Tue, Feb 06, 2018 at 01:46:21PM +0100, Marek Polacek wrote:
> > > --- gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
> > > +++ gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
> > > @@ -0,0 +1,20 @@
> > > +/* PR tree-optimization/84228 */
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-Wstringop-truncation -O2 -g" } */
> > > +
> > > +char *strncpy (char *, const char *, __SIZE_TYPE__);
> > > +struct S
> > > +{
> > > +  char arr[64];
> > > +};
> > > +
> > > +int
> > > +foo (struct S *p1, const char *a)
> > > +{
> > > +  if (a)
> > > +    goto err;
> > > +  strncpy (p1->arr, a, sizeof p1->arr); /* { dg-bogus "specified bound" } */
> >
> > Just curious, what debug stmt is in between those?
>
> Just
>
>   strncpy (_1, 0B, 64);
>   # DEBUG BEGIN_STMT
>   p1_5(D)->arr[3] = 0;
>
> > Wouldn't it be better to force them a couple of debug stmts?
> > Say
> >   int b = 5, c = 6, d = 7;
> > at the start of the function and
> >   b = 8; c = 9; d = 10;
> > in between strncpy and the '\0' store?
>
> Yep.  I tweaked the testcase.  Now we have
>
>   strncpy (_1, 0B, 64);
>   # DEBUG BEGIN_STMT
>   # DEBUG b => 8
>   # DEBUG BEGIN_STMT
>   # DEBUG c => 9
>   # DEBUG BEGIN_STMT
>   # DEBUG d => 10
>   # DEBUG BEGIN_STMT
>   p1_5(D)->arr[3] = 0;
>
> > > +  p1->arr[3] = '\0';
> > > +err:
> > > +  return 0;
> > > +}
> > > diff --git gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
> > > index c3cf432a921..f0f6535017b 100644
> > > --- gcc/tree-ssa-strlen.c
> > > +++ gcc/tree-ssa-strlen.c
> > > @@ -1849,7 +1849,7 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
> > >
> > >    /* Look for dst[i] = '\0'; after the stxncpy() call and if found
> > >       avoid the truncation warning.  */
> > > -  gsi_next (&gsi);
> > > +  gsi_next_nondebug (&gsi);
> > >    gimple *next_stmt = gsi_stmt (gsi);
> > >
> > >    if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
> >
> > Ok for trunk, though generally looking at just next stmt is very fragile, might be
> > better to look at the strncpy's vuse immediate uses if they are within the
> > same basic block and either don't alias with it, or are the store it is
> > looking for, or something similar.
>
> I guess some
> FOR_EACH_IMM_USE_FAST (...)
>   {
>     if (is_gimple_debug (USE_STMT (use_p)))
>       continue;
> ...
> would be better.

Jeff and I had a fairly extensive discussion about how best to
handle debug statements.  I had prototyped his suggestion along
these lines but ran into false positives and other difficulties.
The details are here:

  https://gcc.gnu.org/ml/gcc-patches/2017-11/msg00805.html

I thought I ended up just using gsi_next_nondebug() and added
a test for it but I guess I must be misremembering.  The patch
review spanned a couple of months so it's even possible I forgot
to make the change.

Martin


> Thanks,
>
> 2018-02-06  Marek Polacek  <polacek@redhat.com>
>
> 	PR tree-optimization/84228
> 	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Skip debug statements.
>
> 	* c-c++-common/Wstringop-truncation-3.c: New test.
>
> diff --git gcc/testsuite/c-c++-common/Wstringop-truncation-3.c gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
> index e69de29bb2d..ba6b7de094b 100644
> --- gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
> +++ gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
> @@ -0,0 +1,22 @@
> +/* PR tree-optimization/84228 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wstringop-truncation -O2 -g" } */
> +
> +char *strncpy (char *, const char *, __SIZE_TYPE__);
> +struct S
> +{
> +  char arr[64];
> +};
> +
> +int
> +foo (struct S *p1, const char *a)
> +{
> +  int b = 5, c = 6, d = 7;
> +  if (a)
> +    goto err;
> +  strncpy (p1->arr, a, sizeof p1->arr); /* { dg-bogus "specified bound" } */
> +  b = 8; c = 9; d = 10;
> +  p1->arr[3] = '\0';
> +err:
> +  return 0;
> +}
> diff --git gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
> index c3cf432a921..f0f6535017b 100644
> --- gcc/tree-ssa-strlen.c
> +++ gcc/tree-ssa-strlen.c
> @@ -1849,7 +1849,7 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
>
>    /* Look for dst[i] = '\0'; after the stxncpy() call and if found
>       avoid the truncation warning.  */
> -  gsi_next (&gsi);
> +  gsi_next_nondebug (&gsi);
>    gimple *next_stmt = gsi_stmt (gsi);
>
>    if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
>
> 	Marek