Ensure x86_64 TYPE_EMPTY_P args have distinct addresses (PR c++/92384)
diff mbox series

Message ID 20191107083443.GD4650@tucnak
State New
Headers show
Series
  • Ensure x86_64 TYPE_EMPTY_P args have distinct addresses (PR c++/92384)
Related show

Commit Message

Jakub Jelinek Nov. 7, 2019, 8:34 a.m. UTC
Hi!

TYPE_EMPTY_P arguments (which right now only x86_64 uses) have
data->entry_parm == data->stack_parm being a stack slot with zero size in
the stack parameter passing area.
The problem with that is that in C++ they should have distinct addresses
from other objects, which is not the case right now, as their address is
equal to whatever argument is after it on the stack.

The following patch in the third hunk, if the TYPE_EMPTY_P argument has address
taken forces stack_parm to be NULL, so that a new slot will be created for
it, and the other two hunks just make sure we don't actually copy anything,
as the TYPE_EMPTY_P types contain solely padding and so it is ok if they
are just allocated on the stack, but are not copied.

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

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

	PR c++/92384
	* function.c (assign_parm_setup_block, assign_parm_setup_stack): Don't
	copy TYPE_EMPTY_P arguments from data->entry_parm to data->stack_parm
	slot.
	(assign_parms): For TREE_ADDRESSABLE parms with TYPE_EMPTY_P type
	force creation of a unique data.stack_parm slot.

	* g++.dg/torture/pr92384.C: New test.


	Jakub

Comments

Richard Biener Nov. 8, 2019, 10:27 a.m. UTC | #1
On Thu, 7 Nov 2019, Jakub Jelinek wrote:

