V5, #5 of 15: Support -fstack-protect and large stack frames
diff mbox series

Message ID 20191009202206.GE2063@ibm-toto.the-meissners.org
State New
Headers show
Series
  • V5, #5 of 15: Support -fstack-protect and large stack frames
Related show

Commit Message

Michael Meissner Oct. 9, 2019, 8:22 p.m. UTC
This patch allows -fstack-protect to work with large stack frames.

It does so by adding a new constraint (em) that matches normal memory that does
not use a prefixed address (including PC-relative addresses).

The stack protect test and set functions now can take indexed address forms,
and there is a define_expand to make sure memory would not use a prefixed
address.

Along with the other patches, I have done bootstraps on a little endian power8
system, and there were no regressions in the test suite.  Can I check this into
the trunk?

2019-10-08  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/constraints.md (em constraint): New constraint.
	* config/rs6000/rs6000-protos.h (make_memory_non_prefixed): New
	declaration.
	* config/rs6000/rs6000.c (make_memory_non_prefixed): New function
	to return a traditional non-prefixed instruction.
	* config/rs6000/rs6000.md (stack_protect_setdi): Make sure both
	memory operands are not	prefixed.
	(stack_protect_testdi): Make sure both memory operands are not
	prefixed.
	* doc/md.texi (PowerPC constraints): Document the em constraint.

Comments

Segher Boessenkool Oct. 11, 2019, 9:44 p.m. UTC | #1
On Wed, Oct 09, 2019 at 04:22:06PM -0400, Michael Meissner wrote:
> This patch allows -fstack-protect to work with large stack frames.

I don't understand; please explain.

What I see is a workaround for not properly handling prefixed addresses
in the stack protect code (by forcing the addresses to not be prefixed at
expand time).

