[PATCH][RFC] Fix PR59860

Richard Biener rguenther@suse.de
Mon Jan 20 10:54:00 GMT 2014


On Sun, 19 Jan 2014, Jakub Jelinek wrote:

> On Sun, Jan 19, 2014 at 07:05:12PM +0100, Richard Biener wrote:
> > The following patch fixes PR59860 by removing the only folding
> > builtins.c does that can end up recursing to GIMPLE call stmt
> > folding.  It does that via strcat -> strlen + strcpy folding
> > and then folding the strlen gimple stmt via gimplify which
> > then can use the SSA web to fold that to a constant and then
> > the strcpy call to memcpy.  This confuses the virtual operand
> > updating code - not that it ends up creating wrong virtual SSA
> > form but by bogously marking virtual operands for renaming
> > through the operand scanner as the folding on the just gimplified
> > sequence doesn't see any VOPs yet.
> > 
> > Bootstrapped on x86_64-unknown-linux-gnu, testing reveals that I
> > have to adjust gcc.c-torture/execute/builtins/strcat.c expectations
> > and gcc.dg/strlenopt-* likely because of different input.
> > 
> > I still think this patch is better than the second option, avoiding
> > to call fold_stmt from the gimplifier in this case (either by
> > a new ctx flag or looking at ctx->into_ssa).  I also think that
> > "fixing" this by scheduling update-ssa after objsz is wrong.
> > 
> > Any opinions?  Maybe any different preferences for branch / trunk?
> 
> If you verify that strlen pass does the right thing here (I hope it does,
> but haven't verified), then I think this is the way for the trunk, not
> sure about the branch, I'd prefer there something less intrusive at this
> point, even if it is say just through setting some global flag that will
> disable the strcat folding at the problematic spot, or folds it on the tree
> before gimplification, or gimplifies operands and builds the call in gimple
> by hand for the strcat case.
> 
> > 2014-01-17  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/59860
> > 	* builtins.c (fold_builtin_strcat): Remove case better handled
> > 	by tree-ssa-strlen.c
> 
> Missing dot at the end ;)
> > 
> > 	* gcc.dg/pr59860.c: New testcase.
> > 

Ok, the following simpler patch also fixes the issue and causes
no testsuite fallout - we restrict the folding in builtins.c to
the case with known src strlen.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk
and branch.

Richard.

2014-01-17  Richard Biener  <rguenther@suse.de>

	PR middle-end/59860
	* builtins.c (fold_builtin_strcat): Remove case better handled
	by tree-ssa-strlen.c.

	* gcc.dg/pr59860.c: New testcase.

Index: gcc/testsuite/gcc.dg/pr59860.c
===================================================================
*** gcc/testsuite/gcc.dg/pr59860.c	(revision 0)
--- gcc/testsuite/gcc.dg/pr59860.c	(working copy)
***************
*** 0 ****
--- 1,15 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O" } */
+ 
+ extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) char * __attribute__ ((__nothrow__ , __leaf__))
+ strcat (char *__restrict __dest, const char *__restrict __src)
+ {
+   return __builtin___strcat_chk (__dest, __src, __builtin_object_size (__dest, 2 > 1));
+ }
+ static char raw_decode;
+ void foo (char **argv, char *outfilename)
+ {
+   if (**argv == 'r')
+     raw_decode = 1;
+   strcat (outfilename, raw_decode ? ".raw" : ".wav");
+ }
Index: gcc/builtins.c
===================================================================
*** gcc/builtins.c	(revision 206772)
--- gcc/builtins.c	(working copy)
*************** fold_builtin_strcat (location_t loc ATTR
*** 11759,11775 ****
  	  if (!strlen_fn || !strcpy_fn)
  	    return NULL_TREE;
  
! 	  /* If we don't have a movstr we don't want to emit an strcpy
! 	     call.  We have to do that if the length of the source string
! 	     isn't computable (in that case we can use memcpy probably
! 	     later expanding to a sequence of mov instructions).  If we
! 	     have movstr instructions we can emit strcpy calls.  */
! 	  if (!HAVE_movstr)
! 	    {
! 	      tree len = c_strlen (src, 1);
! 	      if (! len || TREE_SIDE_EFFECTS (len))
! 		return NULL_TREE;
! 	    }
  
  	  /* Stabilize the argument list.  */
  	  dst = builtin_save_expr (dst);
--- 11759,11769 ----
  	  if (!strlen_fn || !strcpy_fn)
  	    return NULL_TREE;
  
! 	  /* If the length of the source string isn't computable don't
! 	     split strcat into strlen and strcpy.  */
! 	  tree len = c_strlen (src, 1);
! 	  if (! len || TREE_SIDE_EFFECTS (len))
! 	    return NULL_TREE;
  
  	  /* Stabilize the argument list.  */
  	  dst = builtin_save_expr (dst);



More information about the Gcc-patches mailing list