diff mbox series

builtins: Fix up strspn/strcspn folding [PR119219]

Message ID Z9Ew7LWBNmxo8Rg2@tucnak
State New
Headers
Series builtins: Fix up strspn/strcspn folding [PR119219] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap fail Patch failed to apply
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap fail Patch failed to apply

Commit Message

Jakub Jelinek March 12, 2025, 6:59 a.m. UTC 
  Hi!

The PR119204 r15-7955 fix caused some regressions.
The problem is that the fold_builtin* APIs document that expr is
either a CALL_EXPR of the call or NULL, so using TREE_TYPE (expr)
can crash e.g. during constexpr evaluation etc.

As can be seen in the surrounding patch, for the neighbouring builtins
(both modf and strpbrk) fold_builtin_2 passes down type, which is the
result type, TREE_TYPE (TREE_TYPE (fndecl)) and those builtins use it
to build the return value, while strspn was always building size_type_node
and strcspn had this change from that to TREE_TYPE (expr).
The patch passes type to these two and uses it there as well.

The patch keeps passing expr because it is used in the
check_nul_terminated_array calls done for both strspn and strcspn,
those calls clearly can deal with NULL expr but prefer if it is non-NULL
for some warning.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2025-03-12  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/119204
	PR middle-end/119219
	* builtins.cc (fold_builtin_2): Pass type as another argument
	to fold_builtin_strspn and fold_builtin_strcspn.
	(fold_builtin_strspn): Add type argument, use it instead of
	size_type_node.
	(fold_builtin_strcspn): Add type argument, use it instead of
	TREE_TYPE (expr).


	Jakub

Comments

Andrew Pinski March 12, 2025, 7:04 a.m. UTC | #1 
On Wed, Mar 12, 2025 at 12:01 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The PR119204 r15-7955 fix caused some regressions.
> The problem is that the fold_builtin* APIs document that expr is
> either a CALL_EXPR of the call or NULL, so using TREE_TYPE (expr)
> can crash e.g. during constexpr evaluation etc.
>
> As can be seen in the surrounding patch, for the neighbouring builtins
> (both modf and strpbrk) fold_builtin_2 passes down type, which is the
> result type, TREE_TYPE (TREE_TYPE (fndecl)) and those builtins use it
> to build the return value, while strspn was always building size_type_node
> and strcspn had this change from that to TREE_TYPE (expr).
> The patch passes type to these two and uses it there as well.
>
> The patch keeps passing expr because it is used in the
> check_nul_terminated_array calls done for both strspn and strcspn,
> those calls clearly can deal with NULL expr but prefer if it is non-NULL
> for some warning.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I think it would be useful to add the C testcase that fails during DOM
from PR 119226 since that is another path to get the ICE.

Thanks,
Andrew Pinski

