Martin Sebor <mse...@gmail.com> writes:
> On 08/24/2017 08:03 AM, Richard Biener wrote:
>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <mse...@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
>
> PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g
> -m32
>
> gcc/ChangeLog:
>
> PR middle-end/81908
> * gimple-fold.c (size_must_be_zero_p): New function.
> (gimple_fold_builtin_memory_op): Call it.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/81908
> * gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
> * gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.
> * gcc.dg/tree-ssa/pr81908.c: New test.
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 251446c..c4184fa 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -628,6 +628,36 @@ var_decl_component_p (tree var)
> return SSA_VAR_P (inner);
> }
>
> +/* If the SIZE argument representing the size of an object is in a range
> + of values of which exactly one is valid (and that is zero), return
> + true, otherwise false. */
> +
> +static bool
> +size_must_be_zero_p (tree size)
> +{
> + if (integer_zerop (size))
> + return true;
> +
> + if (TREE_CODE (size) != SSA_NAME)
> + return false;
> +
> + wide_int min, max;
> + enum value_range_type rtype = get_range_info (size, &min, &max);
> + if (rtype != VR_ANTI_RANGE)
> + return false;
> +
> + tree type = TREE_TYPE (size);
> + int prec = TYPE_PRECISION (type);
> +
> + wide_int wone = wi::one (prec);
> +
> + /* Compute the value of SSIZE_MAX, the largest positive value that
> + can be stored in ssize_t, the signed counterpart of size_t . */
> + wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
> +
> + return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max);
Just a stylistic thing, but since the only use of "wone" is in
the eq_p, it'd be simpler just to use "1". Also, the maximum
value is better calculated as "wi::max_value (prec, SIGNED)". So:
/* Compute the value of SSIZE_MAX, the largest positive value that
can be stored in ssize_t, the signed counterpart of size_t . */
wide_int ssize_max = wi::max_value (prec, SIGNED);
return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);
Thanks,
Richard
