On August 24, 2017 11:52:41 PM GMT+02:00, Jeff Law <l...@redhat.com> wrote:
>On 08/24/2017 09:03 AM, Martin Sebor wrote:
>> 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.
>Seems reasonable. Though I would suggest going forward with trap
>replacement for clearly invalid ranges as a follow-up. Once you do
>trap
>replacement, the input operands all become dead as does all the code
>after the trap and the outgoing edges in the CFG.
>
>That often exposes a fair amount of cleanup. Furthermore on targets
>that have conditional traps, the controlling condition often turns into
>a conditional trap. In all, you get a lot of nice cascading effects
>when you turn something that is clearly bogus into a trap.
>
>gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
>need, including a code you can crib to detect PHI arguments which would
>cause bogus behavior and allow you to isolate that specific path.
>
>
>
>>
>>>
>>> + 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?
>Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]? Or am I
>misunderstanding something.
>
>>
>> Attached is an updated version with the suggested changes
>> plus an additional test to verify the absence of warnings.
>The patch is OK.
>
>I'll note this is another use of anti-ranges. I'd really like to see
>Aldy's work on the new range API move forward and get rid of
>anti-ranges.
Note that anti-ranges are not complicated to handle. In fact the current API
could be trivially extended to return two ranges for them. The complication
will be in the callers which will have to handle the unioning.
I'd like to get rid of anti-ranges in VRP and mainly for the reason that
handling unioning of N ranges will increase precision. I do not see too many
uses of the increased precision outside of VRP which is where Aldhys work is
solely residing.
Richard.
>THanks,
>jeff