> Hi!
> 
> TYPE_EMPTY_P arguments (which right now only x86_64 uses) have
> data->entry_parm == data->stack_parm being a stack slot with zero size in
> the stack parameter passing area.
> The problem with that is that in C++ they should have distinct addresses
> from other objects, which is not the case right now, as their address is
> equal to whatever argument is after it on the stack.
> 
> The following patch in the third hunk, if the TYPE_EMPTY_P argument has address
> taken forces stack_parm to be NULL, so that a new slot will be created for
> it, and the other two hunks just make sure we don't actually copy anything,
> as the TYPE_EMPTY_P types contain solely padding and so it is ok if they
> are just allocated on the stack, but are not copied.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2019-11-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/92384
> 	* function.c (assign_parm_setup_block, assign_parm_setup_stack): Don't
> 	copy TYPE_EMPTY_P arguments from data->entry_parm to data->stack_parm
> 	slot.
> 	(assign_parms): For TREE_ADDRESSABLE parms with TYPE_EMPTY_P type
> 	force creation of a unique data.stack_parm slot.
> 
> 	* g++.dg/torture/pr92384.C: New test.
> 
> --- gcc/function.c.jj	2019-10-01 18:16:13.205128183 +0200
> +++ gcc/function.c	2019-11-06 09:30:47.097353832 +0100
> @@ -3087,7 +3087,7 @@ assign_parm_setup_block (struct assign_p
>  	move_block_from_reg (REGNO (entry_parm), mem,
>  			     size_stored / UNITS_PER_WORD);
>      }
> -  else if (data->stack_parm == 0)
> +  else if (data->stack_parm == 0 && !TYPE_EMPTY_P (data->arg.type))
>      {
>        push_to_sequence2 (all->first_conversion_insn, all->last_conversion_insn);
>        emit_block_move (stack_parm, data->entry_parm, GEN_INT (size),
> @@ -3488,7 +3488,9 @@ assign_parm_setup_stack (struct assign_p
>        dest = validize_mem (copy_rtx (data->stack_parm));
>        src = validize_mem (copy_rtx (data->entry_parm));
>  
> -      if (MEM_P (src))
> +      if (TYPE_EMPTY_P (data->arg.type))
> +	/* Empty types don't really need to be copied.  */;
> +      else if (MEM_P (src))
>  	{
>  	  /* Use a block move to handle potentially misaligned entry_parm.  */
>  	  if (!to_conversion)
> @@ -3643,6 +3645,16 @@ assign_parms (tree fndecl)
>  	{
>  	  assign_parm_find_stack_rtl (parm, &data);
>  	  assign_parm_adjust_entry_rtl (&data);
> +	  /* For arguments that occupy no space in the parameter
> +	     passing area, have non-zero size and have address taken,
> +	     force creation of a stack slot so that they have distinct
> +	     address from other parameters.  */
> +	  if (TYPE_EMPTY_P (data.arg.type)
> +	      && TREE_ADDRESSABLE (parm)
> +	      && data.entry_parm == data.stack_parm
> +	      && MEM_P (data.entry_parm)
> +	      && int_size_in_bytes (data.arg.type))
> +	    data.stack_parm = NULL_RTX;
>  	}
>        /* Record permanently how this parm was passed.  */
>        if (data.arg.pass_by_reference)
> --- gcc/testsuite/g++.dg/torture/pr92384.C.jj	2019-11-06 10:01:39.794413342 +0100
> +++ gcc/testsuite/g++.dg/torture/pr92384.C	2019-11-06 10:02:44.281441632 +0100
> @@ -0,0 +1,38 @@
> +// PR c++/92384
> +// { dg-do run }
> +
> +struct S {};
> +struct T : public S { S a, b, c, d, e, f, g, h, i, j, k, l, m; };
> +struct U { long long a, b, c; };
> +
> +U
> +foo (S, S, S, T, T, T, U g)
> +{
> +  return g;
> +}
> +
> +__attribute__((noipa)) bool
> +bar (S a, S b, S c, T d, T e, T f, U g, void **h)
> +{
> +  h[0] = (void *) &a;
> +  h[1] = (void *) &b;
> +  h[2] = (void *) &c;
> +  h[3] = (void *) &d;
> +  h[4] = (void *) &e;
> +  h[5] = (void *) &f;
> +  h[6] = (void *) &g;
> +  asm volatile ("" : : "r" (h) : "memory");
> +  return (h[0] != h[1] && h[1] != h[2] && h[2] != h[3]
> +	  && h[3] != h[4] && h[4] != h[5] && h[5] != h[6]);
> +}
> +
> +int
> +main ()
> +{
> +  S a;
> +  T b;
> +  U c = { 1, 2, 3 };
> +  void *d[7];
> +  if (!bar (a, a, a, b, b, b, c, d))
> +    __builtin_abort ();
> +}
> 
> 	Jakub
> 
>

Patch
diff mbox series

--- gcc/function.c.jj	2019-10-01 18:16:13.205128183 +0200
+++ gcc/function.c	2019-11-06 09:30:47.097353832 +0100
@@ -3087,7 +3087,7 @@  assign_parm_setup_block (struct assign_p
 	move_block_from_reg (REGNO (entry_parm), mem,
 			     size_stored / UNITS_PER_WORD);
     }
-  else if (data->stack_parm == 0)
+  else if (data->stack_parm == 0 && !TYPE_EMPTY_P (data->arg.type))
     {
       push_to_sequence2 (all->first_conversion_insn, all->last_conversion_insn);
       emit_block_move (stack_parm, data->entry_parm, GEN_INT (size),
@@ -3488,7 +3488,9 @@  assign_parm_setup_stack (struct assign_p
       dest = validize_mem (copy_rtx (data->stack_parm));
       src = validize_mem (copy_rtx (data->entry_parm));
 
-      if (MEM_P (src))
+      if (TYPE_EMPTY_P (data->arg.type))
+	/* Empty types don't really need to be copied.  */;
+      else if (MEM_P (src))
 	{
 	  /* Use a block move to handle potentially misaligned entry_parm.  */
 	  if (!to_conversion)
@@ -3643,6 +3645,16 @@  assign_parms (tree fndecl)
 	{
 	  assign_parm_find_stack_rtl (parm, &data);
 	  assign_parm_adjust_entry_rtl (&data);
+	  /* For arguments that occupy no space in the parameter
+	     passing area, have non-zero size and have address taken,
+	     force creation of a stack slot so that they have distinct
+	     address from other parameters.  */
+	  if (TYPE_EMPTY_P (data.arg.type)
+	      && TREE_ADDRESSABLE (parm)
+	      && data.entry_parm == data.stack_parm
+	      && MEM_P (data.entry_parm)
+	      && int_size_in_bytes (data.arg.type))
+	    data.stack_parm = NULL_RTX;
 	}
       /* Record permanently how this parm was passed.  */
       if (data.arg.pass_by_reference)
--- gcc/testsuite/g++.dg/torture/pr92384.C.jj	2019-11-06 10:01:39.794413342 +0100
+++ gcc/testsuite/g++.dg/torture/pr92384.C	2019-11-06 10:02:44.281441632 +0100
@@ -0,0 +1,38 @@ 
+// PR c++/92384
+// { dg-do run }
+
+struct S {};
+struct T : public S { S a, b, c, d, e, f, g, h, i, j, k, l, m; };
+struct U { long long a, b, c; };
+
+U
+foo (S, S, S, T, T, T, U g)
+{
+  return g;
+}
+
+__attribute__((noipa)) bool
+bar (S a, S b, S c, T d, T e, T f, U g, void **h)
+{
+  h[0] = (void *) &a;
+  h[1] = (void *) &b;
+  h[2] = (void *) &c;
+  h[3] = (void *) &d;
+  h[4] = (void *) &e;
+  h[5] = (void *) &f;
+  h[6] = (void *) &g;
+  asm volatile ("" : : "r" (h) : "memory");
+  return (h[0] != h[1] && h[1] != h[2] && h[2] != h[3]
+	  && h[3] != h[4] && h[4] != h[5] && h[5] != h[6]);
+}
+
+int
+main ()
+{
+  S a;
+  T b;
+  U c = { 1, 2, 3 };
+  void *d[7];
+  if (!bar (a, a, a, b, b, b, c, d))
+    __builtin_abort ();
+}