This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH][PR target/83994] Fix stack-clash-protection code generation on x86
- From: Jeff Law <suzanne dot jeff dot law at gmail dot com>
- To: Uros Bizjak <ubizjak at gmail dot com>
- Cc: gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 23 Jan 2018 16:15:18 -0700
- Subject: [PATCH][PR target/83994] Fix stack-clash-protection code generation on x86
- Authentication-results: sourceware.org; auth=none
pr83994 is a code generation bug in the stack-clash support that affects
openssl (we've turned on stack-clash-protection by default for the F28
builds).
The core problem is stack-clash (like stack-check) will emit a probing
loop if the prologue allocates enough stack space. When emitting a loop
both implementations will need a scratch register.
They use get_scratch_register_at_entry to find a suitable scratch
register. This routine assumes that callee registers saves are
completed at the point where the scratch register is going to be used.
In this particular testcase we select %ebx because ax,cx,dx are used for
parameter passing. That's fine. The problem is %ebx hasn't been saved yet!
-fstack-check has a bit of code in the frame setup/layout code which
forces the prologue to use pushes rather than reg->mem moves for saving
registers. There's a gcc_assert in the prologue expander to catch any
case where the registers aren't saved.
-fstack-clash-protection doesn't have that same bit of magic in the
frame setup/layout code and it bypasses the assertion due to a change I
made back in Nov 2017 due to not being aware of this particular issue.
This patch reverts the assertion bypass I added back in Nov 2017 and
adds clarifying comments. The patch also forces use of push to save
integer registers for a stack-clash protected prologue if probes are
going to be needed.
Bootstrapped and regression tested on x86_64.
While the bug is not marked as a regression, ISTM this needs to be fixed
for gcc-8.
OK for the trunk?
Jeff
* i386.c (get_probe_interval): Move to earlier point.
(ix86_compute_frame_layout): If -fstack-clash-protection and
the frame is larger than the probe interval, then use pushes
to save registers rather than reg->mem moves.
(ix86_expand_prologue): Remove conditional for int_registers_saved
assertion.
* gcc.target/i386/pr83994.c: New test.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 72d25ae..4cb55a8 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -11497,6 +11497,18 @@ static void warn_once_call_ms2sysv_xlogues (const char *feature)
}
}
+/* Return the probing interval for -fstack-clash-protection. */
+
+static HOST_WIDE_INT
+get_probe_interval (void)
+{
+ if (flag_stack_clash_protection)
+ return (HOST_WIDE_INT_1U
+ << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL));
+ else
+ return (HOST_WIDE_INT_1U << STACK_CHECK_PROBE_INTERVAL_EXP);
+}
+
/* When using -fsplit-stack, the allocation routines set a field in
the TCB to the bottom of the stack plus this much space, measured
in bytes. */
@@ -11773,7 +11785,14 @@ ix86_compute_frame_layout (void)
to_allocate = offset - frame->sse_reg_save_offset;
if ((!to_allocate && frame->nregs <= 1)
- || (TARGET_64BIT && to_allocate >= HOST_WIDE_INT_C (0x80000000)))
+ || (TARGET_64BIT && to_allocate >= HOST_WIDE_INT_C (0x80000000))
+ /* If stack clash probing needs a loop, then it needs a
+ scratch register. But the returned register is only guaranteed
+ to be safe to use after register saves are complete. So if
+ stack clash protections are enabled and the allocated frame is
+ larger than the probe interval, then use pushes to save
+ callee saved registers. */
+ || (flag_stack_clash_protection && to_allocate > get_probe_interval ()))
frame->save_regs_using_mov = false;
if (ix86_using_red_zone ()
@@ -12567,18 +12586,6 @@ release_scratch_register_on_entry (struct scratch_reg *sr)
}
}
-/* Return the probing interval for -fstack-clash-protection. */
-
-static HOST_WIDE_INT
-get_probe_interval (void)
-{
- if (flag_stack_clash_protection)
- return (HOST_WIDE_INT_1U
- << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL));
- else
- return (HOST_WIDE_INT_1U << STACK_CHECK_PROBE_INTERVAL_EXP);
-}
-
/* Emit code to adjust the stack pointer by SIZE bytes while probing it.
This differs from the next routine in that it tries hard to prevent
@@ -13727,12 +13734,11 @@ ix86_expand_prologue (void)
&& (flag_stack_check == STATIC_BUILTIN_STACK_CHECK
|| flag_stack_clash_protection))
{
- /* This assert wants to verify that integer registers were saved
- prior to probing. This is necessary when probing may be implemented
- as a function call (Windows). It is not necessary for stack clash
- protection probing. */
- if (!flag_stack_clash_protection)
- gcc_assert (int_registers_saved);
+ /* We expect the GP registers to be saved when probes are used
+ as the probing sequences might need a scratch register and
+ the routine to allocate one assumes the integer registers
+ have already been saved. */
+ gcc_assert (int_registers_saved);
if (flag_stack_clash_protection)
{
diff --git a/gcc/testsuite/gcc.target/i386/pr83994.c b/gcc/testsuite/gcc.target/i386/pr83994.c
new file mode 100644
index 0000000..b57b04b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr83994.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -m32 -march=i686 -fpic -fstack-clash-protection" } */
+
+void f1 (char *);
+
+__attribute__ ((regparm (3)))
+int
+f2 (int arg1, int arg2, int arg3)
+{
+ char buf[16384];
+ f1 (buf);
+ f1 (buf);
+ return 0;
+}
+