Re: [MIPS] Implement static stack checking

[Richard Sandiford <rdsandiford at googlemail dot com>]

Eric Botcazou <ebotcazou@adacore.com> writes:
> This implements static stack checking for MIPS, i.e. checking of the static 
> part of the frame in the prologue when -fstack-check is specified.  This is 
> very similar to the PowerPC and SPARC implementations and makes it possible to 
> pass the full ACATS testsuite with -fstack-check.
>
> Tested on mips64el-linux-gnu (n32/32/64), OK for the mainline?

The Ada bits I'll leave to you. :-)  The config/mips stuff looks good,
but a couple of nits:

> +(define_insn "probe_stack_range<P:mode>"
> +  [(set (match_operand:P 0 "register_operand" "=r")
> +	(unspec_volatile:P [(match_operand:P 1 "register_operand" "0")
> +			    (match_operand:P 2 "register_operand" "r")]
> +			    UNSPEC_PROBE_STACK_RANGE))]
> +  ""
> +  "* return mips_output_probe_stack_range (operands[0], operands[2]);"
> +  [(set_attr "type" "unknown")
> +   (set_attr "can_delay" "no")
> +   (set_attr "mode" "<MODE>")])

Please use "d" rather than "r" in these constraints.  Please use:

  { return mips_output_probe_stack_range (operands[0], operands[2]); }

for the output line.

