This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thu, Feb 8, 2018 at 7:12 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/08/2018 07:39 AM, Richard Biener wrote:
>>
>> 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.
>
>
> It doesn't but only because it doesn't know how to handle VLAs.
>
>> 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).
>
>
> IIRC, one of the problems I ran into was how to handle code
> like this:
>
>   void f (const char *s)
>   {
>     char d[8];
>     char *p = strncpy (d, s, sizeof d);
>
>     foo (p);
>
>     d[7] = 0;
>   }
>
> I.e., having to track all pointers to d between the call to
> strncpy and the assignment of the nul and make sure none of
> them ends up used in a string function.  It didn't seem
> the additional complexity would have been worth the effort
> (or the likely false negatives).

Well, I'd just walk immediate uses of the VDEF of the
strncpy call, not of the pointer argument.  There will be exactly _one_ possible
store (gimple_vdef () is non-NULL) that you need to verify (with using
the current matching
logic).  But it'll skip non-store statements for you.

Richard.

> Martin
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]