> +rtx
> +make_memory_non_prefixed (rtx mem)
> +{
> +  gcc_assert (MEM_P (mem));
> +
> +  rtx old_addr = XEXP (mem, 0);
> +  if (address_is_prefixed (old_addr, GET_MODE (mem), NON_PREFIXED_DEFAULT))
> +    {
> +      rtx new_addr;
> +
> +      if (GET_CODE (old_addr) == PLUS
> +	  && (REG_P (XEXP (old_addr, 0)) || SUBREG_P (XEXP (old_addr, 0)))

How can you ever have a subreg in an address?  One in Pmode even?

> +	  && CONST_INT_P (XEXP (old_addr, 1)))
> +	{
> +	  rtx tmp_reg = force_reg (Pmode, XEXP (old_addr, 1));
> +	  new_addr = gen_rtx_PLUS (Pmode, XEXP (old_addr, 0), tmp_reg);
> +	}
> +      else
> +	new_addr = force_reg (Pmode, old_addr);
> +
> +      mem = change_address (mem, VOIDmode, new_addr);

replace_equiv_address ?

> +(define_expand "stack_protect_setdi"
> +  [(parallel [(set (match_operand:DI 0 "memory_operand")
> +		   (unspec:DI [(match_operand:DI 1 "memory_operand")]
> +		   UNSPEC_SP_SET))
> +	      (set (match_scratch:DI 2)
> +		   (const_int 0))])]
> +  "TARGET_64BIT"
> +{
> +  if (TARGET_PREFIXED_ADDR)
> +    {
> +      operands[0] = make_memory_non_prefixed (operands[0]);
> +      operands[1] = make_memory_non_prefixed (operands[1]);
> +    }
> +})

It shouldn't be terribly hard to make the define_insns just *work* with
prefixed insns, instead?  Is there any reason we are sure these memory
references will not turn into something that needs prefixed insns, after
expand?  It seems fragile like this.

> +@item em
> +A memory operand that does not contain a prefixed address.

"A memory operand that does not require a prefixed instruction"?


Segher
Michael Meissner Oct. 14, 2019, 9:31 p.m. UTC | #2
On Fri, Oct 11, 2019 at 04:44:51PM -0500, Segher Boessenkool wrote:
> On Wed, Oct 09, 2019 at 04:22:06PM -0400, Michael Meissner wrote:
> > This patch allows -fstack-protect to work with large stack frames.
> 
> I don't understand; please explain.
> 
> What I see is a workaround for not properly handling prefixed addresses
> in the stack protect code (by forcing the addresses to not be prefixed at
> expand time).

Several iterations ago, you explicitly told me to not to allow prefixed insns
here.  So I made it to use traditional instructions.

If you recall the code was:

(define_insn "stack_protect_testdi"
  [(set (match_operand:CCEQ 0 "cc_reg_operand" "=x,?y")
        (unspec:CCEQ [(match_operand:DI 1 "memory_operand" "Y,Y")
		      (match_operand:DI 2 "memory_operand" "Y,Y")]
		     UNSPEC_SP_TEST))
   (set (match_scratch:DI 4 "=r,r") (const_int 0))
   (clobber (match_scratch:DI 3 "=&r,&r"))]
  "TARGET_64BIT"
{
  if (prefixed_mem_operand (operands[1], DImode))
    output_asm_insn ("pld %3,%1", operands);
  else
    output_asm_insn ("ld%U1%X1 %3,%1", operands);

  if (prefixed_mem_operand (operands[2], DImode))
    output_asm_insn ("pld %4,%2", operands);
  else
    output_asm_insn ("ld%U2%X2 %4,%2", operands);

  if (which_alternative == 0)
    output_asm_insn ("xor. %3,%3,%4", operands);
  else
    output_asm_insn ("cmpld %0,%3,%4\;li %3,0", operands);

  return "li %4,0";
}
  ;; Back to back prefixed memory instructions take 20 bytes (8 bytes for each
  ;; prefixed instruction + 4 bytes for the possible NOP).
  [(set (attr "length")
	(cond [(and (match_operand 1 "prefixed_mem_operand")
		    (match_operand 2 "prefixed_mem_operand"))
	       (if_then_else (eq_attr "alternative" "0")
			     (const_string "28")
			     (const_string "32"))

	       (ior (match_operand 1 "prefixed_mem_operand")
		    (match_operand 2 "prefixed_mem_operand"))
	       (if_then_else (eq_attr "alternative" "0")
			     (const_string "20")
			     (const_string "24"))]

	      (if_then_else (eq_attr "alternative" "0")
			    (const_string "16")
			    (const_string "20"))))
   (set_attr "prefixed" "no")])

It can't use the normal prefixed support used in other insns because this one
has multiple insns (hence the 'p' trick for ASM_OUTPUT_OPCODE won't work) and
two memory addresses.

> > +rtx
> > +make_memory_non_prefixed (rtx mem)
> > +{
> > +  gcc_assert (MEM_P (mem));
> > +
> > +  rtx old_addr = XEXP (mem, 0);
> > +  if (address_is_prefixed (old_addr, GET_MODE (mem), NON_PREFIXED_DEFAULT))
> > +    {
> > +      rtx new_addr;
> > +
> > +      if (GET_CODE (old_addr) == PLUS
> > +	  && (REG_P (XEXP (old_addr, 0)) || SUBREG_P (XEXP (old_addr, 0)))
> 
> How can you ever have a subreg in an address?  One in Pmode even?

I could imagine having a union or something similar that generates a subreg.

> 
> > +	  && CONST_INT_P (XEXP (old_addr, 1)))
> > +	{
> > +	  rtx tmp_reg = force_reg (Pmode, XEXP (old_addr, 1));
> > +	  new_addr = gen_rtx_PLUS (Pmode, XEXP (old_addr, 0), tmp_reg);
> > +	}
> > +      else
> > +	new_addr = force_reg (Pmode, old_addr);
> > +
> > +      mem = change_address (mem, VOIDmode, new_addr);
> 
> replace_equiv_address ?
> 
> > +(define_expand "stack_protect_setdi"
> > +  [(parallel [(set (match_operand:DI 0 "memory_operand")
> > +		   (unspec:DI [(match_operand:DI 1 "memory_operand")]
> > +		   UNSPEC_SP_SET))
> > +	      (set (match_scratch:DI 2)
> > +		   (const_int 0))])]
> > +  "TARGET_64BIT"
> > +{
> > +  if (TARGET_PREFIXED_ADDR)
> > +    {
> > +      operands[0] = make_memory_non_prefixed (operands[0]);
> > +      operands[1] = make_memory_non_prefixed (operands[1]);
> > +    }
> > +})
> 
> It shouldn't be terribly hard to make the define_insns just *work* with
> prefixed insns, instead?  Is there any reason we are sure these memory
> references will not turn into something that needs prefixed insns, after
> expand?  It seems fragile like this.

As I said, I've done it in the past.  It is complicated because this insn must
generate two or more insns without spliting.  But it is doable.

Patch
diff mbox series

Index: gcc/config/rs6000/constraints.md
===================================================================
--- gcc/config/rs6000/constraints.md	(revision 276713)
+++ gcc/config/rs6000/constraints.md	(working copy)
@@ -210,6 +210,11 @@  several times, or that might not access
   (and (match_code "mem")
        (match_test "GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC")))
 
+(define_memory_constraint "em"
+  "A memory operand that does not contain a prefixed address."
+  (and (match_code "mem")
+       (match_test "non_prefixed_memory (op, mode)")))
+
 (define_memory_constraint "Q"
   "Memory operand that is an offset from a register (it is usually better
 to use @samp{m} or @samp{es} in @code{asm} statements)"
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 276713)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -192,6 +192,7 @@  extern enum insn_form address_to_insn_fo
 extern bool prefixed_load_p (rtx_insn *);
 extern bool prefixed_store_p (rtx_insn *);
 extern bool prefixed_paddi_p (rtx_insn *);
+extern rtx make_memory_non_prefixed (rtx);
 extern void rs6000_asm_output_opcode (FILE *);
 extern void rs6000_final_prescan_insn (rtx_insn *, rtx [], int);
 
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 276717)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -24972,6 +24972,34 @@  prefixed_paddi_p (rtx_insn *insn)
   return (iform == INSN_FORM_PCREL_EXTERNAL || iform == INSN_FORM_PCREL_LOCAL);
 }
 
+/* Make a memory address non-prefixed if it is prefixed.  */
+
+rtx
+make_memory_non_prefixed (rtx mem)
+{
+  gcc_assert (MEM_P (mem));
+
+  rtx old_addr = XEXP (mem, 0);
+  if (address_is_prefixed (old_addr, GET_MODE (mem), NON_PREFIXED_DEFAULT))
+    {
+      rtx new_addr;
+
+      if (GET_CODE (old_addr) == PLUS
+	  && (REG_P (XEXP (old_addr, 0)) || SUBREG_P (XEXP (old_addr, 0)))
+	  && CONST_INT_P (XEXP (old_addr, 1)))
+	{
+	  rtx tmp_reg = force_reg (Pmode, XEXP (old_addr, 1));
+	  new_addr = gen_rtx_PLUS (Pmode, XEXP (old_addr, 0), tmp_reg);
+	}
+      else
+	new_addr = force_reg (Pmode, old_addr);
+
+      mem = change_address (mem, VOIDmode, new_addr);
+    }
+
+  return mem;
+}
+
 /* Whether the next instruction needs a 'p' prefix issued before the
    instruction is printed out.  */
 static bool next_insn_prefixed_p;
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 276716)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11531,9 +11531,25 @@  (define_insn "stack_protect_setsi"
   [(set_attr "type" "three")
    (set_attr "length" "12")])
 