> +/* Emit code to probe a range of stack addresses from FIRST to FIRST+SIZE,
> +   inclusive.  These are offsets from the current stack pointer.  */
> +
> +static void
> +mips_emit_probe_stack_range (HOST_WIDE_INT first, HOST_WIDE_INT size)
> +{

This function doesn't work with MIPS16 mode.  Maybe just:

  if (TARGET_MIPS16)
    sorry ("MIPS16 stack probes");

(We can't test TARGET_MIPS16 in something like STACK_CHECK_STATIC_BUILTIN
because MIPS16ness is a per-function property.)

> +  /* See if we have a constant small number of probes to generate.  If so,
> +     that's the easy case.  */
> +  if (first + size <= 32768)
> +    {
> +      HOST_WIDE_INT i;
> +
> +      /* Probe at FIRST + N * PROBE_INTERVAL for values of N from 1 until
> +	 it exceeds SIZE.  If only one probe is needed, this will not
> +	 generate any code.  Then probe at FIRST + SIZE.  */
> +      for (i = PROBE_INTERVAL; i < size; i += PROBE_INTERVAL)
> +        emit_stack_probe (plus_constant (Pmode, stack_pointer_rtx,
> +					 -(first + i)));
> +
> +      emit_stack_probe (plus_constant (Pmode, stack_pointer_rtx,
> +				       -(first + size)));
> +    }
> +
> +  /* Otherwise, do the same as above, but in a loop.  Note that we must be
> +     extra careful with variables wrapping around because we might be at
> +     the very top (or the very bottom) of the address space and we have
> +     to be able to handle this case properly; in particular, we use an
> +     equality test for the loop condition.  */
> +  else
> +    {
> +      HOST_WIDE_INT rounded_size;
> +      rtx r3 = gen_rtx_REG (Pmode, GP_REG_FIRST + 3);
> +      rtx r12 = gen_rtx_REG (Pmode, GP_REG_FIRST + 12);

Please use MIPS_PROLOGUE_TEMP for r3 (and probably rename r3).
I suppose GP_REG_FIRST + 12 should be MIPS_PROLOGUE_TEMP2, probably as:

#define MIPS_PROLOGUE_TEMP2_REGNUM \
  (TARGET_MIPS16 ? gcc_unreachable () \
   cfun->machine->interrupt_handler_p ? K1_REG_NUM : GP_REG_FIRST + 12)

#define MIPS_PROLOGUE_TEMP2(MODE) \
  gen_rtx_REG (MODE, MIPS_PROLOGUE_TEMP2_REGNUM)

and update the block comment above the MIPS_PROLOGUE_TEMP_REGNUM definition.

> +      /* Sanity check for the addressing mode we're going to use.  */
> +      gcc_assert (first <= 32768);
> +
> +
> +      /* Step 1: round SIZE to the previous multiple of the interval.  */
> +
> +      rounded_size = size & -PROBE_INTERVAL;
> +
> +
> +      /* Step 2: compute initial and final value of the loop counter.  */
> +
> +      /* TEST_ADDR = SP + FIRST.  */
> +      emit_insn (gen_rtx_SET (VOIDmode, r3,
> +			      plus_constant (Pmode, stack_pointer_rtx,
> +					     -first)));
> +
> +      /* LAST_ADDR = SP + FIRST + ROUNDED_SIZE.  */
> +      if (rounded_size > 32768)
> +	{
> +          emit_move_insn (r12, GEN_INT (rounded_size));
> +	  emit_insn (gen_rtx_SET (VOIDmode, r12,
> +			          gen_rtx_MINUS (Pmode, r3, r12)));
> +	}
> +      else
> +	emit_insn (gen_rtx_SET (VOIDmode, r12,
> +			        plus_constant (Pmode, r3, -rounded_size)));
> +
> +
> +      /* Step 3: the loop
> +
> +	while (TEST_ADDR != LAST_ADDR)
> +	  {
> +	    TEST_ADDR = TEST_ADDR + PROBE_INTERVAL
> +	    probe at TEST_ADDR
> +	  }
> +
> +	probes at FIRST + N * PROBE_INTERVAL for values of N from 1
> +	until it is equal to ROUNDED_SIZE.  */
> +
> +      if (TARGET_64BIT && TARGET_LONG64)
> +	emit_insn (gen_probe_stack_rangedi (r3, r3, r12));
> +      else
> +	emit_insn (gen_probe_stack_rangesi (r3, r3, r12));
> +
> +
> +      /* Step 4: probe at FIRST + SIZE if we cannot assert at compile-time
> +	 that SIZE is equal to ROUNDED_SIZE.  */
> +
> +      if (size != rounded_size)
> +	emit_stack_probe (plus_constant (Pmode, r12, rounded_size - size));

I Might Be Wrong, but it looks like this won't probe at FIRST + SIZE
in the case where SIZE == ROUNDED_SIZE, because the loop exits on that
value without probing it.  Should the last line be unconditional,
or does the loop need to be a do-while instead?  (I suppose the latter,
so that there isn't a hole bigger than PROBE_INTERVAL in the
SIZE != ROUNDED_SIZE case?)

> +/* Probe a range of stack addresses from REG1 to REG2 inclusive.  These are
> +   absolute addresses.  */

And as above, it seems to be exclusive of REG2 as things stand.

> +
> +const char *
> +mips_output_probe_stack_range (rtx reg1, rtx reg2)
> +{
> +  static int labelno = 0;
> +  char loop_lab[32], end_lab[32], tmp[64];
> +  rtx xops[2];
> +
> +  ASM_GENERATE_INTERNAL_LABEL (loop_lab, "LPSRL", labelno);
> +  ASM_GENERATE_INTERNAL_LABEL (end_lab, "LPSRE", labelno++);
> +
> +  ASM_OUTPUT_INTERNAL_LABEL (asm_out_file, loop_lab);
> +
> +  /* Jump to END_LAB if TEST_ADDR == LAST_ADDR.  */
> +  xops[0] = reg1;
> +  xops[1] = reg2;
> +  strcpy (tmp, "beq\t%0,%1,");
> +  output_asm_insn (strcat (tmp, &end_lab[1]), xops);
> +
> +  /* TEST_ADDR = TEST_ADDR + PROBE_INTERVAL.  */
> +  xops[1] = GEN_INT (-PROBE_INTERVAL);
> +  if (TARGET_64BIT && TARGET_LONG64)
> +    output_asm_insn (" daddiu\t%0,%0,%1", xops);
> +  else
> +    output_asm_insn (" addiu\t%0,%0,%1", xops);

This only works in noreorder mode.  If there's an asm in the function,
or something else that forces reorder mode (e.g. a -mfix-* option),
the addition won't be put in the delay slot.

"%(%<beq\t%0,%1," and "daddiu\t%0,%0,%1%>%)" should work.  (Note that
our MIPS asm output doesn't have a space before the delay slot; there's
a blank line after it instead.  That's all handled by output_asm_insn though.)

> +  /* Probe at TEST_ADDR and branch.  */
> +  fprintf (asm_out_file, "\tb\t");
> +  assemble_name_raw (asm_out_file, loop_lab);
> +  fputc ('\n', asm_out_file);
> +  if (TARGET_64BIT)
> +    output_asm_insn (" sd\t$0,0(%0)", xops);
> +  else
> +    output_asm_insn (" sw\t$0,0(%0)", xops);

Same here, so I think the branch should use output_asm_insn too.

OK with those changes, thanks.

Richard