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

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Feb 8, 2018 at 6:35 AM, Jeff Law <law@redhat.com> wrote:
> On 02/06/2018 05:57 AM, 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?
>> 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?
>>
>>> +  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.
> Martin and I wandered down this approach a bit and ultimately decided
> against it.  While yes it could avoid a false positive by looking at the
> immediate uses, but I'm not sure avoiding the false positive in those
> cases is actually good!
>
> THe larger the separation between the strcpy and the truncation the more
> likely it is that the code is wrong or at least poorly written and
> deserves a developer looksie.

But it's the next _GIMPLE_ stmt it looks at.  Make it

char d[3];

  void f (const char *s, int x)
  {
    char d[x];
    __builtin_strncpy (d, s, sizeof d);
    d[x-1] = 0;
  }

and I bet it will again warn since x-1 is a separate GIMPLE stmt.

The patch is of course ok but still...  simply looking at all
immediate uses of the VDEF of the strncpy call, stopping
at the single stmt with the "next" VDEF should be better
(we don't have a next_vdef () helper).

Richard.

> jeff