On Mon, 20 Aug 2018, Bernd Edlinger wrote: > > > On 08/20/18 12:23, Richard Biener wrote: > > On Sun, 19 Aug 2018, Bernd Edlinger wrote: > > > >> Hi, > >> > >> > >> I rebased my range computation patch to current trunk, > >> and updated it according to what was discussed here. > >> > >> That means get_range_strlen has already a parameter > >> that is used to differentiate between ranges for warnings > >> and ranges for code-gen. > >> > >> That is called "strict", in the 4-parameter overload > >> and "fuzzy" in the internally used 7-parameter overload. > >> > >> So I added an "optimistic" parameter to my > >> get_inner_char_array_unless_typecast helper function. > >> That's it. > >> > >> Therefore at this time, there is only one warning regression > >> in one test case and one xfailed warning test case fixed. > >> > >> So that is par on the warning regression side. > >> > >> The failed test case is gcc/testsuite/gcc.dg/pr83373.c which > >> uses -fassume-zero-terminated-char-arrays, to enable the > >> (unsafe) feedback from string-length information to VRP to > >> suppress the warning. > >> > >> The 5 test cases that were designed to check the optimized > >> tree dump have to use the new -fassume-zero-terminated-char-arrays > >> option, but that is what we agreed upon. > >> > >> The patch is not dependent on any other patches. > >> > >> > >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > >> Is it OK for trunk? > > > > + tree base = arg; > > + while (TREE_CODE (base) == ARRAY_REF > > + || TREE_CODE (base) == ARRAY_RANGE_REF > > + || TREE_CODE (base) == COMPONENT_REF) > > + base = TREE_OPERAND (base, 0); > > + > > + /* If this looks like a type cast don't assume anything. */ > > + if ((TREE_CODE (base) == MEM_REF > > + && (! integer_zerop (TREE_OPERAND (base, 1)) > > + || TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (TREE_OPERAND (base, > > 0)))) > > + != TYPE_MAIN_VARIANT (TREE_TYPE (base)))) > > > > I'm not convinced you are testing anything useful here. > > TREE_OPERAND (base, 1) might be a pointer which means it's type > > doesn't have any semantics so you are testing the access type > > against "random". If you'd restrict this to ADDR_EXPRs and > > look at the objects declared type then you'd still miss > > type-changes from a dynamic type that is different from what > > is declared. > > > > This whole function is only used for warnings or if the test case > asks for unsafe optimization via -ffassume-zero-terminated-char-arrays. > > So yes, it is understood, but it has proven to be an oracle with > 99.9% likelihood to give the right answer. > > > So my conclusion is if you really want to not want to return > > arg for things that look like a type cast then you have to > > unconditionally return NULL_TREE. > > Yes. :-) > > > > > + || TREE_CODE (base) == VIEW_CONVERT_EXPR > > + /* Or other stuff that would be handled by get_inner_reference. */ > > > > simply use || handled_component_p (base) for the above and the rest > > to be sure to handle everything that is not stripped above. > > > > + || TREE_CODE (base) == BIT_FIELD_REF > > + || TREE_CODE (base) == REALPART_EXPR > > + || TREE_CODE (base) == IMAGPART_EXPR) > > + return NULL_TREE; > > > > Yes, good point. > > > Btw, you are always returning the passed arg or NULL_TREE so > > formulating this as a predicate function makes uses easier. > > Not sure why it is called "inner" char array? > > > ; > > Yes, in a previous version of this patch, this function actually > walked towards the innermost array, and returned that, but I dropped > that part, as it caused too many test cases regress. > > So agreed, I think I will convert that to a _p function and think > of a better name. > > > > There do seem to be independently useful fixes in the patch that > > I'd approve immediately. > > > > Yes, I found some peanuts on my way. > > For instance this fix for PR middle-end/86121 survives bootstrap on > it's own, and fixes one xfail. > > Is it OK for trunk?
Yes, that's OK for trunk. > > Btw, I don't think we want sth like > > flag_assume_zero_terminated_char_arrays or even make it default at > > -Ofast. > > > > Yes, I agree. Is there a consensus about this? Well, it's my own opinion of course. Show me a benchmark that improves with -fassume-zero-terminated-char-arrays. Certainly for security reasons it sounds a dangerous thing (and the documentation needs a more thorough description of what it really means). > If yes, I go ahead and remove that option again. > > BTW: I needed this option in a few test cases, that insist in checking the > optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so). Any example? > But we can as well remove those test cases. > > Bernd.
