Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
On 2/2/19 3:22 AM, Jakub Jelinek wrote:
> On Fri, Feb 01, 2019 at 11:52:04PM +0100, Eric Botcazou wrote:
>>> So, can we e.g. keep emitting the epilogue where it is now for
>>> naked_return_label != NULL_RTX and move it otherwise?
>>> For __builtin_return the setter and use of the hard register won't be
>>> adjacent in any case.
>>
>> See my comment in the audit trail of the PR; I'd suspend it and go to bed.
>> ;-)
>
> While the set of -fno- and -f options in some PRs are unlikely to be used by
> people in the wild, often those PRs uncover latent bugs that could cause
> serious wrong-code or ICEs, of course not always. So IMHO we shouldn't
> ignore those PRs, especially if they are regressions.
>
> In the meantime, I've bootstrapped/regtested successfully following version
> of the patch that should fix the builtin return case.
> In addition to normal {x86_64,i686}-linux bootstrap/regtest I've done a
> distro build where we 1) build the compiler itself with
> -fstack-protector-strong 2) run testsuite with
> --tool-test=\{,-fstack-protector-strong\}, so far on
> {x86_64,i686,powerpc64le}-linux, other targets still pending.
>
> The only "regression" was gcc.target/i386/call-1.c with
> -fstack-protector-strong, but that is because the test is invalid:
> void set_eax(int val)
> {
> __asm__ __volatile__ ("mov %0, %%eax" : : "m" (val));
> }
> - missing "eax" clobber or "=a" (dummy) output and when set_eax is inlined
> that can break optimizations badly. Of course, in addition to fixing that,
> I'd expect if the tests wants to test what it originally wanted to test, it
> needs to disable inlining or perhaps all IPA opts, not sure if just for
> set_eax or also for foo/bar. Perhaps we can keep the testcase as is with
> the "eax" clobber and add another one with __attribute__((noipa)) on
> set_eax/foo/bar.
>
> 2019-02-01 Jakub Jelinek
>
> PR rtl-optimization/87485
> * function.c (expand_function_end): Move stack_protect_epilogue
> before loading of return value into hard register(s).
>
> * gcc.dg/pr87485.c: New test.
OK. Though you do need to twiddle the call-1.c test in some manner.
I've got no strong opinions on setting noipa on everything in the test
vs twiddling the clobbers list. Your call on best approach for
adjusting the call-1 test.
jeff
Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
On Fri, Feb 01, 2019 at 11:52:04PM +0100, Eric Botcazou wrote:
> > So, can we e.g. keep emitting the epilogue where it is now for
> > naked_return_label != NULL_RTX and move it otherwise?
> > For __builtin_return the setter and use of the hard register won't be
> > adjacent in any case.
>
> See my comment in the audit trail of the PR; I'd suspend it and go to bed. ;-)
While the set of -fno- and -f options in some PRs are unlikely to be used by
people in the wild, often those PRs uncover latent bugs that could cause
serious wrong-code or ICEs, of course not always. So IMHO we shouldn't
ignore those PRs, especially if they are regressions.
In the meantime, I've bootstrapped/regtested successfully following version
of the patch that should fix the builtin return case.
In addition to normal {x86_64,i686}-linux bootstrap/regtest I've done a
distro build where we 1) build the compiler itself with
-fstack-protector-strong 2) run testsuite with
--tool-test=\{,-fstack-protector-strong\}, so far on
{x86_64,i686,powerpc64le}-linux, other targets still pending.
The only "regression" was gcc.target/i386/call-1.c with
-fstack-protector-strong, but that is because the test is invalid:
void set_eax(int val)
{
__asm__ __volatile__ ("mov %0, %%eax" : : "m" (val));
}
- missing "eax" clobber or "=a" (dummy) output and when set_eax is inlined
that can break optimizations badly. Of course, in addition to fixing that,
I'd expect if the tests wants to test what it originally wanted to test, it
needs to disable inlining or perhaps all IPA opts, not sure if just for
set_eax or also for foo/bar. Perhaps we can keep the testcase as is with
the "eax" clobber and add another one with __attribute__((noipa)) on
set_eax/foo/bar.
2019-02-01 Jakub Jelinek
PR rtl-optimization/87485
* function.c (expand_function_end): Move stack_protect_epilogue
before loading of return value into hard register(s).
* gcc.dg/pr87485.c: New test.
--- gcc/function.c.jj 2019-01-29 16:47:02.0 +0100
+++ gcc/function.c 2019-02-01 16:23:07.471877843 +0100
@@ -5330,6 +5330,12 @@ expand_function_end (void)
communicate between __builtin_eh_return and the epilogue. */
expand_eh_return ();
+ /* If stack protection is enabled for this function, check the guard. */
+ if (crtl->stack_protect_guard
+ && targetm.stack_protect_runtime_enabled_p ()
+ && naked_return_label == NULL_RTX)
+stack_protect_epilogue ();
+
/* If scalar return value was computed in a pseudo-reg, or was a named
return value that got dumped to the stack, copy that to the hard
return register. */
@@ -5476,7 +5482,9 @@ expand_function_end (void)
emit_insn (gen_blockage ());
/* If stack protection is enabled for this function, check the guard. */
- if (crtl->stack_protect_guard && targetm.stack_protect_runtime_enabled_p ())
+ if (crtl->stack_protect_guard
+ && targetm.stack_protect_runtime_enabled_p ()
+ && naked_return_label)
stack_protect_epilogue ();
/* If we had calls to alloca, and this machine needs
--- gcc/testsuite/gcc.dg/pr87485.c.jj 2019-02-01 16:30:51.101211900 +0100
+++ gcc/testsuite/gcc.dg/pr87485.c 2019-02-01 16:31:48.660260183 +0100
@@ -0,0 +1,29 @@
+/* PR rtl-optimization/87485 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fschedule-insns -fno-guess-branch-probability
-fno-isolate-erroneous-paths-dereference -fno-omit-frame-pointer
-fno-split-wide-types -fno-tree-ccp -fno-tree-sra" } */
+/* { dg-additional-options "-fstack-protector-strong" { target
fstack_protector } } */
+
+int *a;
+
+int
+foo (__int128 x, int y, int z)
+{
+ __int128 b;
+ *a = ((!!y ? y : x) * y | x) * 2;
+ if (z == 0)
+{
+ unsigned int c = 1;
+ __int128 *d =
+ for (*a = 0; *a < 1; *a += y)
+ ;
+ *a += b < (c / 0); /* { dg-warning "division by zero" } */
+ goto l;
+ m:
+ while (b < 1)
+ ;
+ ++*a;
+}
+ goto m;
+ l:
+ return 0;
+}
Jakub
Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
> So, can we e.g. keep emitting the epilogue where it is now for > naked_return_label != NULL_RTX and move it otherwise? > For __builtin_return the setter and use of the hard register won't be > adjacent in any case. See my comment in the audit trail of the PR; I'd suspend it and go to bed. ;-) -- Eric Botcazou
Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
On Fri, Feb 01, 2019 at 11:37:06PM +0100, Eric Botcazou wrote: > > As discussed in the PR and suggested by Uros, scheduler has code to keep a > > use of hard register next to the assignment that sets that hard register > > from a pseudo, which is desirable so that RA can deal with it properly. > > Unfortunately, with -fstack-protector* we stick the stack protect epilogue > > in between, which splits the load and use to different basic blocks. > > The code emitted by expand_function_end between these two spots is only the > > loading of the return value into registers, so generally it shouldn't > > contain any stores which stack protection wants to guard against, so I > > believe from security POV this shouldn't weaken anything, but fixes the > > testcase. > > This moves the stack protect epilogue from after the naked_return_label to > before though, so it will be skipped for a naked return. So, can we e.g. keep emitting the epilogue where it is now for naked_return_label != NULL_RTX and move it otherwise? For __builtin_return the setter and use of the hard register won't be adjacent in any case. Jakub
Re: [PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
> As discussed in the PR and suggested by Uros, scheduler has code to keep a > use of hard register next to the assignment that sets that hard register > from a pseudo, which is desirable so that RA can deal with it properly. > Unfortunately, with -fstack-protector* we stick the stack protect epilogue > in between, which splits the load and use to different basic blocks. > The code emitted by expand_function_end between these two spots is only the > loading of the return value into registers, so generally it shouldn't > contain any stores which stack protection wants to guard against, so I > believe from security POV this shouldn't weaken anything, but fixes the > testcase. This moves the stack protect epilogue from after the naked_return_label to before though, so it will be skipped for a naked return. -- Eric Botcazou
[PATCH] Move stack protector epilogue before loading return hard reg(s) from pseudo(s) (PR rtl-optimization/87485)
Hi!
As discussed in the PR and suggested by Uros, scheduler has code to keep a
use of hard register next to the assignment that sets that hard register
from a pseudo, which is desirable so that RA can deal with it properly.
Unfortunately, with -fstack-protector* we stick the stack protect epilogue
in between, which splits the load and use to different basic blocks.
The code emitted by expand_function_end between these two spots is only the
loading of the return value into registers, so generally it shouldn't
contain any stores which stack protection wants to guard against, so I
believe from security POV this shouldn't weaken anything, but fixes the
testcase.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2019-02-01 Jakub Jelinek
PR rtl-optimization/87485
* function.c (expand_function_end): Move stack_protect_epilogue
before loading of return value into hard register(s).
* gcc.dg/pr87485.c: New test.
--- gcc/function.c.jj 2019-01-29 16:47:02.0 +0100
+++ gcc/function.c 2019-02-01 16:23:07.471877843 +0100
@@ -5330,6 +5330,10 @@ expand_function_end (void)
communicate between __builtin_eh_return and the epilogue. */
expand_eh_return ();
+ /* If stack protection is enabled for this function, check the guard. */
+ if (crtl->stack_protect_guard && targetm.stack_protect_runtime_enabled_p ())
+stack_protect_epilogue ();
+
/* If scalar return value was computed in a pseudo-reg, or was a named
return value that got dumped to the stack, copy that to the hard
return register. */
@@ -5475,10 +5479,6 @@ expand_function_end (void)
&& targetm_common.except_unwind_info (_options) != UI_SJLJ)
emit_insn (gen_blockage ());
- /* If stack protection is enabled for this function, check the guard. */
- if (crtl->stack_protect_guard && targetm.stack_protect_runtime_enabled_p ())
-stack_protect_epilogue ();
-
/* If we had calls to alloca, and this machine needs
an accurate stack pointer to exit the function,
insert some code to save and restore the stack pointer. */
--- gcc/testsuite/gcc.dg/pr87485.c.jj 2019-02-01 16:30:51.101211900 +0100
+++ gcc/testsuite/gcc.dg/pr87485.c 2019-02-01 16:31:48.660260183 +0100
@@ -0,0 +1,29 @@
+/* PR rtl-optimization/87485 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fschedule-insns -fno-guess-branch-probability
-fno-isolate-erroneous-paths-dereference -fno-omit-frame-pointer
-fno-split-wide-types -fno-tree-ccp -fno-tree-sra" } */
+/* { dg-additional-options "-fstack-protector-strong" { target
fstack_protector } } */
+
+int *a;
+
+int
+foo (__int128 x, int y, int z)
+{
+ __int128 b;
+ *a = ((!!y ? y : x) * y | x) * 2;
+ if (z == 0)
+{
+ unsigned int c = 1;
+ __int128 *d =
+ for (*a = 0; *a < 1; *a += y)
+ ;
+ *a += b < (c / 0); /* { dg-warning "division by zero" } */
+ goto l;
+ m:
+ while (b < 1)
+ ;
+ ++*a;
+}
+ goto m;
+ l:
+ return 0;
+}
Jakub
