diff mbox

Add workaround for PR64715

Message ID 20150325183832.GQ1746@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek March 25, 2015, 6:38 p.m. UTC
Hi!

As discussed in the PR, fixing this issue for real (make sure we at least
until the objsz pass don't lose information on which field's address if any
has been taken) is probably too dangerous at this point, so this patch
just adds a simple workaround:
another objsz pass instance run early before first ccp pass, in which we
only process __bos (x, 1) and __bos (x, 3), and rather than folding them
right away we instead just replace say
  _1 = __builtin_object_size (ptr_2, 1);
with
  _7 = __builtin_object_size (ptr_2, 1);
  _1 = MIN <_7, 17>;
if 17 is what the __builtin_object_size folds to.  The reason for the MIN or
MAX is that later DCE etc. could still make the value smaller later on
(as shown in the third snippet of __builtin_object_size).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

For GCC 6 will need to write some real fix and revert this (except for the
testcases).

2015-03-25  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/64715
	* passes.def: Add another instance of pass_object_sizes before
	ccp1.
	* tree-object-size.c (pass_object_sizes::execute): In
	first_pass_instance, only handle __bos (, 1) and __bos (, 3)
	calls, and keep the call in the IL, as {MIN,MAX}_EXPR of the
	__bos result and the computed constant.  Remove redundant
	checks, obsoleted by gimple_call_builtin_p test.  When propagating
	folded __bos into uses, if the use is {MIN,MAX}_EXPR we can fold
	into constant, propagate even that constant into their uses.

	* gcc.dg/builtin-object-size-15.c: New test.
	* gcc.dg/pr64715-1.c: New test.
	* gcc.dg/pr64715-2.c: New test.


	Jakub

Comments

Richard Biener March 26, 2015, 8:33 a.m. UTC | #1
On Wed, 25 Mar 2015, Jakub Jelinek wrote:

> Hi!
> 
> As discussed in the PR, fixing this issue for real (make sure we at least
> until the objsz pass don't lose information on which field's address if any
> has been taken) is probably too dangerous at this point, so this patch
> just adds a simple workaround:
> another objsz pass instance run early before first ccp pass, in which we
> only process __bos (x, 1) and __bos (x, 3), and rather than folding them
> right away we instead just replace say
>   _1 = __builtin_object_size (ptr_2, 1);
> with
>   _7 = __builtin_object_size (ptr_2, 1);
>   _1 = MIN <_7, 17>;
> if 17 is what the __builtin_object_size folds to.  The reason for the MIN or
> MAX is that later DCE etc. could still make the value smaller later on
> (as shown in the third snippet of __builtin_object_size).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Looks good to me besides...

