Hi Richard, > -----Original Message----- > From: Richard Biener <rguent...@suse.de> > Sent: Tuesday, October 9, 2018 08:28 > To: Tamar Christina <tamar.christ...@arm.com> > Cc: Jeff Law <l...@redhat.com>; gcc-patches@gcc.gnu.org; nd > <n...@arm.com>; i...@airs.com > Subject: RE: [PATCH][GCC][mid-end] Add a hook to support telling the mid- > end when to probe the stack [patch (2/6)] > > On Tue, 9 Oct 2018, Tamar Christina wrote: > > > Hi All, > > > > I'm looking for permission to backport this patch to the GCC-8 branch > > to fix PR86486. > > > > OK for backport? > > This doesn't seem to fix a regression and it's been on trunk only for a few > days.
I probably should have asked for the backport on patch 0/8 instead of individually. But the reason for asking for the backport is that on GCC-8 the current implementation is insufficient for AArch64. The only protection a user would get from turning on stack-clash protection would be from the mid-end alloca code. static allocations and SVE code would get zero protection. The changes, including the mid-end changes don't affect any other target. The mid-end change is gated by a target hook that only aarch64 defines. I'm happy to let it sit on trunk a bit longer for more testing if that's what's required or to test against more targets. The current patch set was tested trunk on x86_64 and AArch64 by comparing a) builds with stack-clash off (with patches vs w/o patches) and b) builds with stack-clash on (with patches vs w/o patches). For AArch64 the glibc testsuite was also ran (with patches vs w/o patches) with stack-clash on. The patches apply almost cleanly on GCC-8, modulo some small conflicts like line numbers in configure file and removal of some minor features not available in GCC-8. So far the GCC-8 version I have only done a bootstrap and regression with stack-clash on by default on AArch64. I am planning more testing on AArch64 and x86_64 but wanted to check on the backport before doing the exhaustive testing. I believe a backport of only the target specific bits may also be sufficient to give protection, but have not worked through this scenario yet. It's not the preferred option but one that I'd be happy to investigate if it's the only one available. Regards, Tamar > > Richard. > > > Thanks, > > Tamar > > > > > -----Original Message----- > > > From: Jeff Law <l...@redhat.com> > > > Sent: Wednesday, July 11, 2018 19:53 > > > To: Tamar Christina <tamar.christ...@arm.com>; > > > gcc-patches@gcc.gnu.org > > > Cc: nd <n...@arm.com>; rguent...@suse.de; i...@airs.com > > > Subject: Re: [PATCH][GCC][mid-end] Add a hook to support telling the > > > mid- end when to probe the stack [patch (2/6)] > > > > > > On 07/11/2018 05:21 AM, Tamar Christina wrote: > > > > Hi All, > > > > > > > > This patch adds a hook to tell the mid-end about the probing > > > > requirements of the target. On AArch64 we allow a specific range > > > > for which no probing needs to be done. This same range is also > > > > the amount that will have to be probed up when a probe is needed > > > > after dropping the > > > stack. > > > > > > > > Defining this probe comes with the extra requirement that the > > > > outgoing arguments size of any function that uses alloca and stack > > > > clash be at the very least 8 bytes. With this invariant we can > > > > skip doing the zero checks for alloca and save some code. > > > > > > > > A simplified version of the AArch64 stack frame is: > > > > > > > > +-----------------------+ > > > > | | > > > > | | > > > > | | > > > > +-----------------------+ > > > > |LR | > > > > +-----------------------+ > > > > |FP | > > > > +-----------------------+ > > > > |dynamic allocations | -\ probe range hook effects these > > > > +-----------------------+ --\ and ensures that outgoing stack > > > > |padding | -- args is always > 8 when alloca. > > > > +-----------------------+ ---/ Which means it's always safe to > > > > probe > > > > |outgoing stack args |-/ at SP > > > > +-----------------------+ > > > > > > > > > > > > This allows us to generate better code than without the hook > > > > without affecting other targets. > > > > > > > > With this patch I am also removing the > > > > stack_clash_protection_final_dynamic_probe > > > > hook which was added specifically for AArch64 but that is no longer > needed. > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > x86_64-pc-linux-gnu > > > and no issues. > > > > Both targets were tested with stack clash on and off by default. > > > > > > > > Ok for trunk? > > > > > > > > Thanks, > > > > Tamar > > > > > > > > gcc/ > > > > 2018-07-11 Tamar Christina <tamar.christ...@arm.com> > > > > > > > > PR target/86486 > > > > * explow.c (anti_adjust_stack_and_probe_stack_clash): Support > > > custom > > > > probe ranges. > > > > * target.def (stack_clash_protection_alloca_probe_range): New. > > > > (stack_clash_protection_final_dynamic_probe): Remove. > > > > * targhooks.h > > > > (default_stack_clash_protection_alloca_probe_range) > > > New. > > > > (default_stack_clash_protection_final_dynamic_probe): Remove. > > > > * targhooks.c: Likewise. > > > > * doc/tm.texi.in > > > (TARGET_STACK_CLASH_PROTECTION_ALLOCA_PROBE_RANGE): New. > > > > (TARGET_STACK_CLASH_PROTECTION_FINAL_DYNAMIC_PROBE): > > > Remove. > > > > * doc/tm.texi: Regenerate. > > > > > > > The control flow is a bit convoluted here, but after a few false > > > starts where I thought this was wrong, I think it's OK. > > > > > > Jeff > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > Richard Biener <rguent...@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, > HRB 21284 (AG Nuernberg)
