[PING #2] [PATCH] make -Wrestrict for strcat more meaningful (PR 83698)

[Martin Sebor]

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html

As I mentioned, this doesn't solve a regression per se but
rather implements what I consider an important usability
improvement to the -Wrestrict warnings.  Printing offsets
that are the most meaningful makes debugging the problems
the warnings point out easier.

On 01/30/2018 10:24 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html
>
> This is a minor improvement but there have been a couple of
> comments lately pointing out that the numbers in the -Wrestrict
> messages can make them confusing.
>
> Jakub, since you made one of those comments (the other came up
> in the context of bug 84095 - [8 Regression] false-positive
> -Wrestrict warnings for memcpy within array).  Can you please
> either approve this patch or suggest changes?
>
> On 01/16/2018 05:35 PM, Martin Sebor wrote:
>> On 01/16/2018 02:32 PM, Jakub Jelinek wrote:
>>> On Tue, Jan 16, 2018 at 01:36:26PM -0700, Martin Sebor wrote:
>>>> --- gcc/gimple-ssa-warn-restrict.c    (revision 256752)
>>>> +++ gcc/gimple-ssa-warn-restrict.c    (working copy)
>>>> @@ -384,6 +384,12 @@ builtin_memref::builtin_memref (tree expr, tree si
>>>>        base = SSA_NAME_VAR (base);
>>>>        }
>>>>
>>>> +  if (DECL_P (base) && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE)
>>>> +    {
>>>> +      if (offrange[0] < 0 && offrange[1] > 0)
>>>> +    offrange[0] = 0;
>>>> +    }
>>>
>>> Why the 2 nested ifs?
>>
>> No particular reason.  There may have been more code in there
>> that I ended up removing.  Or a comment.  I can remove the
>> extra braces when the patch is approved.
>>
>>>
>>>> @@ -1079,14 +1085,35 @@ builtin_access::strcat_overlap ()
>>>>      return false;
>>>>
>>>>    /* When strcat overlap is certain it is always a single byte:
>>>> -     the terminatinn NUL, regardless of offsets and sizes.  When
>>>> +     the terminating NUL, regardless of offsets and sizes.  When
>>>>       overlap is only possible its range is [0, 1].  */
>>>>    acs.ovlsiz[0] = dstref->sizrange[0] == dstref->sizrange[1] ? 1 : 0;
>>>>    acs.ovlsiz[1] = 1;
>>>> -  acs.ovloff[0] = (dstref->sizrange[0] +
>>>> dstref->offrange[0]).to_shwi ();
>>>> -  acs.ovloff[1] = (dstref->sizrange[1] +
>>>> dstref->offrange[1]).to_shwi ();
>>>
>>> You use to_shwi many times in the patch, do the callers or something
>>> earlier
>>> in this function guarantee that you aren't throwing away any bits
>>> (unlike
>>> tree_to_shwi, to_shwi method doesn't ICE, just throws away upper bits).
>>> Especially when you perform additions like here, even if both
>>> wide_ints fit
>>> into a shwi, the result might not.
>>
>> No, I'm not sure.  In fact, it wouldn't surprise me if it did
>> happen.  It doesn't cause false positives or negatives but it
>> can make the offsets less than meaningful in cases where they
>> are within valid bounds.  There are also cases where they are
>> meaningless to begin with and there is little the pass can do
>> about that.
>>
>> IMO, the ideal solution to the first problem is to add a format
>> specifier for wide ints to the pretty printer and get rid of
>> the conversions.  It's probably too late for something like
>> that now but I'd like to do it for GCC 9.  Unless someone
>> files a bug/regression, it's also too late for me to go and
>> try to find and fix these conversions now.
>>
>> Martin
>>
>> PS While looking for a case you asked about I came up with
>> the following.  I don't think there's any slicing involved
>> but the offsets are just as meaningless as if there were.
>> I think the way to do significantly better is to detect
>> out-of-bounds offsets earlier (e.g., as in this patch:
>> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02143.html)
>>
>> $ cat z.c && gcc -O2 -S -Warray-bounds -m32 z.c
>> extern int a[];
>>
>> void f (__PTRDIFF_TYPE__ i)
>> {
>>   if (i < __PTRDIFF_MAX__ - 7 || __PTRDIFF_MAX__ - 5 < i)
>>     i = __PTRDIFF_MAX__ -  7;
>>
>>   const int *s = a + i;
>>
>>   __builtin_memcpy (a, &s[i], 3);
>> }
>> z.c: In function ‘f’:
>> z.c:10:3: warning: ‘__builtin_memcpy’ offset [-64, -48] is out of the
>> bounds of object ‘a’ with type ‘int[]’ [-Warray-bounds]
>>    __builtin_memcpy (a, &s[i], 3);
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> z.c:1:12: note: ‘a’ declared here
>>  extern int a[];
>>             ^
>>
>