On Mon, 20 Jan 2014, Jakub Jelinek wrote:
> On Mon, Jan 20, 2014 at 02:37:21PM +0100, Richard Biener wrote:
> > Well, strcat itself can do that ... but yes, as I said, if you can
> > CSE that strlen call then it's a win. But you can't know this without
>
> It can't, strcat doesn't know the length of the src string, we don't have
> any 3 argument strcat alternative API where the compiler tells it the length
> of the string that the compiler knows, but the function doesn't.
>
> > > Normally, if folding of a builtin folds it into a call to some other
> > > builtin, that other builtin is folded right away, so the common case is
> > > optimized immediately, the only problem is if gimple_fold_builtin tries
> > > harder to optimize using maximum lengths or exact length (as in this
> > > case).
> > > And, for this it wouldn't even help if we handled STRCAT/STRCAT_CHK
> > > specially too and passed the src length to the folder routine, because
> > > gimple_fold_builtin first folds normally and only if that fails, attempts
> > > harder.
> >
> > But we still can re-introduce the folding I removed from builtins.c
> > below the avoid_folding_inline_builtin section as in that case
> > builtins.c didn't do the folding and the gimple folding has
> > strictly more information. No? I don't really like a special
>
> There are two cases. One is where the length of the second string
> is really variable or not known to the compiler. Then I agree library
> strcat ought to do the exact same job, it is strlen + movstr instruction.
> The other case is your testcase, where it is just because the compiler did a
> poor job. I guess we can use something like following untested patch
> for that. Or supposedly we could only change fold_builtin_strcat and
> leave strcat_chk unmodified, after all strcat_chk folding is only handling
> the case where the size is -1UL (i.e. unknown) and then folds to strcat.
That works for me.
Richard.
> 2014-01-20 Jakub Jelinek <ja...@redhat.com>
>
> * tree.h (fold_builtin_strcat, fold_builtin_strcat_chk): New
> prototypes.
> * builtins.c (fold_builtin_strcat): No longer static. Add len
> argument, if non-NULL, don't call c_strlen. Optimize
> directly into __builtin_memcpy instead of __builtin_strcpy.
> (fold_builtin_strcat_chk): No longer static. Add len argument,
> if non-NULL, call fold_builtin_strcat.
> (fold_builtin_2): Adjust fold_builtin_strcat{,_chk} callers.
> * gimple-fold.c (gimple_fold_builtin): Handle BUILT_IN_STRCAT
> and BUILT_IN_STRCAT_CHK.
>
> --- gcc/tree.h.jj 2014-01-07 17:49:40.000000000 +0100
> +++ gcc/tree.h 2014-01-20 15:04:48.273418236 +0100
> @@ -5854,12 +5854,14 @@ extern tree fold_call_expr (location_t,
> extern tree fold_builtin_fputs (location_t, tree, tree, bool, bool, tree);
> extern tree fold_builtin_strcpy (location_t, tree, tree, tree, tree);
> extern tree fold_builtin_strncpy (location_t, tree, tree, tree, tree, tree);
> +extern tree fold_builtin_strcat (location_t, tree, tree, tree);
> extern tree fold_builtin_memory_chk (location_t, tree, tree, tree, tree,
> tree, tree, bool,
> enum built_in_function);
> extern tree fold_builtin_stxcpy_chk (location_t, tree, tree, tree, tree,
> tree, bool,
> enum built_in_function);
> extern tree fold_builtin_stxncpy_chk (location_t, tree, tree, tree, tree,
> tree, bool,
> enum built_in_function);
> +extern tree fold_builtin_strcat_chk (location_t, tree, tree, tree, tree,
> tree);
> extern tree fold_builtin_snprintf_chk (location_t, tree, tree, enum
> built_in_function);
> extern bool fold_builtin_next_arg (tree, bool);
> extern enum built_in_function builtin_mathfn_code (const_tree);
> --- gcc/builtins.c.jj 2014-01-20 12:41:48.000000000 +0100
> +++ gcc/builtins.c 2014-01-20 15:19:23.585947825 +0100
> @@ -180,7 +180,6 @@ static tree fold_builtin_varargs (locati
> static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
> static tree fold_builtin_strstr (location_t, tree, tree, tree);
> static tree fold_builtin_strrchr (location_t, tree, tree, tree);
> -static tree fold_builtin_strcat (location_t, tree, tree);
> static tree fold_builtin_strncat (location_t, tree, tree, tree);
> static tree fold_builtin_strspn (location_t, tree, tree);
> static tree fold_builtin_strcspn (location_t, tree, tree);
> @@ -194,7 +193,6 @@ static void maybe_emit_chk_warning (tree
> static void maybe_emit_sprintf_chk_warning (tree, enum built_in_function);
> static void maybe_emit_free_warning (tree);
> static tree fold_builtin_object_size (tree, tree);
> -static tree fold_builtin_strcat_chk (location_t, tree, tree, tree, tree);
> static tree fold_builtin_strncat_chk (location_t, tree, tree, tree, tree,
> tree);
> static tree fold_builtin_sprintf_chk (location_t, tree, enum
> built_in_function);
> static tree fold_builtin_printf (location_t, tree, tree, tree, bool, enum
> built_in_function);
> @@ -10770,7 +10768,7 @@ fold_builtin_2 (location_t loc, tree fnd
> return fold_builtin_strstr (loc, arg0, arg1, type);
>
> case BUILT_IN_STRCAT:
> - return fold_builtin_strcat (loc, arg0, arg1);
> + return fold_builtin_strcat (loc, arg0, arg1, NULL_TREE);
>
> case BUILT_IN_STRSPN:
> return fold_builtin_strspn (loc, arg0, arg1);
> @@ -10963,7 +10961,8 @@ fold_builtin_3 (location_t loc, tree fnd
> ignore, fcode);
>
> case BUILT_IN_STRCAT_CHK:
> - return fold_builtin_strcat_chk (loc, fndecl, arg0, arg1, arg2);
> + return fold_builtin_strcat_chk (loc, fndecl, arg0, arg1,
> + NULL_TREE, arg2);
>
> case BUILT_IN_PRINTF_CHK:
> case BUILT_IN_VPRINTF_CHK:
> @@ -11813,8 +11812,9 @@ fold_builtin_strpbrk (location_t loc, tr
> COMPOUND_EXPR in the chain will contain the tree for the simplified
> form of the builtin function call. */
>
> -static tree
> -fold_builtin_strcat (location_t loc ATTRIBUTE_UNUSED, tree dst, tree src)
> +tree
> +fold_builtin_strcat (location_t loc ATTRIBUTE_UNUSED, tree dst, tree src,
> + tree len)
> {
> if (!validate_arg (dst, POINTER_TYPE)
> || !validate_arg (src, POINTER_TYPE))
> @@ -11832,14 +11832,15 @@ fold_builtin_strcat (location_t loc ATTR
> /* See if we can store by pieces into (dst + strlen(dst)). */
> tree newdst, call;
> tree strlen_fn = builtin_decl_implicit (BUILT_IN_STRLEN);
> - tree strcpy_fn = builtin_decl_implicit (BUILT_IN_STRCPY);
> + tree memcpy_fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
>
> - if (!strlen_fn || !strcpy_fn)
> + if (!strlen_fn || !memcpy_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);
> + split strcat into strlen and memcpy. */
> + if (! len)
> + len = c_strlen (src, 1);
> if (! len || TREE_SIDE_EFFECTS (len))
> return NULL_TREE;
>
> @@ -11853,7 +11854,11 @@ fold_builtin_strcat (location_t loc ATTR
> newdst = fold_build_pointer_plus_loc (loc, dst, newdst);
> newdst = builtin_save_expr (newdst);
>
> - call = build_call_expr_loc (loc, strcpy_fn, 2, newdst, src);
> + len = fold_convert_loc (loc, size_type_node, len);
> + len = size_binop_loc (loc, PLUS_EXPR, len,
> + build_int_cst (size_type_node, 1));
> +
> + call = build_call_expr_loc (loc, memcpy_fn, 3, newdst, src, len);
> return build2 (COMPOUND_EXPR, TREE_TYPE (dst), call, dst);
> }
> return NULL_TREE;
> @@ -13005,9 +13010,9 @@ fold_builtin_stxncpy_chk (location_t loc
> /* Fold a call to the __strcat_chk builtin FNDECL. DEST, SRC, and SIZE
> are the arguments to the call. */
>
> -static tree
> +tree
> fold_builtin_strcat_chk (location_t loc, tree fndecl, tree dest,
> - tree src, tree size)
> + tree src, tree len, tree size)
> {
> tree fn;
> const char *p;
> @@ -13030,6 +13035,12 @@ fold_builtin_strcat_chk (location_t loc,
> if (!fn)
> return NULL_TREE;
>
> + if (len)
> + {
> + tree ret = fold_builtin_strcat (loc, dest, src, len);
> + if (ret)
> + return ret;
> + }
> return build_call_expr_loc (loc, fn, 2, dest, src);
> }
>
> --- gcc/gimple-fold.c.jj 2013-11-05 13:06:02.000000000 +0100
> +++ gcc/gimple-fold.c 2014-01-20 15:12:42.297991142 +0100
> @@ -866,6 +866,8 @@ gimple_fold_builtin (gimple stmt)
> break;
> case BUILT_IN_STRCPY:
> case BUILT_IN_STRNCPY:
> + case BUILT_IN_STRCAT:
> + case BUILT_IN_STRCAT_CHK:
> arg_idx = 1;
> type = 0;
> break;
> @@ -941,6 +943,22 @@ gimple_fold_builtin (gimple stmt)
> val[1]);
> break;
>
> + case BUILT_IN_STRCAT:
> + if (val[1] && is_gimple_val (val[1]) && nargs == 2)
> + result = fold_builtin_strcat (loc, gimple_call_arg (stmt, 0),
> + gimple_call_arg (stmt, 1),
> + val[1]);
> + break;
> +
> + case BUILT_IN_STRCAT_CHK:
> + if (val[1] && is_gimple_val (val[1]) && nargs == 3)
> + result = fold_builtin_strcat_chk (loc, callee,
> + gimple_call_arg (stmt, 0),
> + gimple_call_arg (stmt, 1),
> + val[1],
> + gimple_call_arg (stmt, 2));
> + break;
> +
> case BUILT_IN_FPUTS:
> if (nargs == 2)
> result = fold_builtin_fputs (loc, gimple_call_arg (stmt, 0),
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
