On Thu, 28 Nov 2019, Jakub Jelinek wrote:
> Hi!
>
> The various routines propagate to the caller whether
> if (check_and_optimize_stmt (&gsi, &cleanup_eh, evrp.get_vr_values ()))
> gsi_next (&gsi);
> should do gsi_next or not (return false if e.g. gsi_remove is done, thus
> gsi is already moved to the next stmt).
> handle_printf_call returns that too, though with the values swapped,
> but since the move of handle_printf_call (then called handle_gimple_call)
> from the separate sprintf pass to strlen pass, the return value is ignored,
> while it must not be. In some cases it means the following statement is not
> processed by the strlen pass, which can e.g. mean wrong-code because some
> strlen information is not invalidated when it should, or in other cases like
> in this testcase where the sprintf call that was removed was at the end of a
> bb
> it means an ICE, because gsi_next when gsi is already at the end of bb is
> invalid.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
OK.
Richard.
> 2019-11-27 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/92691
> * tree-ssa-strlen.c (handle_store): Clarify return value meaning
> in function comment.
> (strlen_check_and_optimize_call): Likewise. For handle_printf_call
> calls, return !handle_printf_call rather than always returning true.
> (check_and_optimize_stmt): Describe return value meaning in function
> comment. Formatting fix.
>
> * gcc.dg/tree-ssa/builtin-snprintf-10.c: New test.
>
> --- gcc/tree-ssa-strlen.c.jj 2019-11-22 19:11:54.901951408 +0100
> +++ gcc/tree-ssa-strlen.c 2019-11-27 12:25:14.778564388 +0100
> @@ -4300,7 +4300,8 @@ count_nonzero_bytes (tree exp, unsigned
> /* Handle a single or multibyte store other than by a built-in function,
> either via a single character assignment or by multi-byte assignment
> either via MEM_REF or via a type other than char (such as in
> - '*(int*)a = 12345'). Return true when handled. */
> + '*(int*)a = 12345'). Return true to let the caller advance *GSI to
> + the next statement in the basic block and false otherwise. */
>
> static bool
> handle_store (gimple_stmt_iterator *gsi, bool *zero_write, const vr_values
> *rvals)
> @@ -4728,8 +4729,8 @@ is_char_type (tree type)
> }
>
> /* Check the built-in call at GSI for validity and optimize it.
> - Return true to let the caller advance *GSI to the statement
> - in the CFG and false otherwise. */
> + Return true to let the caller advance *GSI to the next statement
> + in the basic block and false otherwise. */
>
> static bool
> strlen_check_and_optimize_call (gimple_stmt_iterator *gsi,
> @@ -4738,16 +4739,13 @@ strlen_check_and_optimize_call (gimple_s
> {
> gimple *stmt = gsi_stmt (*gsi);
>
> + /* When not optimizing we must be checking printf calls which
> + we do even for user-defined functions when they are declared
> + with attribute format. */
> if (!flag_optimize_strlen
> || !strlen_optimize
> || !valid_builtin_call (stmt))
> - {
> - /* When not optimizing we must be checking printf calls which
> - we do even for user-defined functions when they are declared
> - with attribute format. */
> - handle_printf_call (gsi, rvals);
> - return true;
> - }
> + return !handle_printf_call (gsi, rvals);
>
> tree callee = gimple_call_fndecl (stmt);
> switch (DECL_FUNCTION_CODE (callee))
> @@ -4806,7 +4804,8 @@ strlen_check_and_optimize_call (gimple_s
> return false;
> break;
> default:
> - handle_printf_call (gsi, rvals);
> + if (handle_printf_call (gsi, rvals))
> + return false;
> break;
> }
>
> @@ -4932,7 +4931,8 @@ handle_integral_assign (gimple_stmt_iter
> /* Attempt to check for validity of the performed access a single statement
> at *GSI using string length knowledge, and to optimize it.
> If the given basic block needs clean-up of EH, CLEANUP_EH is set to
> - true. */
> + true. Return true to let the caller advance *GSI to the next statement
> + in the basic block and false otherwise. */
>
> static bool
> check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh,
> @@ -4973,32 +4973,32 @@ check_and_optimize_stmt (gimple_stmt_ite
> /* Handle assignment to a character. */
> handle_integral_assign (gsi, cleanup_eh);
> else if (TREE_CODE (lhs) != SSA_NAME && !TREE_SIDE_EFFECTS (lhs))
> - {
> - tree type = TREE_TYPE (lhs);
> - if (TREE_CODE (type) == ARRAY_TYPE)
> - type = TREE_TYPE (type);
> -
> - bool is_char_store = is_char_type (type);
> - if (!is_char_store && TREE_CODE (lhs) == MEM_REF)
> - {
> - /* To consider stores into char objects via integer types
> - other than char but not those to non-character objects,
> - determine the type of the destination rather than just
> - the type of the access. */
> - tree ref = TREE_OPERAND (lhs, 0);
> - type = TREE_TYPE (ref);
> - if (TREE_CODE (type) == POINTER_TYPE)
> - type = TREE_TYPE (type);
> - if (TREE_CODE (type) == ARRAY_TYPE)
> - type = TREE_TYPE (type);
> - if (is_char_type (type))
> - is_char_store = true;
> - }
> -
> - /* Handle a single or multibyte assignment. */
> - if (is_char_store && !handle_store (gsi, &zero_write, rvals))
> - return false;
> - }
> + {
> + tree type = TREE_TYPE (lhs);
> + if (TREE_CODE (type) == ARRAY_TYPE)
> + type = TREE_TYPE (type);
> +
> + bool is_char_store = is_char_type (type);
> + if (!is_char_store && TREE_CODE (lhs) == MEM_REF)
> + {
> + /* To consider stores into char objects via integer types
> + other than char but not those to non-character objects,
> + determine the type of the destination rather than just
> + the type of the access. */
> + tree ref = TREE_OPERAND (lhs, 0);
> + type = TREE_TYPE (ref);
> + if (TREE_CODE (type) == POINTER_TYPE)
> + type = TREE_TYPE (type);
> + if (TREE_CODE (type) == ARRAY_TYPE)
> + type = TREE_TYPE (type);
> + if (is_char_type (type))
> + is_char_store = true;
> + }
> +
> + /* Handle a single or multibyte assignment. */
> + if (is_char_store && !handle_store (gsi, &zero_write, rvals))
> + return false;
> + }
> }
> else if (gcond *cond = dyn_cast<gcond *> (stmt))
> {
> --- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-10.c.jj 2019-11-27
> 12:23:19.180349031 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-10.c 2019-11-27
> 12:23:48.070903012 +0100
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/92691 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +void
> +foo (int x, char *y)
> +{
> + if (x != 0)
> + __builtin_snprintf (y, 0, "foo");
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