>
> 2025-03-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR middle-end/119204
>         PR middle-end/119219
>         * builtins.cc (fold_builtin_2): Pass type as another argument
>         to fold_builtin_strspn and fold_builtin_strcspn.
>         (fold_builtin_strspn): Add type argument, use it instead of
>         size_type_node.
>         (fold_builtin_strcspn): Add type argument, use it instead of
>         TREE_TYPE (expr).
>
> --- gcc/builtins.cc.jj  2025-03-11 12:05:42.561533453 +0100
> +++ gcc/builtins.cc     2025-03-11 15:36:47.774314011 +0100
> @@ -176,8 +176,8 @@ static tree fold_builtin_iseqsig (locati
>  static tree fold_builtin_varargs (location_t, tree, tree*, int);
>
>  static tree fold_builtin_strpbrk (location_t, tree, tree, tree, tree);
> -static tree fold_builtin_strspn (location_t, tree, tree, tree);
> -static tree fold_builtin_strcspn (location_t, tree, tree, tree);
> +static tree fold_builtin_strspn (location_t, tree, tree, tree, tree);
> +static tree fold_builtin_strcspn (location_t, tree, tree, tree, tree);
>
>  static rtx expand_builtin_object_size (tree);
>  static rtx expand_builtin_memory_chk (tree, rtx, machine_mode,
> @@ -10800,10 +10800,10 @@ fold_builtin_2 (location_t loc, tree exp
>        return fold_builtin_modf (loc, arg0, arg1, type);
>
>      case BUILT_IN_STRSPN:
> -      return fold_builtin_strspn (loc, expr, arg0, arg1);
> +      return fold_builtin_strspn (loc, expr, arg0, arg1, type);
>
>      case BUILT_IN_STRCSPN:
> -      return fold_builtin_strcspn (loc, expr, arg0, arg1);
> +      return fold_builtin_strcspn (loc, expr, arg0, arg1, type);
>
>      case BUILT_IN_STRPBRK:
>        return fold_builtin_strpbrk (loc, expr, arg0, arg1, type);
> @@ -11304,7 +11304,7 @@ fold_builtin_strpbrk (location_t loc, tr
>     form of the builtin function call.  */
>
>  static tree
> -fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2)
> +fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2, tree type)
>  {
>    if (!validate_arg (s1, POINTER_TYPE)
>        || !validate_arg (s2, POINTER_TYPE))
> @@ -11320,8 +11320,7 @@ fold_builtin_strspn (location_t loc, tre
>    if ((p1 && *p1 == '\0') || (p2 && *p2 == '\0'))
>      /* Evaluate and ignore both arguments in case either one has
>         side-effects.  */
> -    return omit_two_operands_loc (loc, size_type_node, size_zero_node,
> -                                 s1, s2);
> +    return omit_two_operands_loc (loc, type, size_zero_node, s1, s2);
>    return NULL_TREE;
>  }
>
> @@ -11344,7 +11343,7 @@ fold_builtin_strspn (location_t loc, tre
>     form of the builtin function call.  */
>
>  static tree
> -fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2)
> +fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2, tree type)
>  {
>    if (!validate_arg (s1, POINTER_TYPE)
>        || !validate_arg (s2, POINTER_TYPE))
> @@ -11360,8 +11359,7 @@ fold_builtin_strcspn (location_t loc, tr
>      {
>        /* Evaluate and ignore argument s2 in case it has
>          side-effects.  */
> -      return omit_one_operand_loc (loc, TREE_TYPE (expr),
> -                                  size_zero_node, s2);
> +      return omit_one_operand_loc (loc, type, size_zero_node, s2);
>      }
>
>    /* If the second argument is "", return __builtin_strlen(s1).  */
> @@ -11375,7 +11373,7 @@ fold_builtin_strcspn (location_t loc, tr
>        if (!fn)
>         return NULL_TREE;
>
> -      return fold_convert_loc (loc, TREE_TYPE (expr),
> +      return fold_convert_loc (loc, type,
>                                build_call_expr_loc (loc, fn, 1, s1));
>      }
>    return NULL_TREE;
>
>         Jakub
>
Richard Biener March 12, 2025, 7:22 a.m. UTC | #2 
On Wed, 12 Mar 2025, Jakub Jelinek wrote:

> Hi!
> 
> The PR119204 r15-7955 fix caused some regressions.
> The problem is that the fold_builtin* APIs document that expr is
> either a CALL_EXPR of the call or NULL, so using TREE_TYPE (expr)
> can crash e.g. during constexpr evaluation etc.
> 
> As can be seen in the surrounding patch, for the neighbouring builtins
> (both modf and strpbrk) fold_builtin_2 passes down type, which is the
> result type, TREE_TYPE (TREE_TYPE (fndecl)) and those builtins use it
> to build the return value, while strspn was always building size_type_node
> and strcspn had this change from that to TREE_TYPE (expr).
> The patch passes type to these two and uses it there as well.
> 
> The patch keeps passing expr because it is used in the
> check_nul_terminated_array calls done for both strspn and strcspn,
> those calls clearly can deal with NULL expr but prefer if it is non-NULL
> for some warning.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

> 2025-03-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/119204
> 	PR middle-end/119219
> 	* builtins.cc (fold_builtin_2): Pass type as another argument
> 	to fold_builtin_strspn and fold_builtin_strcspn.
> 	(fold_builtin_strspn): Add type argument, use it instead of
> 	size_type_node.
> 	(fold_builtin_strcspn): Add type argument, use it instead of
> 	TREE_TYPE (expr).
> 
> --- gcc/builtins.cc.jj	2025-03-11 12:05:42.561533453 +0100
> +++ gcc/builtins.cc	2025-03-11 15:36:47.774314011 +0100
> @@ -176,8 +176,8 @@ static tree fold_builtin_iseqsig (locati
>  static tree fold_builtin_varargs (location_t, tree, tree*, int);
>  
>  static tree fold_builtin_strpbrk (location_t, tree, tree, tree, tree);
> -static tree fold_builtin_strspn (location_t, tree, tree, tree);
> -static tree fold_builtin_strcspn (location_t, tree, tree, tree);
> +static tree fold_builtin_strspn (location_t, tree, tree, tree, tree);
> +static tree fold_builtin_strcspn (location_t, tree, tree, tree, tree);
>  
>  static rtx expand_builtin_object_size (tree);
>  static rtx expand_builtin_memory_chk (tree, rtx, machine_mode,
> @@ -10800,10 +10800,10 @@ fold_builtin_2 (location_t loc, tree exp
>        return fold_builtin_modf (loc, arg0, arg1, type);
>  
>      case BUILT_IN_STRSPN:
> -      return fold_builtin_strspn (loc, expr, arg0, arg1);
> +      return fold_builtin_strspn (loc, expr, arg0, arg1, type);
>  
>      case BUILT_IN_STRCSPN:
> -      return fold_builtin_strcspn (loc, expr, arg0, arg1);
> +      return fold_builtin_strcspn (loc, expr, arg0, arg1, type);
>  
>      case BUILT_IN_STRPBRK:
>        return fold_builtin_strpbrk (loc, expr, arg0, arg1, type);
> @@ -11304,7 +11304,7 @@ fold_builtin_strpbrk (location_t loc, tr
>     form of the builtin function call.  */
>  
>  static tree
> -fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2)
> +fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2, tree type)
>  {
>    if (!validate_arg (s1, POINTER_TYPE)
>        || !validate_arg (s2, POINTER_TYPE))
> @@ -11320,8 +11320,7 @@ fold_builtin_strspn (location_t loc, tre
>    if ((p1 && *p1 == '\0') || (p2 && *p2 == '\0'))
>      /* Evaluate and ignore both arguments in case either one has
>         side-effects.  */
> -    return omit_two_operands_loc (loc, size_type_node, size_zero_node,
> -				  s1, s2);
> +    return omit_two_operands_loc (loc, type, size_zero_node, s1, s2);
>    return NULL_TREE;
>  }
>  
> @@ -11344,7 +11343,7 @@ fold_builtin_strspn (location_t loc, tre
>     form of the builtin function call.  */
>  
>  static tree
> -fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2)
> +fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2, tree type)
>  {
>    if (!validate_arg (s1, POINTER_TYPE)
>        || !validate_arg (s2, POINTER_TYPE))
> @@ -11360,8 +11359,7 @@ fold_builtin_strcspn (location_t loc, tr
>      {
>        /* Evaluate and ignore argument s2 in case it has
>  	 side-effects.  */
> -      return omit_one_operand_loc (loc, TREE_TYPE (expr),
> -				   size_zero_node, s2);
> +      return omit_one_operand_loc (loc, type, size_zero_node, s2);
>      }
>  
>    /* If the second argument is "", return __builtin_strlen(s1).  */
> @@ -11375,7 +11373,7 @@ fold_builtin_strcspn (location_t loc, tr
>        if (!fn)
>  	return NULL_TREE;
>  
> -      return fold_convert_loc (loc, TREE_TYPE (expr),
> +      return fold_convert_loc (loc, type,
>  			       build_call_expr_loc (loc, fn, 1, s1));
>      }
>    return NULL_TREE;
> 
> 	Jakub
> 
>
Jakub Jelinek March 12, 2025, 7:31 a.m. UTC | #3 
On Wed, Mar 12, 2025 at 12:04:52AM -0700, Andrew Pinski wrote:
> I think it would be useful to add the C testcase that fails during DOM
> from PR 119226 since that is another path to get the ICE.

I've committed the patch already.
Feel free to commit the testcase for it (preferably a little bit cleaned
up, replacing the fancy identifiers in the test with something short and
__SIZE_TYPE__ as return type from strcspn prototype).

	Jakub
diff mbox series

Patch

--- gcc/builtins.cc.jj	2025-03-11 12:05:42.561533453 +0100
+++ gcc/builtins.cc	2025-03-11 15:36:47.774314011 +0100
@@ -176,8 +176,8 @@  static tree fold_builtin_iseqsig (locati
 static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree, tree);
-static tree fold_builtin_strspn (location_t, tree, tree, tree);
-static tree fold_builtin_strcspn (location_t, tree, tree, tree);
+static tree fold_builtin_strspn (location_t, tree, tree, tree, tree);
+static tree fold_builtin_strcspn (location_t, tree, tree, tree, tree);
 
 static rtx expand_builtin_object_size (tree);
 static rtx expand_builtin_memory_chk (tree, rtx, machine_mode,
@@ -10800,10 +10800,10 @@  fold_builtin_2 (location_t loc, tree exp
       return fold_builtin_modf (loc, arg0, arg1, type);
 
     case BUILT_IN_STRSPN:
-      return fold_builtin_strspn (loc, expr, arg0, arg1);
+      return fold_builtin_strspn (loc, expr, arg0, arg1, type);
 
     case BUILT_IN_STRCSPN:
-      return fold_builtin_strcspn (loc, expr, arg0, arg1);
+      return fold_builtin_strcspn (loc, expr, arg0, arg1, type);
 
     case BUILT_IN_STRPBRK:
       return fold_builtin_strpbrk (loc, expr, arg0, arg1, type);
@@ -11304,7 +11304,7 @@  fold_builtin_strpbrk (location_t loc, tr
    form of the builtin function call.  */
 
 static tree
-fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2)
+fold_builtin_strspn (location_t loc, tree expr, tree s1, tree s2, tree type)
 {
   if (!validate_arg (s1, POINTER_TYPE)
       || !validate_arg (s2, POINTER_TYPE))
@@ -11320,8 +11320,7 @@  fold_builtin_strspn (location_t loc, tre
   if ((p1 && *p1 == '\0') || (p2 && *p2 == '\0'))
     /* Evaluate and ignore both arguments in case either one has
        side-effects.  */
-    return omit_two_operands_loc (loc, size_type_node, size_zero_node,
-				  s1, s2);
+    return omit_two_operands_loc (loc, type, size_zero_node, s1, s2);
   return NULL_TREE;
 }
 
@@ -11344,7 +11343,7 @@  fold_builtin_strspn (location_t loc, tre
    form of the builtin function call.  */
 
 static tree
-fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2)
+fold_builtin_strcspn (location_t loc, tree expr, tree s1, tree s2, tree type)
 {
   if (!validate_arg (s1, POINTER_TYPE)
       || !validate_arg (s2, POINTER_TYPE))
@@ -11360,8 +11359,7 @@  fold_builtin_strcspn (location_t loc, tr
     {
       /* Evaluate and ignore argument s2 in case it has
 	 side-effects.  */
-      return omit_one_operand_loc (loc, TREE_TYPE (expr),
-				   size_zero_node, s2);
+      return omit_one_operand_loc (loc, type, size_zero_node, s2);
     }
 
   /* If the second argument is "", return __builtin_strlen(s1).  */
@@ -11375,7 +11373,7 @@  fold_builtin_strcspn (location_t loc, tr
       if (!fn)
 	return NULL_TREE;
 
-      return fold_convert_loc (loc, TREE_TYPE (expr),
+      return fold_convert_loc (loc, type,
 			       build_call_expr_loc (loc, fn, 1, s1));
     }
   return NULL_TREE;