-(define_insn "stack_protect_setdi"
-  [(set (match_operand:DI 0 "memory_operand" "=Y")
-	(unspec:DI [(match_operand:DI 1 "memory_operand" "Y")] UNSPEC_SP_SET))
+(define_expand "stack_protect_setdi"
+  [(parallel [(set (match_operand:DI 0 "memory_operand")
+		   (unspec:DI [(match_operand:DI 1 "memory_operand")]
+		   UNSPEC_SP_SET))
+	      (set (match_scratch:DI 2)
+		   (const_int 0))])]
+  "TARGET_64BIT"
+{
+  if (TARGET_PREFIXED_ADDR)
+    {
+      operands[0] = make_memory_non_prefixed (operands[0]);
+      operands[1] = make_memory_non_prefixed (operands[1]);
+    }
+})
+
+(define_insn "*stack_protect_setdi"
+  [(set (match_operand:DI 0 "non_prefixed_memory" "=em")
+	(unspec:DI [(match_operand:DI 1 "non_prefixed_memory" "em")]
+		   UNSPEC_SP_SET))
    (set (match_scratch:DI 2 "=&r") (const_int 0))]
   "TARGET_64BIT"
   "ld%U1%X1 %2,%1\;std%U0%X0 %2,%0\;li %2,0"
@@ -11577,10 +11593,27 @@  (define_insn "stack_protect_testsi"
    lwz%U1%X1 %3,%1\;lwz%U2%X2 %4,%2\;cmplw %0,%3,%4\;li %3,0\;li %4,0"
   [(set_attr "length" "16,20")])
 
-(define_insn "stack_protect_testdi"
+(define_expand "stack_protect_testdi"
+  [(parallel [(set (match_operand:CCEQ 0 "cc_reg_operand")
+		   (unspec:CCEQ [(match_operand:DI 1 "memory_operand")
+				 (match_operand:DI 2 "memory_operand")]
+				UNSPEC_SP_TEST))
+	      (set (match_scratch:DI 4)
+		   (const_int 0))
+	      (clobber (match_scratch:DI 3))])]
+  "TARGET_64BIT"
+{
+  if (TARGET_PREFIXED_ADDR)
+    {
+      operands[1] = make_memory_non_prefixed (operands[1]);
+      operands[2] = make_memory_non_prefixed (operands[2]);
+    }
+})
+
+(define_insn "*stack_protect_testdi"
   [(set (match_operand:CCEQ 0 "cc_reg_operand" "=x,?y")
-        (unspec:CCEQ [(match_operand:DI 1 "memory_operand" "Y,Y")
-		      (match_operand:DI 2 "memory_operand" "Y,Y")]
+        (unspec:CCEQ [(match_operand:DI 1 "non_prefixed_memory" "em,em")
+		      (match_operand:DI 2 "non_prefixed_memory" "em,em")]
 		     UNSPEC_SP_TEST))
    (set (match_scratch:DI 4 "=r,r") (const_int 0))
    (clobber (match_scratch:DI 3 "=&r,&r"))]
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi	(revision 276713)
+++ gcc/doc/md.texi	(working copy)
@@ -3373,6 +3373,9 @@  asm ("st %1,%0" : "=m<>" (mem) : "r" (va
 
 is not.
 
+@item em
+A memory operand that does not contain a prefixed address.
+
 @item es
 A ``stable'' memory operand; that is, one which does not include any
 automodification of the base register.  This used to be useful when