Fix ICE in tree-ssa-strlen.c (PR tree-optimization/92691)

Message ID 20191127233454.GC10088@tucnak
State New
Headers show
Series
  • Fix ICE in tree-ssa-strlen.c (PR tree-optimization/92691)
Related show

Commit Message

Jakub Jelinek Nov. 27, 2019, 11:34 p.m.
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?

2019-11-27  Jakub Jelinek  <jakub@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.


	Jakub

Comments

Richard Biener Nov. 28, 2019, 8:30 a.m. | #1
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  <jakub@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Patch

--- 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");
+}