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

[Jeff Law]

On 02/06/2018 08:59 AM, Martin Sebor wrote:
> 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.
Right.  I remember that being the ultimate decision (gsi_next_nondebug).
 Perhaps the patch got lost in the long cycle times.

Jeff