diff mbox series

Reject VLAs in inline asm operands that require registers (PR inline-asm/92352)

Message ID 20191105073939.GP4650@tucnak
State New
Headers show
Series Reject VLAs in inline asm operands that require registers (PR inline-asm/92352) | expand

Commit Message

Jakub Jelinek Nov. 5, 2019, 7:39 a.m. UTC 
Hi!

On VLAs with register only constraints we ICE, because during gimplification
we try to create temporaries for them and force_constant_size aborts in that
case.

The following patch diagnoses those early, like we diagnose already C++
non-PODs.

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

2019-11-05  Jakub Jelinek  <jakub@redhat.com>

	PR inline-asm/92352
	* gimplify.c (gimplify_asm_expr): Reject VLA in output or input
	operands with non-memory constraints.

	* c-c++-common/pr92352.c: New test.


	Jakub

Comments

Richard Biener Nov. 5, 2019, 8:27 a.m. UTC | #1 
On Tue, 5 Nov 2019, Jakub Jelinek wrote:

> Hi!
> 
> On VLAs with register only constraints we ICE, because during gimplification
> we try to create temporaries for them and force_constant_size aborts in that
> case.
> 
> The following patch diagnoses those early, like we diagnose already C++
> non-PODs.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-11-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR inline-asm/92352
> 	* gimplify.c (gimplify_asm_expr): Reject VLA in output or input
> 	operands with non-memory constraints.
> 
> 	* c-c++-common/pr92352.c: New test.
> 
> --- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
> +++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
> @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
>  	  is_inout = false;
>  	}
>  
> -      /* If we can't make copies, we can only accept memory.  */
> -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> +      /* If we can't make copies, we can only accept memory.
> +	 Similarly for VLAs.  */
> +      tree outtype = TREE_TYPE (TREE_VALUE (link));
> +      if (outtype != error_mark_node

so for error_mark_node we don't diagnose anything?

> +	  && (TREE_ADDRESSABLE (outtype)
> +	      || !COMPLETE_TYPE_P (outtype)
> +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
> +		  && max_int_size_in_bytes (outtype))))

so max_int_size_in_bytes == 0 is OK?  I suppose we have a testcase
for this?

Otherwise looks reasonable to me.

Thanks,
Richard.

>  	{
>  	  if (allows_mem)
>  	    allows_reg = 0;
> @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_
>  			      oconstraints, &allows_mem, &allows_reg);
>  
>        /* If we can't make copies, we can only accept memory.  */
> -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> +      tree intype = TREE_TYPE (TREE_VALUE (link));
> +      if (intype != error_mark_node
> +	  && (TREE_ADDRESSABLE (intype)
> +	      || !COMPLETE_TYPE_P (intype)
> +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype))
> +		  && max_int_size_in_bytes (intype))))
>  	{
>  	  if (allows_mem)
>  	    allows_reg = 0;
> --- gcc/testsuite/c-c++-common/pr92352.c.jj	2019-11-04 14:03:18.725275255 +0100
> +++ gcc/testsuite/c-c++-common/pr92352.c	2019-11-04 14:02:55.211629675 +0100
> @@ -0,0 +1,15 @@
> +/* PR inline-asm/92352 */
> +
> +void
> +foo (int x)
> +{
> +  int var[x];
> +  asm volatile ("" : "+r" (var));	/* { dg-error "impossible constraint in 'asm'" } */
> +}					/* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */
> +
> +void
> +bar (int x)
> +{
> +  int var[x];
> +  asm volatile ("" : "+m" (var));
> +}
> 
> 	Jakub
> 
>
Jakub Jelinek Nov. 5, 2019, 10:24 a.m. UTC | #2 
On Tue, Nov 05, 2019 at 09:27:45AM +0100, Richard Biener wrote:
> > --- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
> > +++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
> > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
> >  	  is_inout = false;
> >  	}
> >  
> > -      /* If we can't make copies, we can only accept memory.  */
> > -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> > +      /* If we can't make copies, we can only accept memory.
> > +	 Similarly for VLAs.  */
> > +      tree outtype = TREE_TYPE (TREE_VALUE (link));
> > +      if (outtype != error_mark_node
> 
> so for error_mark_node we don't diagnose anything?

Yes, we should have diagnosed it already.  The != error_mark_node
I've added only after seeing tons of ICEs in the testsuite with earlier
version of the patch.

> > +	  && (TREE_ADDRESSABLE (outtype)
> > +	      || !COMPLETE_TYPE_P (outtype)
> > +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
> > +		  && max_int_size_in_bytes (outtype))))
> 
> so max_int_size_in_bytes == 0 is OK?  I suppose we have a testcase
> for this?

