Hi Martin,
On 9 January 2017 at 04:14, Jeff Law <[email protected]> wrote:
> On 01/08/2017 02:04 PM, Martin Sebor wrote:
>>
>> On 01/06/2017 09:45 AM, Jeff Law wrote:
>>>
>>> On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>>>>>
>>>>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>>>>> imply removal of operand_signed_p.
>>>>>>>
>>>>>>> What are the implications if we do that?
>>>>>>
>>>>>>
>>>>>> I just got back to this yesterday. The implications of the removal
>>>>>> of the anti-range handling are a number of false negatives in the
>>>>>> test suite:
>>>>>
>>>>> I was thinking more at a higher level. ie, are the warnings still
>>>>> useful if we don't have the anti-range handling? I suspect so, but
>>>>> would like to hear your opinion.
>>>>
>>>> ...
>>>>>>
>>>>>> n = ~[-4, MAX]; (I.e., n is either negative or too big.)
>>>>>> p = malloc (n);
>>>>>
>>>>> Understood. The low level question is do we get these kinds of ranges
>>>>> often enough in computations leading to allocation sizes?
>>>>
>>>>
>>>> My intuition tells me that they are likely common enough not to
>>>> disregard but I don't have a lot of data to back it up with. In
>>>> a Bash build a full 23% of all checked calls are of this kind (24
>>>> out of 106). In a Binutils build only 4% are (9 out of 228). In
>>>> Glibc, a little under 3%. My guess is that the number will be
>>>> inversely proportional to the quality of the code.
>>>
>>> So I think you've made a case that we do want to handle this case. So
>>> what's left is how best to avoid the infinite recursion and mitigate the
>>> pathological cases.
>>>
>>> What you're computing seems to be "this object may have been derived
>>> from a signed type". Right? It's a property we can compute for any
>>> given SSA_NAME and it's not context/path specific beyond the
>>> context/path information encoded by the SSA graph.
>>>
>>> So just thinking out load here, could we walk the IL once to identify
>>> call sites and build a worklist of SSA_NAMEs we care about. Then we
>>> iterate on the worklist much like Aldy's code he's working on for the
>>> unswitching vs uninitialized variable issue?
>>
>>
>> Thanks for the suggestion. It occurred to me while working on the fix
>> for 78973 (the non-bug) that size ranges should be handled the same by
>> -Wstringop-overflow as by -Walloc-size-larger-than, and that both have
>> the same problem: missing or incomplete support for anti-ranges. The
>> attached patch moves get_size_range() from builtins.c to calls.c and
>> adds better support for anti-ranges. That solves the problems also
>> lets it get rid of the objectionable operand_positive_p function.
>>
>> Martin
>>
>> PS The change to the alloc_max_size function is only needed to make
>> it possible to specify any argument to the -Walloc-size-larger-than
>> option, including 0 and -1, so that allocations of any size, including
>> zero can be flagged.
>>
>> gcc-78775.diff
>>
>>
>> PR tree-optimization/78775 - [7 Regression] ICE in
>> maybe_warn_alloc_args_overflow
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/78775
>> * builtins.c (get_size_range): Move...
>> * calls.c: ...to here.
>> (alloc_max_size): Accept zero argument.
>> (operand_signed_p): Remove.
>> (maybe_warn_alloc_args_overflow): Call get_size_range.
>> * calls.h (get_size_range): Declare.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/78775
>> * gcc.dg/attr-alloc_size-4.c: Add test cases.
>> * gcc.dg/pr78775.c: New test.
>> * gcc.dg/pr78973-2.c: New test.
>> * gcc.dg/pr78973.c: New test.
>>
>
The new test (gcc.dg/pr78973.c) fails on arm targets (there's no warning).
In addition, I have noticed a new failure:
gcc.dg/attr-alloc_size-4.c (test for warnings, line 140)
on target arm-none-linux-gnueabihf --with-cpu=cortex-a5
(works fine --with-cpu=cortex-a9)
Christophe
>
>> +
>> + wide_int min, max;
>> + enum value_range_type range_type = get_range_info (exp, &min, &max);
>> +
>> + tree exptype = TREE_TYPE (exp);
>> + unsigned expprec = TYPE_PRECISION (exptype);
>> + wide_int wzero = wi::zero (expprec);
>> +
>> + /* Set SIGNED_P once (will be used by recursive calls). */
>> + if (signed_p < 0)
>> + signed_p = !TYPE_UNSIGNED (exptype);
>> +
>> + if (range_type == VR_VARYING)
>> {
>> - gimple *def = SSA_NAME_DEF_STMT (op);
>> - if (is_gimple_assign (def))
>> + /* No range information available. */
>> + range[0] = NULL_TREE;
>> + range[1] = NULL_TREE;
>> + return false;
>> + }
>
> Might as well move this range_type test earlier since it doesn't use
> exptype, expprec, wzero or signed_p.
>
> OK with that change.
>
> jeff
>
>
