[PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

Jeff Law law@redhat.com
Mon Jan 9 03:14:00 GMT 2017


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.
>


> +
> +  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




More information about the Gcc-patches mailing list