Actually, I meant max_int_size_in_bytes (outtype) < 0, i.e. something
on which force_constant_size ICE immediately, sorry for screwing it up in
the end.
All these VLAs with max_int_size_in_bytes >= 0 sizes are specific to Ada
and I have no idea what is and isn't valid there, for C/C++ it should
always return -1.

> Otherwise looks reasonable to me.

So, is the following ok if it passes bootstrap/regtest, or shall
I just go for || !tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))) ?

2019-11-05  Jakub Jelinek  <jakub@redhat.com>

	PR inline-asm/92352
	* gimplify.c (gimplify_asm_expr): Reject VLA in output or input
	operands with non-memory constraints.

	* c-c++-common/pr92352.c: New test.

--- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
+++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
@@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
 	  is_inout = false;
 	}
 
-      /* If we can't make copies, we can only accept memory.  */
-      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
+      /* If we can't make copies, we can only accept memory.
+	 Similarly for VLAs.  */
+      tree outtype = TREE_TYPE (TREE_VALUE (link));
+      if (outtype != error_mark_node
+	  && (TREE_ADDRESSABLE (outtype)
+	      || !COMPLETE_TYPE_P (outtype)
+	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
+		  && max_int_size_in_bytes (outtype) < 0)))
 	{
 	  if (allows_mem)
 	    allows_reg = 0;
@@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_
 			      oconstraints, &allows_mem, &allows_reg);
 
       /* If we can't make copies, we can only accept memory.  */
-      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
+      tree intype = TREE_TYPE (TREE_VALUE (link));
+      if (intype != error_mark_node
+	  && (TREE_ADDRESSABLE (intype)
+	      || !COMPLETE_TYPE_P (intype)
+	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype))
+		  && max_int_size_in_bytes (intype) < 0)))
 	{
 	  if (allows_mem)
 	    allows_reg = 0;
--- gcc/testsuite/c-c++-common/pr92352.c.jj	2019-11-04 14:03:18.725275255 +0100
+++ gcc/testsuite/c-c++-common/pr92352.c	2019-11-04 14:02:55.211629675 +0100
@@ -0,0 +1,15 @@
+/* PR inline-asm/92352 */
+
+void
+foo (int x)
+{
+  int var[x];
+  asm volatile ("" : "+r" (var));	/* { dg-error "impossible constraint in 'asm'" } */
+}					/* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */
+
+void
+bar (int x)
+{
+  int var[x];
+  asm volatile ("" : "+m" (var));
+}


	Jakub
Richard Biener Nov. 5, 2019, 10:44 a.m. UTC | #3 
On Tue, 5 Nov 2019, Jakub Jelinek wrote:

> On Tue, Nov 05, 2019 at 09:27:45AM +0100, Richard Biener wrote:
> > > --- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
> > > +++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
> > > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
> > >  	  is_inout = false;
> > >  	}
> > >  
> > > -      /* If we can't make copies, we can only accept memory.  */
> > > -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> > > +      /* If we can't make copies, we can only accept memory.
> > > +	 Similarly for VLAs.  */
> > > +      tree outtype = TREE_TYPE (TREE_VALUE (link));
> > > +      if (outtype != error_mark_node
> > 
> > so for error_mark_node we don't diagnose anything?
> 
> Yes, we should have diagnosed it already.  The != error_mark_node
> I've added only after seeing tons of ICEs in the testsuite with earlier
> version of the patch.
> 
> > > +	  && (TREE_ADDRESSABLE (outtype)
> > > +	      || !COMPLETE_TYPE_P (outtype)
> > > +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
> > > +		  && max_int_size_in_bytes (outtype))))
> > 
> > so max_int_size_in_bytes == 0 is OK?  I suppose we have a testcase
> > for this?
> 
> Actually, I meant max_int_size_in_bytes (outtype) < 0, i.e. something
> on which force_constant_size ICE immediately, sorry for screwing it up in
> the end.
> All these VLAs with max_int_size_in_bytes >= 0 sizes are specific to Ada
> and I have no idea what is and isn't valid there, for C/C++ it should
> always return -1.
> 
> > Otherwise looks reasonable to me.
> 
> So, is the following ok if it passes bootstrap/regtest, or shall
> I just go for || !tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))) ?

