Hi Martin, On 9 January 2017 at 04:14, Jeff Law <l...@redhat.com> 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 > >