On 08/24/2017 08:03 AM, Richard Biener wrote:
> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>> test triggered by a recent VRP improvement.  A simple test case
>> that approximates the warning is:
>>
>>   void f (char *d, const char *s, size_t n)
>>   {
>>     if (n > 0 && n <= SIZE_MAX / 2)
>>       n = 0;
>>
>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>   }
>>
>> Since the only valid value of n is zero the call to memcpy can
>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>> the maximum size of the largest object and would surely make
>> the call crash).
>>
>> The important difference between the test case and Bug 81908
>> is that in the latter, the code is emitted by GCC itself from
>> what appears to be correct source (looks like it's the result
>> of the loop distribution pass).  I believe the warning for
>> the test case above and for other human-written code like it
>> is helpful, but warning for code emitted by GCC, even if it's
>> dead or unreachable, is obviously not (at least not to users).
>>
>> The attached patch enhances the gimple_fold_builtin_memory_op
>> function to eliminate this patholohgical case by making use
>> of range information to fold into no-ops calls to memcpy whose
>> size argument is in a range where the only valid value is zero.
>> This gets rid of the warning and improves the emitted code.
>>
>> Tested on x86_64-linux.
>
> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
>    tree destvar, srcvar;
>    location_t loc = gimple_location (stmt);
>
> +  if (tree valid_len = only_valid_value (len))
> +    {
> +      /* LEN is in range whose only valid value is zero.  */
> +      len = valid_len;
> +    }
> +
>    /* If the LEN parameter is zero, return DEST.  */
>    if (integer_zerop (len))
>
> just enhance this check to
>
>   if (integer_zerop (len)
>       || size_must_be_zero_p (len))
>
> ?  'only_valid_value' looks too generic for this.

Sure.

FWIW, the reason I did it this was because my original patch
returned error_mark_node for entirely invalid ranges and had
the caller replace the call with a trap.  I decided not to
include that part in this fix to keep it contained.

>
> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
> +    return NULL_TREE;
> +
>
> why?

Only because I never remember what APIs are safe to use with
what input.

> +  if (wi::eq_p (min, wone)
> +      && wi::geu_p (max + 1, ssize_max))
>
>    if (wi::eq_p (min, 1)
>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>
> your ssize_max isn't signed size max, and max + 1 might overflow to zero.

You're right that the addition to max would be better done
as subtraction from the result of (1 << N).  Thank you.

If (max + 1) overflowed then (max == TYPE_MAX) would have
to hold which I thought could never be true for an anti
range. (The patch includes tests for this case.)  Was I
wrong?

Attached is an updated version with the suggested changes
plus an additional test to verify the absence of warnings.

Martin