Hi Jeff,
Sorry for the delay in reviewing this. It mostly looks good :-)
On Sun, Jul 30, 2017 at 11:45:16PM -0600, Jeff Law wrote:
>
> This contains the PPC bits for stack clash protection.
>
> Changes since V2:
>
> Exploits inlined/unrolled probes and rotated loops for the dynamic area.
> Some trivial simplifications. It also uses the new params to control
> if probes are needed and how often to probe.
> * config/rs6000/rs6000-protos.h (output_probe_stack_range): Update
> prototype for new argument.
> * config/rs6000/rs6000.c (wrap_frame_mem): New function extracted
> from rs6000_emit_allocate_stack.
> (handle_residual): New function.
Trailing space.
> (rs6000_emit_probe_stack_range_stack_clash): New function.
> (rs6000_emit_allocate_stack): Use wrap_frame_mem.
> Call rs6000_emit_probe_stack_range_stack_clash as needed.
> (rs6000_emit_probe_stack_range): Add additional argument
> to call to gen_probe_stack_range{si,di}.
> (output_probe_stack_range): New.
> (output_probe_stack_range_1): Renamed from output_probe_stack_range.
Double space.
> (output_probe_stack_range_stack_clash): New.
> (rs6000_emit_prologue): Emit notes into dump file as requested.
> * rs6000.md (allocate_stack): Handle -fstack-clash-protection
Missing full stop.
> (probe_stack_range<P:mode>): Operand 0 is now early-clobbered.
> Add additional operand and pass it to output_probe_stack_range.
> +/* INSN allocates SIZE bytes on the stack (STACK_REG) using a store
> + with update style insn.
> +
> + Set INSN's alias set/attributes and add suitable flags and notes
> + for the dwarf CFI machinery. */
> +static void
> +wrap_frame_mem (rtx insn, rtx stack_reg, HOST_WIDE_INT size)
This isn't such a great name... Something with "frame allocate"? And
the "wrap" part is meaningless... "set_attrs_for_frame_allocation"?
Or maybe "stack allocation" is better.
Actually, everywhere it is used it has a Pmode == SImode wart before
it, to emit the right update_stack insn... So fold that into this
function, name it rs6000_emit_allocate_stack_1?
> +/* Allocate ROUNDED_SIZE - ORIG_SIZE bytes on the stack, storing
> + ORIG_SP into *sp after the allocation.
This is a bit misleading: it has to store it at the _same time_ as doing
any change to r1. Or, more precisely, it has to ensure r1 points at a
valid stack backchain at all times. Since the red zone is pretty small
(if it exists at all, it doesn't on all ABIs) you need atomic store-with-
update in most cases.
> +static rtx_insn *
> +handle_residual (HOST_WIDE_INT orig_size,
> + HOST_WIDE_INT rounded_size,
> + rtx orig_sp)
Not a great name either. Since this is only used once, and it is hard
to make a good name for it, and it is only a handful of statements, just
inline it?
> +/* Allocate ORIG_SIZE bytes on the stack and probe the newly
> + allocated space every STACK_CLASH_PROTECTION_PROBE_INTERVAL bytes.
> +
> + COPY_REG, if non-null, should contain a copy of the original
> + stack pointer modified by COPY_OFF at exit from this function.
> +
> + This is subtly different than the Ada probing in that it tries hard to
> + prevent attacks that jump the stack guard. Thus it is never allowed to
> + allocate more than STACK_CLASH_PROTECTION_PROBE_INTERVAL bytes of stack
> + space without a suitable probe. */
Please handle the COPY_OFF part in the (single) caller of this, that
will simplify things a little I think?
You don't describe what the return value here is (but neither does
rs6000_emit_allocate_stack); it is the single instruction that actually
changes the stack pointer? For the split stack support? (Does the stack
clash code actually work with split stack, has that been tested?)
Maybe we should keep track of that sp_adjust insn in a global, or in
machine_function, or even in the stack info struct.
> +static rtx_insn *
> +rs6000_emit_probe_stack_range_stack_clash (HOST_WIDE_INT orig_size,
> + rtx copy_reg, int copy_off)
> +{
> + rtx orig_sp = copy_reg;
> +
> + HOST_WIDE_INT probe_interval
> + = PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
> +
> + /* Round the size down to a multiple of PROBE_INTERVAL. */
> + HOST_WIDE_INT rounded_size = orig_size & -probe_interval;
Use ROUND_DOWN?
> +
> + /* If explicitly requested,
> + or the rounded size is not the same as the original size
> + or the the rounded size is greater than a page,
> + then we will need a copy of the original stack pointer. */
> + if (rounded_size != orig_size
> + || rounded_size > probe_interval
> + || copy_reg)
> + {
> + /* If the caller requested a copy of the incoming stack pointer,
> + then ORIG_SP == COPY_REG and will not be NULL.
> +
> + If no copy was requested, then we use r0 to hold the copy. */
> + if (orig_sp == NULL_RTX)
> + orig_sp = gen_rtx_REG (Pmode, 0);
> + emit_move_insn (orig_sp, stack_pointer_rtx);
> + }
I'm not sure r0 is always free to use here, it's not obvious. You can't
use the START_USE etc. macros to check, those only work inside
rs6000_emit_prologue. I'll figure something out.
> +
> + /* There's three cases here.
> +
> + One is a single probe which is the most common and most efficiently
> + implemented as it does not have to have a copy of the original
> + stack pointer if there are no residuals.
> +
> + Second is unrolled allocation/probes which we use if there's just
> + a few of them. It needs to save the original stack pointer into a
> + temporary for use as a source register in the allocation/probe.
> +
> + Last is a loop. This is the most uncommon case and least efficient. */
The way more common case is no probes at all, but that is handled in the
caller; maybe move it to this function instead?
> + rtx_insn *insn;
> + rtx_insn *retval = NULL;
> + if (rounded_size == probe_interval)
> + {
> + if (Pmode == SImode)
> + insn = emit_insn (gen_movsi_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + GEN_INT (-probe_interval),
> + stack_pointer_rtx));
> + else
> + insn = emit_insn (gen_movdi_di_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + GEN_INT (-probe_interval),
> + stack_pointer_rtx));
> + wrap_frame_mem (insn, stack_pointer_rtx, probe_interval);
> +
> + /* If this was the only allocation, then we can return the allocating
> + insn. */
> + if (rounded_size == orig_size)
> + retval = insn;
> +
> + dump_stack_clash_frame_info (PROBE_INLINE, rounded_size != orig_size);
> + }
> + else if (rounded_size <= 8 * probe_interval)
> + {
> + /* The ABI requires using the store with update insns to allocate
> + space and store the outer frame into the stack.
> +
> + So we save the current stack pointer into a temporary, then
> + emit the store-with-update insns to store the saved stack pointer
> + into the right location in each new page. */
> + rtx probe_int = GEN_INT (-probe_interval);
> + for (int i = 0; i < rounded_size; i += probe_interval)
> + {
> + if (Pmode == SImode)
> + insn = emit_insn (gen_movsi_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + probe_int, orig_sp));
> + else
> + insn = emit_insn (gen_movdi_di_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + probe_int, orig_sp));
> + wrap_frame_mem (insn, stack_pointer_rtx, probe_interval);
> + }
> + retval = NULL;
> + dump_stack_clash_frame_info (PROBE_INLINE, rounded_size != orig_size);
> + }
It seems the only thing preventing the probes from being elided is that
GCC does not realise the probe stores are not actually needed? I guess
that should be enough, but maybe there should be a test for this.
> + else
> + {
> + /* Compute the ending address. */
> + rtx end_addr
> + = copy_reg ? gen_rtx_REG (Pmode, 0) : gen_rtx_REG (Pmode, 12);
> + rtx rs = GEN_INT (-rounded_size);
> + rtx insn;
> + if (add_operand (rs, Pmode))
> + insn = emit_insn (gen_add3_insn (end_addr, stack_pointer_rtx, rs));
> + else
> + {
> + emit_move_insn (end_addr, GEN_INT (-rounded_size));
> + insn = emit_insn (gen_add3_insn (end_addr, end_addr,
> + stack_pointer_rtx));
> + add_reg_note (insn, REG_FRAME_RELATED_EXPR,
> + gen_rtx_SET (end_addr,
> + gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> + rs)));
> + }
> + RTX_FRAME_RELATED_P (insn) = 1;
What are these notes for? Not totally obvious... could use a comment :-)
> +
> + /* Emit the loop. */
> + if (TARGET_64BIT)
> + insn = emit_insn (gen_probe_stack_rangedi (stack_pointer_rtx,
> + stack_pointer_rtx, orig_sp,
> + end_addr));
> + else
> + insn = emit_insn (gen_probe_stack_rangesi (stack_pointer_rtx,
> + stack_pointer_rtx, orig_sp,
> + end_addr));
> + RTX_FRAME_RELATED_P (insn) = 1;
> + add_reg_note (insn, REG_FRAME_RELATED_EXPR,
> + gen_rtx_SET (stack_pointer_rtx, end_addr));
> + retval = NULL;
> + dump_stack_clash_frame_info (PROBE_LOOP, rounded_size != orig_size);
> + }
> +
> + if (orig_size != rounded_size)
> + {
> + insn = handle_residual (orig_size, rounded_size, orig_sp);
> +
> + /* If the residual was the only allocation, then we can return the
> + allocating insn. */
> + if (rounded_size == 0)
> + retval = insn;
> + }
> +
> + /* If we asked for a copy with an offset, then we still need add in tnhe
Typo ("tnhe").
> + offset. */
> + if (copy_reg && copy_off)
> + emit_insn (gen_add3_insn (copy_reg, copy_reg, GEN_INT (copy_off)));
> + return retval;
> +}
> +/* Probe a range of stack addresses from REG1 to REG3 inclusive. These are
> + absolute addresses. REG2 contains the outer stack pointer that must be
> + stored into *sp at each allocation.
s/outer stack pointer/pointer to the previous frame/ ? Or just say
"backchain"?
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index f78dbf9..9262c82 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -10262,7 +10262,7 @@
>
> (define_expand "allocate_stack"
> [(set (match_operand 0 "gpc_reg_operand" "")
> - (minus (reg 1) (match_operand 1 "reg_or_short_operand" "")))
> + (minus (reg 1) (match_operand 1 "reg_or_cint_operand" "")))
> (set (reg 1)
> (minus (reg 1) (match_dup 1)))]
> ""
> @@ -10272,6 +10272,15 @@
> rtx neg_op0;
> rtx insn, par, set, mem;
>
> + /* By allowing reg_or_cint_operand as the predicate we can get
> + better code for stack-clash-protection because we do not lose
> + size information. But the rest of the code expects the operand
> + to be reg_or_short_operand. If it isn't, then force it into
> + a register. */
> + rtx orig_op1 = operands[1];
> + if (!reg_or_short_operand (operands[1], Pmode))
> + operands[1] = force_reg (Pmode, operands[1]);
I don't understand the "we do not lose size information" bit. Maybe
you can show an example?
> + /* Allocate and probe if requested.
> + This may look similar to the loop we use for prologue allocations,
> + but it is critically different. For the former we know the loop
> + will iterate, but do not know that generally here. The former
> + uses that knowledge to rotate the loop. Combining them would be
> + possible with some performance cost. */
> + if (flag_stack_clash_protection)
> + {
> + rtx rounded_size, last_addr, residual;
> + HOST_WIDE_INT probe_interval;
> + compute_stack_clash_protection_loop_data (&rounded_size, &last_addr,
> + &residual, &probe_interval,
> + orig_op1);
> +
> + /* We do occasionally get in here with constant sizes, we might
> + as well do a reasonable job when we obviously can. */
> + if (rounded_size != CONST0_RTX (Pmode))
That's just const0_rtx.
> + {
> + rtx loop_lab, end_loop;
> + bool rotated = GET_CODE (rounded_size) == CONST_INT;
> +
> + emit_stack_clash_protection_probe_loop_start (&loop_lab, &end_loop,
> + last_addr, rotated);
> +
> + if (Pmode == SImode)
> + emit_insn (gen_movsi_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + GEN_INT (-probe_interval),
> + chain));
> + else
> + emit_insn (gen_movdi_di_update_stack (stack_pointer_rtx,
> + stack_pointer_rtx,
> + GEN_INT (-probe_interval),
> + chain));
> + emit_stack_clash_protection_probe_loop_end (loop_lab, end_loop,
> + last_addr, rotated);
> + }
> +
> + /* Now handle residuals. We just have to set operands[1] correctly
> + and let the rest of the expander run. */
> + if (REG_P (residual) || GET_CODE (residual) == CONST_INT)
CONST_INT_P
> + operands[1] = residual;
> + else
> + {
> + operands[1] = gen_reg_rtx (Pmode);
> + force_operand (residual, operands[1]);
> + }
> + }
Maybe this (from the comment on) could just be
operands[1] = residual;
if (!CONST_INT_P (operands[1]))
operands[1] = force_reg (Pmode, operands[1]);
?
Segher