Hmm, can we actually copy VLAs with max_int_size_in_bytes != -1?  I 
suppose we can copy even unconstrained VLAs, we just miss a
WITH_SIZE_EXPR here but then it's unlikely we ever fit that into
a non-memory.  That's true as well if we just know the max size,
so allowing that seems suspicious at best.

So I'd say OK with just !tree_fits_poly_uint64_p.

Thanks,
Richard.

> 2019-11-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR inline-asm/92352
> 	* gimplify.c (gimplify_asm_expr): Reject VLA in output or input
> 	operands with non-memory constraints.
> 
> 	* c-c++-common/pr92352.c: New test.
> 
> --- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
> +++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
> @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_
>  	  is_inout = false;
>  	}
>  
> -      /* If we can't make copies, we can only accept memory.  */
> -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> +      /* If we can't make copies, we can only accept memory.
> +	 Similarly for VLAs.  */
> +      tree outtype = TREE_TYPE (TREE_VALUE (link));
> +      if (outtype != error_mark_node
> +	  && (TREE_ADDRESSABLE (outtype)
> +	      || !COMPLETE_TYPE_P (outtype)
> +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
> +		  && max_int_size_in_bytes (outtype) < 0)))
>  	{
>  	  if (allows_mem)
>  	    allows_reg = 0;
> @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_
>  			      oconstraints, &allows_mem, &allows_reg);
>  
>        /* If we can't make copies, we can only accept memory.  */
> -      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
> +      tree intype = TREE_TYPE (TREE_VALUE (link));
> +      if (intype != error_mark_node
> +	  && (TREE_ADDRESSABLE (intype)
> +	      || !COMPLETE_TYPE_P (intype)
> +	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype))
> +		  && max_int_size_in_bytes (intype) < 0)))
>  	{
>  	  if (allows_mem)
>  	    allows_reg = 0;
> --- gcc/testsuite/c-c++-common/pr92352.c.jj	2019-11-04 14:03:18.725275255 +0100
> +++ gcc/testsuite/c-c++-common/pr92352.c	2019-11-04 14:02:55.211629675 +0100
> @@ -0,0 +1,15 @@
> +/* PR inline-asm/92352 */
> +
> +void
> +foo (int x)
> +{
> +  int var[x];
> +  asm volatile ("" : "+r" (var));	/* { dg-error "impossible constraint in 'asm'" } */
> +}					/* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */
> +
> +void
> +bar (int x)
> +{
> +  int var[x];
> +  asm volatile ("" : "+m" (var));
> +}
> 
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- gcc/gimplify.c.jj	2019-11-02 10:00:59.595253274 +0100
+++ gcc/gimplify.c	2019-11-05 00:21:01.585958514 +0100
@@ -6235,8 +6235,14 @@  gimplify_asm_expr (tree *expr_p, gimple_
 	  is_inout = false;
 	}
 
-      /* If we can't make copies, we can only accept memory.  */
-      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
+      /* If we can't make copies, we can only accept memory.
+	 Similarly for VLAs.  */
+      tree outtype = TREE_TYPE (TREE_VALUE (link));
+      if (outtype != error_mark_node
+	  && (TREE_ADDRESSABLE (outtype)
+	      || !COMPLETE_TYPE_P (outtype)
+	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))
+		  && max_int_size_in_bytes (outtype))))
 	{
 	  if (allows_mem)
 	    allows_reg = 0;
@@ -6392,7 +6398,12 @@  gimplify_asm_expr (tree *expr_p, gimple_
 			      oconstraints, &allows_mem, &allows_reg);
 
       /* If we can't make copies, we can only accept memory.  */
-      if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link))))
+      tree intype = TREE_TYPE (TREE_VALUE (link));
+      if (intype != error_mark_node
+	  && (TREE_ADDRESSABLE (intype)
+	      || !COMPLETE_TYPE_P (intype)
+	      || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype))
+		  && max_int_size_in_bytes (intype))))
 	{
 	  if (allows_mem)
 	    allows_reg = 0;
--- gcc/testsuite/c-c++-common/pr92352.c.jj	2019-11-04 14:03:18.725275255 +0100
+++ gcc/testsuite/c-c++-common/pr92352.c	2019-11-04 14:02:55.211629675 +0100
@@ -0,0 +1,15 @@ 
+/* PR inline-asm/92352 */
+
+void
+foo (int x)
+{
+  int var[x];
+  asm volatile ("" : "+r" (var));	/* { dg-error "impossible constraint in 'asm'" } */
+}					/* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */
+
+void
+bar (int x)
+{
+  int var[x];
+  asm volatile ("" : "+m" (var));
+}