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.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797

Reply via email to