Re: [PATCH] make -Wrestrict for strcat more meaningful (PR 83698)

[Martin Sebor <msebor@gmail.com>]

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[];
             ^