Fix PR92088
diff mbox series

Message ID nycvar.YFH.7.76.1911081318570.5566@zhemvz.fhfr.qr
State New
Headers show
Series
  • Fix PR92088
Related show

Commit Message

Richard Biener Nov. 8, 2019, 12:21 p.m. UTC
The following works around a middle-end limitation not being able
to deal with by value-passing of VLAs transparently during inlining
(but only DECL_BY_REFERENCE is handled) in the C frontend by marking
said functions as not inlinable.  This avoids ICEs later.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

OK for trunk?

Thanks,
Richard.

2019-11-09  Richard Biener  <rguenther@suse.de>

	PR c/92088
	c/
	* c-decl.c (grokdeclarator): Prevent inlining of nested
	function with VLA arguments.

	* builtins.c (compute_objsize): Deal with VLAs.

	* gcc.dg/torture/pr92088-1.c: New testcase.
	* gcc.dg/torture/pr92088-2.c: Likewise.

Comments

Richard Biener Nov. 19, 2019, 8:59 a.m. UTC | #1
On Fri, 8 Nov 2019, Richard Biener wrote:

> 
> The following works around a middle-end limitation not being able
> to deal with by value-passing of VLAs transparently during inlining
> (but only DECL_BY_REFERENCE is handled) in the C frontend by marking
> said functions as not inlinable.  This avoids ICEs later.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> OK for trunk?

Ping.

> Thanks,
> Richard.
> 
> 2019-11-09  Richard Biener  <rguenther@suse.de>
> 
> 	PR c/92088
> 	c/
> 	* c-decl.c (grokdeclarator): Prevent inlining of nested
> 	function with VLA arguments.
> 
> 	* builtins.c (compute_objsize): Deal with VLAs.
> 
> 	* gcc.dg/torture/pr92088-1.c: New testcase.
> 	* gcc.dg/torture/pr92088-2.c: Likewise.
> 
> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c	(revision 277906)
> +++ gcc/builtins.c	(working copy)
> @@ -3708,7 +3708,8 @@ compute_objsize (tree dest, int ostype,
>    if (DECL_P (ref))
>      {
>        *pdecl = ref;
> -      return DECL_SIZE_UNIT (ref);
> +      if (tree size = DECL_SIZE_UNIT (ref))
> +	return TREE_CODE (size) == INTEGER_CST ? size : NULL_TREE;
>      }
>  
>    tree type = TREE_TYPE (dest);
> Index: gcc/c/c-decl.c
> ===================================================================
> --- gcc/c/c-decl.c	(revision 277906)
> +++ gcc/c/c-decl.c	(working copy)
> @@ -7304,6 +7304,23 @@ grokdeclarator (const struct c_declarato
>  	      	    "no linkage");
>        }
>  
> +    /* For nested functions disqualify ones taking VLAs by value
> +       from inlining since the middle-end cannot deal with this.
> +       ???  We should arrange for those to be passed by reference
> +       with emitting the copy on the caller side in the frontend.  */
> +    if (storage_class == csc_none
> +	&& TREE_CODE (type) == FUNCTION_TYPE)
> +      for (tree al = TYPE_ARG_TYPES (type); al; al = TREE_CHAIN (al))
> +	{
> +	  tree arg = TREE_VALUE (al);
> +	  if (arg != error_mark_node
> +	      && C_TYPE_VARIABLE_SIZE (TREE_VALUE (al)))
> +	    {
> +	      DECL_UNINLINABLE (decl) = 1;
> +	      break;
> +	    }
> +	}
> +
>      /* Record `register' declaration for warnings on &
>         and in case doing stupid register allocation.  */
>  
> Index: gcc/testsuite/gcc.dg/torture/pr92088-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/torture/pr92088-1.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/torture/pr92088-1.c	(working copy)
> @@ -0,0 +1,22 @@
> +/* { dg-do run } */
> +
> +int __attribute__((noipa))
> +g (char *p)
> +{
> +  return p[9];
> +}
> +int main (int argc, char **argv)
> +{
> +  struct S {
> +    char toto[argc + 16];
> +  };
> +  int f (struct S arg) {
> +      __builtin_strcpy(arg.toto, "helloworld");
> +      return g (arg.toto);
> +  }
> +  struct S bob;
> +  __builtin_strcpy(bob.toto, "coucoucoucou");
> +  if (f(bob) != 'd' || __builtin_strcmp (bob.toto, "coucoucoucou"))
> +    __builtin_abort ();
> +  return 0;
> +}
> Index: gcc/testsuite/gcc.dg/torture/pr92088-2.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/torture/pr92088-2.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/torture/pr92088-2.c	(working copy)
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +
> +void foo(int n)
> +{
> +  struct X { int a[n]; } y;
> +
> +  struct X baz (struct X x)
> +    {
> +      x.a[0] = 1;
> +      return x;
> +    }
> +
> +  y.a[0] = 0;
> +  y = baz(y);
> +  if (y.a[0] != 1)
> +    __builtin_abort ();
> +}
>
Joseph Myers Nov. 19, 2019, 10:05 p.m. UTC | #2
On Tue, 19 Nov 2019, Richard Biener wrote:

> > +    /* For nested functions disqualify ones taking VLAs by value
> > +       from inlining since the middle-end cannot deal with this.
> > +       ???  We should arrange for those to be passed by reference
> > +       with emitting the copy on the caller side in the frontend.  */
> > +    if (storage_class == csc_none
> > +	&& TREE_CODE (type) == FUNCTION_TYPE)
> > +      for (tree al = TYPE_ARG_TYPES (type); al; al = TREE_CHAIN (al))
> > +	{
> > +	  tree arg = TREE_VALUE (al);
> > +	  if (arg != error_mark_node
> > +	      && C_TYPE_VARIABLE_SIZE (TREE_VALUE (al)))

The second use of TREE_VALUE (al) looks like it would be better as a 
reference to arg.  OK with that change.

Patch
diff mbox series

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 277906)
+++ gcc/builtins.c	(working copy)
@@ -3708,7 +3708,8 @@  compute_objsize (tree dest, int ostype,
   if (DECL_P (ref))
     {
       *pdecl = ref;
-      return DECL_SIZE_UNIT (ref);
+      if (tree size = DECL_SIZE_UNIT (ref))
+	return TREE_CODE (size) == INTEGER_CST ? size : NULL_TREE;
     }
 
   tree type = TREE_TYPE (dest);
Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 277906)
+++ gcc/c/c-decl.c	(working copy)
@@ -7304,6 +7304,23 @@  grokdeclarator (const struct c_declarato
 	      	    "no linkage");
       }
 
+    /* For nested functions disqualify ones taking VLAs by value
+       from inlining since the middle-end cannot deal with this.
+       ???  We should arrange for those to be passed by reference
+       with emitting the copy on the caller side in the frontend.  */
+    if (storage_class == csc_none
+	&& TREE_CODE (type) == FUNCTION_TYPE)
+      for (tree al = TYPE_ARG_TYPES (type); al; al = TREE_CHAIN (al))
+	{
+	  tree arg = TREE_VALUE (al);
+	  if (arg != error_mark_node
+	      && C_TYPE_VARIABLE_SIZE (TREE_VALUE (al)))
+	    {
+	      DECL_UNINLINABLE (decl) = 1;
+	      break;
+	    }
+	}
+
     /* Record `register' declaration for warnings on &
        and in case doing stupid register allocation.  */
 
Index: gcc/testsuite/gcc.dg/torture/pr92088-1.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr92088-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr92088-1.c	(working copy)
@@ -0,0 +1,22 @@ 
+/* { dg-do run } */
+
+int __attribute__((noipa))
+g (char *p)
+{
+  return p[9];
+}
+int main (int argc, char **argv)
+{
+  struct S {
+    char toto[argc + 16];
+  };
+  int f (struct S arg) {
+      __builtin_strcpy(arg.toto, "helloworld");
+      return g (arg.toto);
+  }
+  struct S bob;
+  __builtin_strcpy(bob.toto, "coucoucoucou");
+  if (f(bob) != 'd' || __builtin_strcmp (bob.toto, "coucoucoucou"))
+    __builtin_abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr92088-2.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr92088-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr92088-2.c	(working copy)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+
+void foo(int n)
+{
+  struct X { int a[n]; } y;
+
+  struct X baz (struct X x)
+    {
+      x.a[0] = 1;
+      return x;
+    }
+
+  y.a[0] = 0;
+  y = baz(y);
+  if (y.a[0] != 1)
+    __builtin_abort ();
+}