Hi All, I'm looking for permission to backport this patch to the GCC-8 branch to fix PR86486.
OK for backport? Thanks, Tamar > -----Original Message----- > From: James Greenhalgh <james.greenha...@arm.com> > Sent: Tuesday, September 11, 2018 16:56 > To: Tamar Christina <tamar.christ...@arm.com> > Cc: Richard Sandiford <richard.sandif...@arm.com>; Jeff Law > <l...@redhat.com>; gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard > Earnshaw <richard.earns...@arm.com>; Marcus Shawcroft > <marcus.shawcr...@arm.com> > Subject: Re: [PATCH][GCC][AArch64] Updated stack-clash implementation > supporting 64k probes. [patch (1/7)] > > 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.christ...@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 <l...@redhat.com> > > Richard Sandiford <richard.sandif...@linaro.org> > > Tamar Christina <tamar.christ...@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 <l...@redhat.com> > > Richard Sandiford <richard.sandif...@linaro.org> > > Tamar Christina <tamar.christ...@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. > >