This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Make strlen range computations more conservative



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.
2018-08-20  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/86121
	* tree-ssa-strlen.c (adjust_last_stmt): Avoid folding away undefined
	behaviour.

testsuite:
2018-08-20  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/86121
	* gcc.dg/Wstringop-overflow-6.c: Remove xfail.

diff -ur gcc/testsuite/gcc.dg/Wstringop-overflow-6.c gcc/testsuite/gcc.dg/Wstringop-overflow-6.c
--- gcc/testsuite/gcc.dg/Wstringop-overflow-6.c	2018-06-12 20:05:13.000000000 +0200
+++ gcc/testsuite/gcc.dg/Wstringop-overflow-6.c	2018-08-20 14:53:55.605350343 +0200
@@ -25,7 +25,7 @@
 
 void test_strcpy_strcat_2 (void)
 {
-  strcpy (a2, "12"), strcat (a2, "3");   /* { dg-warning "\\\[-Wstringop-overflow=]" "bug 86121" { xfail *-*-* } } */
+  strcpy (a2, "12"), strcat (a2, "3");   /* { dg-warning "\\\[-Wstringop-overflow=]" } */
 }
 
 void test_strcpy_strcat_3 (void)
diff -ur gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
--- gcc/tree-ssa-strlen.c	2018-08-02 01:39:35.000000000 +0200
+++ gcc/tree-ssa-strlen.c	2018-08-20 12:41:23.352955874 +0200
@@ -1107,6 +1107,13 @@
 	 to store the extra '\0' in that case.  */
       if ((tree_to_uhwi (len) & 3) == 0)
 	return;
+
+      /* Don't fold away an out of bounds access, as this defeats proper
+	 warnings.  */
+      tree dst = gimple_call_arg (last.stmt, 0);
+      tree size = compute_objsize (dst, 0);
+      if (size && tree_int_cst_lt (size, len))
+	return;
     }
   else if (TREE_CODE (len) == SSA_NAME)
     {

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]