[PATCH] c: Fix ICE caused by get_parm_array_spec [PR101702]

[PATCH] c: Fix ICE caused by get_parm_array_spec [PR101702]

[Jakub Jelinek]

Hi!

The following testcase ICEs, because nelts is NOP_EXPR around INTEGER_CST
- it is a VLA whose extent folds into a constant - and get_parm_array_spec
has specific INTEGER_CST handling and otherwise strips nops from nelts
and stores it into a TREE_LIST that is later asserted to be a DECL_P
or EXPR_P, where the INTEGER_CST is neither of that.

So, either we can strip nops earlier (needs moving the integral type
check first as STRIP_NOPS can alter that e.g. to pointer or from
pointer to integer) and thus handle as INTEGER_CST even the case
of INTEGER_CST wrapped into casts as this patch does, or we need
to change handle_argspec_attribute's assertion to allow INTEGER_CSTs
as well there.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or do you prefer to change handle_argspec_attribute ?

2021-08-03  Jakub Jelinek  <jakub@redhat.com>

	PR c/101702
	* c-decl.c (get_parm_array_spec): Check for non-integral
	type first, then STRIP_NOPS and only afterwards check for
	INTEGER_CST.

	* gcc.dg/pr101702.c: New test.

--- gcc/c/c-decl.c.jj	2021-07-15 18:50:52.000000000 +0200
+++ gcc/c/c-decl.c	2021-08-02 18:56:35.532045128 +0200
@@ -5842,6 +5842,11 @@ get_parm_array_spec (const struct c_parm
       if (pd->u.array.static_p)
 	spec += 's';
 
+      if (!INTEGRAL_TYPE_P (TREE_TYPE (nelts)))
+	/* Avoid invalid NELTS.  */
+	return attrs;
+
+      STRIP_NOPS (nelts);
       if (TREE_CODE (nelts) == INTEGER_CST)
 	{
 	  /* Skip all constant bounds except the most significant one.
@@ -5859,13 +5864,9 @@ get_parm_array_spec (const struct c_parm
 	  spec += buf;
 	  break;
 	}
-      else if (!INTEGRAL_TYPE_P (TREE_TYPE (nelts)))
-	/* Avoid invalid NELTS.  */
-	return attrs;
 
       /* Each variable VLA bound is represented by a dollar sign.  */
       spec += "$";
-      STRIP_NOPS (nelts);
       vbchain = tree_cons (NULL_TREE, nelts, vbchain);
     }
 
--- gcc/testsuite/gcc.dg/pr101702.c.jj	2021-08-02 18:58:24.614534975 +0200
+++ gcc/testsuite/gcc.dg/pr101702.c	2021-08-02 18:57:52.611978024 +0200
@@ -0,0 +1,11 @@
+/* PR c/101702 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+double foo (double x[!__builtin_copysignf (~2, 3)]);
+
+double
+bar (double *x)
+{
+  return foo (x);
+}

	Jakub

Re: [PATCH] c: Fix ICE caused by get_parm_array_spec [PR101702]

[Martin Sebor]

On 8/3/21 1:17 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase ICEs, because nelts is NOP_EXPR around INTEGER_CST
> - it is a VLA whose extent folds into a constant - and get_parm_array_spec
> has specific INTEGER_CST handling and otherwise strips nops from nelts
> and stores it into a TREE_LIST that is later asserted to be a DECL_P
> or EXPR_P, where the INTEGER_CST is neither of that.
> 
> So, either we can strip nops earlier (needs moving the integral type
> check first as STRIP_NOPS can alter that e.g. to pointer or from
> pointer to integer) and thus handle as INTEGER_CST even the case
> of INTEGER_CST wrapped into casts as this patch does, or we need
> to change handle_argspec_attribute's assertion to allow INTEGER_CSTs
> as well there.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> Or do you prefer to change handle_argspec_attribute ?

I think the bug is in the bound being treated as variable but then
in determining its value to be constant.  I expect the following
to be diagnosed again, the same way as in GCC 11:

   double foo (double x[!__builtin_copysignf (~2, 3)]);
   double foo (double x[]);

Your fix prevents it.  So I think the right fix would treat
the bound as constant in this case.

Martin

> 
> 2021-08-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/101702
> 	* c-decl.c (get_parm_array_spec): Check for non-integral
> 	type first, then STRIP_NOPS and only afterwards check for
> 	INTEGER_CST.
> 
> 	* gcc.dg/pr101702.c: New test.
> 
> --- gcc/c/c-decl.c.jj	2021-07-15 18:50:52.000000000 +0200
> +++ gcc/c/c-decl.c	2021-08-02 18:56:35.532045128 +0200
> @@ -5842,6 +5842,11 @@ get_parm_array_spec (const struct c_parm
>         if (pd->u.array.static_p)
>   	spec += 's';
>   
> +      if (!INTEGRAL_TYPE_P (TREE_TYPE (nelts)))
> +	/* Avoid invalid NELTS.  */
> +	return attrs;
> +
> +      STRIP_NOPS (nelts);
>         if (TREE_CODE (nelts) == INTEGER_CST)
>   	{
>   	  /* Skip all constant bounds except the most significant one.
> @@ -5859,13 +5864,9 @@ get_parm_array_spec (const struct c_parm
>   	  spec += buf;
>   	  break;
>   	}
> -      else if (!INTEGRAL_TYPE_P (TREE_TYPE (nelts)))
> -	/* Avoid invalid NELTS.  */
> -	return attrs;
>   
>         /* Each variable VLA bound is represented by a dollar sign.  */
>         spec += "$";
> -      STRIP_NOPS (nelts);
>         vbchain = tree_cons (NULL_TREE, nelts, vbchain);
>       }
>   
> --- gcc/testsuite/gcc.dg/pr101702.c.jj	2021-08-02 18:58:24.614534975 +0200
> +++ gcc/testsuite/gcc.dg/pr101702.c	2021-08-02 18:57:52.611978024 +0200
> @@ -0,0 +1,11 @@
> +/* PR c/101702 */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +
> +double foo (double x[!__builtin_copysignf (~2, 3)]);
> +
> +double
> +bar (double *x)
> +{
> +  return foo (x);
> +}
> 
> 	Jakub
>