> For GCC 6 will need to write some real fix and revert this (except for the
> testcases).
> 
> 2015-03-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/64715
> 	* passes.def: Add another instance of pass_object_sizes before
> 	ccp1.
> 	* tree-object-size.c (pass_object_sizes::execute): In
> 	first_pass_instance, only handle __bos (, 1) and __bos (, 3)
> 	calls, and keep the call in the IL, as {MIN,MAX}_EXPR of the
> 	__bos result and the computed constant.  Remove redundant
> 	checks, obsoleted by gimple_call_builtin_p test.  When propagating
> 	folded __bos into uses, if the use is {MIN,MAX}_EXPR we can fold
> 	into constant, propagate even that constant into their uses.
> 
> 	* gcc.dg/builtin-object-size-15.c: New test.
> 	* gcc.dg/pr64715-1.c: New test.
> 	* gcc.dg/pr64715-2.c: New test.
> 
> --- gcc/passes.def.jj	2015-01-19 14:40:46.000000000 +0100
> +++ gcc/passes.def	2015-03-25 12:18:21.079207954 +0100
> @@ -77,6 +77,7 @@ along with GCC; see the file COPYING3.
>        PUSH_INSERT_PASSES_WITHIN (pass_all_early_optimizations)
>  	  NEXT_PASS (pass_remove_cgraph_callee_edges);
>  	  NEXT_PASS (pass_rename_ssa_copies);
> +	  NEXT_PASS (pass_object_sizes);
>  	  NEXT_PASS (pass_ccp);
>  	  /* After CCP we rewrite no longer addressed locals into SSA
>  	     form if possible.  */
> --- gcc/tree-object-size.c.jj	2015-03-20 17:58:31.000000000 +0100
> +++ gcc/tree-object-size.c	2015-03-25 14:40:03.664185560 +0100
> @@ -1268,25 +1268,60 @@ pass_object_sizes::execute (function *fu
>  	    continue;
>  
>  	  init_object_sizes ();
> +
> +	  /* In the first pass instance, only attempt to fold
> +	     __builtin_object_size (x, 1) and __builtin_object_size (x, 3),
> +	     and rather than folding the builtin to the constant if any,
> +	     create a MIN_EXPR or MAX_EXPR of the __builtin_object_size
> +	     call result and the computed constant.  */
> +	  if (first_pass_instance)
> +	    {
> +	      tree ost = gimple_call_arg (call, 1);
> +	      if (tree_fits_uhwi_p (ost))
> +		{
> +		  unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
> +		  tree ptr = gimple_call_arg (call, 0);
> +		  tree lhs = gimple_call_lhs (call);
> +		  if ((object_size_type == 1 || object_size_type == 3)
> +		      && (TREE_CODE (ptr) == ADDR_EXPR
> +			  || TREE_CODE (ptr) == SSA_NAME)
> +		      && lhs)
> +		    {
> +		      tree type = TREE_TYPE (lhs);
> +		      unsigned HOST_WIDE_INT bytes
> +			= compute_builtin_object_size (ptr, object_size_type);
> +		      if (bytes != (unsigned HOST_WIDE_INT) (object_size_type == 1
> +							     ? -1 : 0)
> +			  && wi::fits_to_tree_p (bytes, type))
> +			{
> +			  tree tem = make_ssa_name (type);
> +			  gimple_call_set_lhs (call, tem);
> +			  enum tree_code code
> +			    = object_size_type == 1 ? MIN_EXPR : MAX_EXPR;
> +			  tree cst = build_int_cstu (type, bytes);
> +			  gimple g = gimple_build_assign (lhs, code, tem, cst);
> +			  gsi_insert_after (&i, g, GSI_NEW_STMT);
> +			  update_stmt (call);
> +			}
> +		    }
> +		}
> +	      continue;
> +	    }
> +
>  	  result = fold_call_stmt (as_a <gcall *> (call), false);
>  	  if (!result)
>  	    {
> -	      if (gimple_call_num_args (call) == 2
> -		  && POINTER_TYPE_P (TREE_TYPE (gimple_call_arg (call, 0))))
> -		{
> -		  tree ost = gimple_call_arg (call, 1);
> +	      tree ost = gimple_call_arg (call, 1);
>  
> -		  if (tree_fits_uhwi_p (ost))
> -		    {
> -		      unsigned HOST_WIDE_INT object_size_type
> -			= tree_to_uhwi (ost);
> +	      if (tree_fits_uhwi_p (ost))
> +		{
> +		  unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
>  
> -		      if (object_size_type < 2)
> -			result = fold_convert (size_type_node,
> -					       integer_minus_one_node);
> -		      else if (object_size_type < 4)
> -			result = build_zero_cst (size_type_node);
> -		    }
> +		  if (object_size_type < 2)
> +		    result = fold_convert (size_type_node,
> +					   integer_minus_one_node);
> +		  else if (object_size_type < 4)
> +		    result = build_zero_cst (size_type_node);
>  		}
>  
>  	      if (!result)
> @@ -1317,8 +1352,37 @@ pass_object_sizes::execute (function *fu
>  	      FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
>  		SET_USE (use_p, result);
>  	      gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
> +	      enum tree_code use_code = ERROR_MARK;
> +	      if (is_gimple_assign (use_stmt))
> +		use_code = gimple_assign_rhs_code (use_stmt);
>  	      fold_stmt (&gsi);
> -	      update_stmt (gsi_stmt (gsi));
> +	      use_stmt = gsi_stmt (gsi);
> +	      if (use_stmt)
> +		{
> +		  update_stmt (use_stmt);
> +		  /* objsz1 pass might produce MIN or MAX on the result.
> +		     If we manage to optimize them into INTEGER_CSTs,
> +		     propagate even those into all uses and fold those
> +		     stmts.  */
> +		  if ((use_code == MIN_EXPR || use_code == MAX_EXPR)
> +		      && is_gimple_assign (use_stmt)
> +		      && gimple_assign_rhs_code (use_stmt) == INTEGER_CST)
> +		    {
> +		      imm_use_iterator iter2;
> +		      tree lhs2 = gimple_assign_lhs (use_stmt);
> +		      tree rhs = gimple_assign_rhs1 (use_stmt);
> +		      FOR_EACH_IMM_USE_STMT (use_stmt, iter2, lhs2)
> +			{
> +			  FOR_EACH_IMM_USE_ON_STMT (use_p, iter2)
> +			    SET_USE (use_p, rhs);
> +			  gsi = gsi_for_stmt (use_stmt);
> +			  fold_stmt (&gsi);
> +			  use_stmt = gsi_stmt (gsi);
> +			  if (use_stmt)
> +			    update_stmt (use_stmt);
> +			}
> +		    }
> +		}

this hunk which I think is not really necessary given that
the late object-size pass now runs right before FRE which
performs constant propagation.  My dev tree has the existing
code simplified to

          /* Propagate into all uses.  */
          replace_uses_by (lhs, result);

but I guess we could now as well replace the __bos stmt via
update_call_from_tree.

Thus, ok without the extra propagation into min/max and with
or without cleanup opportunities I poitned out.

Thanks,
Richard.

>  	    }
>  	}
>      }
> --- gcc/testsuite/gcc.dg/builtin-object-size-15.c.jj	2015-03-25 13:01:46.735777306 +0100
> +++ gcc/testsuite/gcc.dg/builtin-object-size-15.c	2015-03-25 14:13:24.307094194 +0100
> @@ -0,0 +1,27 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +extern void abort (void);
> +
> +int
> +main ()
> +{
> +  struct A { char buf1[9]; char buf2[1]; } a;
> +
> +  if (__builtin_object_size (a.buf1 + (0 + 4), 1) != 5)
> +    abort ();
> +  char *p = a.buf1;
> +  p += 1;
> +  p += 3;
> +  if (__builtin_object_size (p, 1) != 5)
> +    abort ();
> +  p = (char *) &a;
> +  char *q = p + 1;
> +  char *r = q + 3;
> +  char *t = r;
> +  if (r != (char *) &a + 4)
> +    t = (char *) &a + 1;
> +  if (__builtin_object_size (t, 1) != 6)
> +    abort ();
> +  return 0;
> +}
> --- gcc/testsuite/gcc.dg/pr64715-1.c.jj	2015-03-25 13:42:15.369350086 +0100
> +++ gcc/testsuite/gcc.dg/pr64715-1.c	2015-03-25 14:15:00.477536803 +0100
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/64715 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +extern inline __attribute__ ((always_inline, gnu_inline, artificial, nothrow, leaf)) char *
> +strcpy (char *__restrict dest, const char *__restrict src)
> +{
> +  return __builtin___strcpy_chk (dest, src, __builtin_object_size (dest, 2 > 1));
> +}
> +
> +const char *str1 = "JIHGFEDCBA";
> +void bar (char *);
> +
> +void
> +foo ()
> +{
> +  struct A { char buf1[9]; char buf2[1]; } a;
> +  strcpy (a.buf1 + (0 + 4), str1 + 5);
> +  bar ((char *) &a);
> +}
> +
> +/* { dg-final { scan-tree-dump "__builtin___strcpy_chk\[^;\n\r\]*, 5\\\);" "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> --- gcc/testsuite/gcc.dg/pr64715-2.c.jj	2015-03-25 14:46:18.453113325 +0100
> +++ gcc/testsuite/gcc.dg/pr64715-2.c	2015-03-25 14:47:26.093017440 +0100
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/64715 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-objsz2" } */
> +
> +void bar (char *, int);
> +
> +void
> +foo (int x)
> +{
> +  char p[16], *q;
> +  q = p;
> +  if (x)
> +    q = p + 3;
> +  __builtin___strcpy_chk (q, "abcdefghijkl", __builtin_object_size (q, 1));
> +  bar (p, x);
> +}
> +
> +/* { dg-final { scan-tree-dump "__builtin_memcpy \\\(\[^;\n\r\]*, \"abcdefghijkl\", 13\\\);" "objsz2" } } */
> +/* { dg-final { cleanup-tree-dump "objsz2" } } */
> 
> 	Jakub
> 
>
diff mbox

Patch

--- gcc/passes.def.jj	2015-01-19 14:40:46.000000000 +0100
+++ gcc/passes.def	2015-03-25 12:18:21.079207954 +0100
@@ -77,6 +77,7 @@  along with GCC; see the file COPYING3.
       PUSH_INSERT_PASSES_WITHIN (pass_all_early_optimizations)
 	  NEXT_PASS (pass_remove_cgraph_callee_edges);
 	  NEXT_PASS (pass_rename_ssa_copies);
+	  NEXT_PASS (pass_object_sizes);
 	  NEXT_PASS (pass_ccp);
 	  /* After CCP we rewrite no longer addressed locals into SSA
 	     form if possible.  */
--- gcc/tree-object-size.c.jj	2015-03-20 17:58:31.000000000 +0100
+++ gcc/tree-object-size.c	2015-03-25 14:40:03.664185560 +0100
@@ -1268,25 +1268,60 @@  pass_object_sizes::execute (function *fu
 	    continue;
 
 	  init_object_sizes ();
+
+	  /* In the first pass instance, only attempt to fold
+	     __builtin_object_size (x, 1) and __builtin_object_size (x, 3),
+	     and rather than folding the builtin to the constant if any,
+	     create a MIN_EXPR or MAX_EXPR of the __builtin_object_size
+	     call result and the computed constant.  */
+	  if (first_pass_instance)
+	    {
+	      tree ost = gimple_call_arg (call, 1);
+	      if (tree_fits_uhwi_p (ost))
+		{
+		  unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
+		  tree ptr = gimple_call_arg (call, 0);
+		  tree lhs = gimple_call_lhs (call);
+		  if ((object_size_type == 1 || object_size_type == 3)
+		      && (TREE_CODE (ptr) == ADDR_EXPR
+			  || TREE_CODE (ptr) == SSA_NAME)
+		      && lhs)
+		    {
+		      tree type = TREE_TYPE (lhs);
+		      unsigned HOST_WIDE_INT bytes
+			= compute_builtin_object_size (ptr, object_size_type);
+		      if (bytes != (unsigned HOST_WIDE_INT) (object_size_type == 1
+							     ? -1 : 0)
+			  && wi::fits_to_tree_p (bytes, type))
+			{
+			  tree tem = make_ssa_name (type);
+			  gimple_call_set_lhs (call, tem);
+			  enum tree_code code
+			    = object_size_type == 1 ? MIN_EXPR : MAX_EXPR;
+			  tree cst = build_int_cstu (type, bytes);
+			  gimple g = gimple_build_assign (lhs, code, tem, cst);
+			  gsi_insert_after (&i, g, GSI_NEW_STMT);
+			  update_stmt (call);
+			}
+		    }
+		}
+	      continue;
+	    }
+
 	  result = fold_call_stmt (as_a <gcall *> (call), false);
 	  if (!result)
 	    {
-	      if (gimple_call_num_args (call) == 2
-		  && POINTER_TYPE_P (TREE_TYPE (gimple_call_arg (call, 0))))
-		{
-		  tree ost = gimple_call_arg (call, 1);
+	      tree ost = gimple_call_arg (call, 1);
 
-		  if (tree_fits_uhwi_p (ost))
-		    {
-		      unsigned HOST_WIDE_INT object_size_type
-			= tree_to_uhwi (ost);
+	      if (tree_fits_uhwi_p (ost))
+		{
+		  unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
 
-		      if (object_size_type < 2)
-			result = fold_convert (size_type_node,
-					       integer_minus_one_node);
-		      else if (object_size_type < 4)
-			result = build_zero_cst (size_type_node);
-		    }
+		  if (object_size_type < 2)
+		    result = fold_convert (size_type_node,
+					   integer_minus_one_node);
+		  else if (object_size_type < 4)
+		    result = build_zero_cst (size_type_node);
 		}
 
 	      if (!result)
@@ -1317,8 +1352,37 @@  pass_object_sizes::execute (function *fu
 	      FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
 		SET_USE (use_p, result);
 	      gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
+	      enum tree_code use_code = ERROR_MARK;
+	      if (is_gimple_assign (use_stmt))
+		use_code = gimple_assign_rhs_code (use_stmt);
 	      fold_stmt (&gsi);
-	      update_stmt (gsi_stmt (gsi));
+	      use_stmt = gsi_stmt (gsi);
+	      if (use_stmt)
+		{
+		  update_stmt (use_stmt);
+		  /* objsz1 pass might produce MIN or MAX on the result.
+		     If we manage to optimize them into INTEGER_CSTs,
+		     propagate even those into all uses and fold those
+		     stmts.  */
+		  if ((use_code == MIN_EXPR || use_code == MAX_EXPR)
+		      && is_gimple_assign (use_stmt)
+		      && gimple_assign_rhs_code (use_stmt) == INTEGER_CST)
+		    {
+		      imm_use_iterator iter2;
+		      tree lhs2 = gimple_assign_lhs (use_stmt);
+		      tree rhs = gimple_assign_rhs1 (use_stmt);
+		      FOR_EACH_IMM_USE_STMT (use_stmt, iter2, lhs2)
+			{
+			  FOR_EACH_IMM_USE_ON_STMT (use_p, iter2)
+			    SET_USE (use_p, rhs);
+			  gsi = gsi_for_stmt (use_stmt);
+			  fold_stmt (&gsi);
+			  use_stmt = gsi_stmt (gsi);
+			  if (use_stmt)
+			    update_stmt (use_stmt);
+			}
+		    }
+		}
 	    }
 	}
     }
--- gcc/testsuite/gcc.dg/builtin-object-size-15.c.jj	2015-03-25 13:01:46.735777306 +0100
+++ gcc/testsuite/gcc.dg/builtin-object-size-15.c	2015-03-25 14:13:24.307094194 +0100
@@ -0,0 +1,27 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+extern void abort (void);
+
+int
+main ()
+{
+  struct A { char buf1[9]; char buf2[1]; } a;
+
+  if (__builtin_object_size (a.buf1 + (0 + 4), 1) != 5)
+    abort ();
+  char *p = a.buf1;
+  p += 1;
+  p += 3;
+  if (__builtin_object_size (p, 1) != 5)
+    abort ();
+  p = (char *) &a;
+  char *q = p + 1;
+  char *r = q + 3;
+  char *t = r;
+  if (r != (char *) &a + 4)
+    t = (char *) &a + 1;
+  if (__builtin_object_size (t, 1) != 6)
+    abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/pr64715-1.c.jj	2015-03-25 13:42:15.369350086 +0100
+++ gcc/testsuite/gcc.dg/pr64715-1.c	2015-03-25 14:15:00.477536803 +0100
@@ -0,0 +1,23 @@ 
+/* PR tree-optimization/64715 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+extern inline __attribute__ ((always_inline, gnu_inline, artificial, nothrow, leaf)) char *
+strcpy (char *__restrict dest, const char *__restrict src)
+{
+  return __builtin___strcpy_chk (dest, src, __builtin_object_size (dest, 2 > 1));
+}
+
+const char *str1 = "JIHGFEDCBA";
+void bar (char *);
+
+void
+foo ()
+{
+  struct A { char buf1[9]; char buf2[1]; } a;
+  strcpy (a.buf1 + (0 + 4), str1 + 5);
+  bar ((char *) &a);
+}
+
+/* { dg-final { scan-tree-dump "__builtin___strcpy_chk\[^;\n\r\]*, 5\\\);" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
--- gcc/testsuite/gcc.dg/pr64715-2.c.jj	2015-03-25 14:46:18.453113325 +0100
+++ gcc/testsuite/gcc.dg/pr64715-2.c	2015-03-25 14:47:26.093017440 +0100
@@ -0,0 +1,19 @@ 
+/* PR tree-optimization/64715 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-objsz2" } */
+
+void bar (char *, int);
+
+void
+foo (int x)
+{
+  char p[16], *q;
+  q = p;
+  if (x)
+    q = p + 3;
+  __builtin___strcpy_chk (q, "abcdefghijkl", __builtin_object_size (q, 1));
+  bar (p, x);
+}
+
+/* { dg-final { scan-tree-dump "__builtin_memcpy \\\(\[^;\n\r\]*, \"abcdefghijkl\", 13\\\);" "objsz2" } } */
+/* { dg-final { cleanup-tree-dump "objsz2" } } */