[PATCH] Make strlen range computations more conservative

[Richard Biener]

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.

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.

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

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?

There do seem to be independently useful fixes in the patch that
I'd approve immediately.

Btw, I don't think we want sth like 
flag_assume_zero_terminated_char_arrays or even make it default at
-Ofast.

Richard.