Re: [PATCH][RFC] Fix PR59860

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Jan 20, 2014 at 11:54:15AM +0100, Richard Biener wrote:
> 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.

But then we simply never optimize this anymore.
Or regress when you e.g. in your testcase replace
__builtin___strcat_chk with __builtin_strcat and remove last argument
(while if there is no inline, it is still optimized).

tree-ssa-strlen.c apparently both doesn't this case (unknown first strlen,
known second strlen or movstr pattern, will only transform that if the
length of the resulting string is needed afterwards), and isn't run
for -Os or -O1 anyway.

For other builtins, this case is usually handled in gimple-fold.c
(gimple_fold_builtin), but BUILT_IN_STRCAT isn't handled there.
So perhaps it should and add one argument to fold_builtin_strcat
if we know the length, but don't know where the argument points to.

And/or handle src being SSA_NAME with GIMPLE_PHI def stmt, by
calling c_strlen on each of the phi arguments individually (i.e. handle
it similarly to the COND_EXPR case) and verifying it is the same.

But I guess if we optimize it again, your testcase would crash again, right?

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

	Jakub