Re: [PATCH][GCC][AArch64] Updated stack-clash implementation supporting 64k probes. [patch (1/7)]

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Fri, Sep 07, 2018 at 11:03:28AM -0500, Tamar Christina wrote:
> Hi Richard,
> 
> The 08/28/2018 21:58, Richard Sandiford wrote:
> > Tamar Christina <Tamar.Christina@arm.com> writes:
> > > +  HOST_WIDE_INT guard_used_by_caller = STACK_CLASH_CALLER_GUARD;
> > > +  /* When doing the final adjustment for the outgoing argument size we can't
> > > +     assume that LR was saved at position 0.  So subtract it's offset from the
> > > +     ABI safe buffer so that we don't accidentally allow an adjustment that
> > > +     would result in an allocation larger than the ABI buffer without
> > > +     probing.  */
> > > +  HOST_WIDE_INT min_probe_threshold
> > > +    = final_adjustment_p
> > > +      ? guard_used_by_caller - cfun->machine->frame.reg_offset[LR_REGNUM]
> > > +      : guard_size - guard_used_by_caller;
> > [...]
> > > +  if (residual)
> > > +    {
> > > +      aarch64_sub_sp (temp1, temp2, residual, frame_related_p);
> > > +      if (residual >= min_probe_threshold)
> > > +	{
> > > +	  if (dump_file)
> > > +	    fprintf (dump_file,
> > > +		     "Stack clash AArch64 prologue residuals: "
> > > +		     HOST_WIDE_INT_PRINT_DEC " bytes, probing will be required."
> > > +		     "\n", residual);
> > > +	  emit_stack_probe (plus_constant (Pmode, stack_pointer_rtx,
> > > +					   STACK_CLASH_CALLER_GUARD));
> > 
> > reg_offsets are nonnegative, so if LR_REGNUM isn't saved at position 0,
> > min_probe_threshold will be less than STACK_CLASH_CALLER_GUARD.  It looks
> > like the probe would then write above the region.
> > 
> > Using >= rather than > means that the same thing could happen when
> > LR_REGNUM is at position 0, if the residual is exactly
> > STACK_CLASH_CALLER_GUARD.
> 
> That's true. While addressing this we changed how the residuals are probed.
> 
> To address a comment you raised offline about the saving of LR when calling
> a no-return function using a tail call and -fomit-frame-pointer, I think this should
> be safe as the code in frame_layout (line 4131-4136) would ensure that R30 is saved.
> 
> I have added two new tests to check for this, so that if it does change in the future they
> would fail. 
> 
> Attached is the updated patch and new changelog
> 
> Ok for trunk?

I'm happy with this patch version; I'd have preferred a FORNOW comment on this:

> +  /* If SIZE is not large enough to require probing, just adjust the stack and
> +     exit.  */
> +  if (!poly_size.is_constant (&size)
> +      || known_lt (poly_size, min_probe_threshold)
> +      || !flag_stack_clash_protection)

as you don't fix it until 2/7, but that is a minor point.

I'm happy with you responding to Richard S' request for an assert either in
this patch, or tacked on as an 8/7.

OK.

Thanks,
James

> Thanks,
> Tamar
> 
> gcc/
> 2018-09-07  Jeff Law  <law@redhat.com>
> 	    Richard Sandiford <richard.sandiford@linaro.org>
> 	    Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/86486
> 	* config/aarch64/aarch64.md
> 	(probe_stack_range): Add k (SP) constraint.
> 	* config/aarch64/aarch64.h (STACK_CLASH_CALLER_GUARD,
> 	STACK_CLASH_MAX_UNROLL_PAGES): New.
> 	* config/aarch64/aarch64.c (aarch64_output_probe_stack_range): Emit
> 	stack probes for stack clash.
> 	(aarch64_allocate_and_probe_stack_space): New.
> 	(aarch64_expand_prologue): Use it.
> 	(aarch64_expand_epilogue): Likewise and update IP regs re-use criteria.
> 	(aarch64_sub_sp): Add emit_move_imm optional param.
> 
> gcc/testsuite/
> 2018-09-07  Jeff Law  <law@redhat.com>
> 	    Richard Sandiford <richard.sandiford@linaro.org>
> 	    Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/86486
> 	* gcc.target/aarch64/stack-check-12.c: New.
> 	* gcc.target/aarch64/stack-check-13.c: New.
> 	* gcc.target/aarch64/stack-check-cfa-1.c: New.
> 	* gcc.target/aarch64/stack-check-cfa-2.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-1.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-10.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-11.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-12.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-13.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-14.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-15.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-2.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-3.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-4.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-5.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-6.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-7.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-8.c: New.
> 	* gcc.target/aarch64/stack-check-prologue-9.c: New.
> 	* gcc.target/aarch64/stack-check-prologue.h: New.
> 	* lib/target-supports.exp
> 	(check_effective_target_supports_stack_clash_protection): Add AArch64.
>