[PATCH] Make strlen range computations more conservative

[Bernd Edlinger]

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?

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

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

But we can as well remove those test cases.


Bernd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-pr86121.diff
Type: text/x-patch
Size: 1509 bytes
Desc: patch-pr86121.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180820/b852cbc4/attachment.bin>