On Mon, Sep 14, 2020 at 8:12 AM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Mon, Sep 14, 2020 at 7:35 AM Jakub Jelinek <ja...@redhat.com> wrote:
> >
> > On Sat, Sep 12, 2020 at 10:11:36AM -0700, H.J. Lu via Gcc-patches wrote:
> > > Clobbering the stack pointer in asm statment has been deprecated. Adding
> > > the stack pointer register to the clobber list has traditionally had some
> > > undocumented and somewhat obscure side-effects, including ICE. Issue
> > > a warning and ignore the clobbered stack pointer in asm statment.
> > >
> > > gcc/
> > >
> > > PR target/97032
> > > * cfgexpand.c (asm_clobber_reg_kind): New enum.
> > > (asm_clobber_reg_is_valid): Renamed to ...
> > > (get_asm_clobber_reg_kind): This. Ignore the stack pointer.
> > > (expand_asm_stmt): Replace asm_clobber_reg_is_valid with
> > > get_asm_clobber_reg_kind. Skip ignored clobbered register.
> >
> > AFAIK in the PR52813 discussions it was mentioned that the sp clobbers
> > had some user visible effects like forcing no red-zone, or (at least for
> > some GCC versions) forcing frame pointer in the function containing the asm.
> > Such clobbers are deprecated since GCC 9, so I think e.g. this patch isn't
> > really backportable to 9 branch. Are we ready to stop that behavior?
> > And if yes, shouldn't we instead just error on this because a warning
> > clearly doesn't help, as users ignore warnings (at least in firefox this
> > isn't fixed yet)?
> > Is there some other way to fix the ICE (mark functions containing that and
> > perhaps penalize the code, in this case e.g. by perhaps forcing unnecessary
> > realignment, but not ICE)?
>
> For GCC 8/9, we can inform the backend that SP is clobbered by asm statement
> and each backend can mitigate its impact.
>
Something like this for GCC 8 and 9.
--
H.J.
From 72c62a2576808d715c1232cc6816483229d542e9 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.to...@gmail.com>
Date: Mon, 14 Sep 2020 08:52:27 -0700
Subject: [PATCH] [GCC 9] rtl_data: Add sp_is_clobbered_by_asm
Add sp_is_clobbered_by_asm to rtl_data to inform backends that the stack
pointer is clobbered by asm statement.
gcc/
PR target/97032
* cfgexpand.c (asm_clobber_reg_kind): Set sp_is_clobbered_by_asm
to true if the stack pointer is clobbered by asm statement.
* emit-rtl.h (rtl_data): Add sp_is_clobbered_by_asm.
* config/i386/i386.c (ix86_get_drap_rtx): Set need_drap to true
if the stack pointer is clobbered by asm statement.
gcc/testsuite/
PR target/97032
* gcc.target/i386/pr97032.c: New test.
---
gcc/cfgexpand.c | 14 +++++++++-----
gcc/config/i386/i386.c | 6 ++++--
gcc/emit-rtl.h | 3 +++
gcc/testsuite/gcc.target/i386/pr97032.c | 23 +++++++++++++++++++++++
4 files changed, 39 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr97032.c
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index e252975f546..1b0591193c3 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2879,11 +2879,15 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
as it was before, so no asm can validly clobber the stack pointer in
the usual sense. Adding the stack pointer to the clobber list has
traditionally had some undocumented and somewhat obscure side-effects. */
- if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)
- && warning (OPT_Wdeprecated, "listing the stack pointer register"
- " %qs in a clobber list is deprecated", regname))
- inform (input_location, "the value of the stack pointer after an %<asm%>"
- " statement must be the same as it was before the statement");
+ if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
+ {
+ crtl->sp_is_clobbered_by_asm = true;
+ if (warning (OPT_Wdeprecated, "listing the stack pointer register"
+ " %qs in a clobber list is deprecated", regname))
+ inform (input_location, "the value of the stack pointer after"
+ " an %<asm%> statement must be the same as it was before"
+ " the statement");
+ }
return is_valid;
}
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ae5046ee6a0..6a0b356803e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12283,10 +12283,12 @@ ix86_update_stack_boundary (void)
static rtx
ix86_get_drap_rtx (void)
{
- /* We must use DRAP if there are outgoing arguments on stack and
+ /* We must use DRAP if there are outgoing arguments on stack or
+ the stack pointer register is clobbered by asm statment and
ACCUMULATE_OUTGOING_ARGS is false. */
if (ix86_force_drap
- || (cfun->machine->outgoing_args_on_stack
+ || ((cfun->machine->outgoing_args_on_stack
+ || crtl->sp_is_clobbered_by_asm)
&& !ACCUMULATE_OUTGOING_ARGS))
crtl->need_drap = true;
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index 7b1cecd3c44..b8a07d21d37 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -266,6 +266,9 @@ struct GTY(()) rtl_data {
pass_stack_ptr_mod has run. */
bool sp_is_unchanging;
+ /* True if the stack pointer is clobbered by asm statement. */
+ bool sp_is_clobbered_by_asm;
+
/* Nonzero if function being compiled doesn't contain any calls
(ignoring the prologue and epilogue). This is set prior to
register allocation in IRA and is valid for the remaining
diff --git a/gcc/testsuite/gcc.target/i386/pr97032.c b/gcc/testsuite/gcc.target/i386/pr97032.c
new file mode 100644
index 00000000000..7cbbe9bc22a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr97032.c
@@ -0,0 +1,23 @@
+/* { dg-do compile { target { ia32 && fstack_protector } } } */
+/* { dg-options "-O2 -mincoming-stack-boundary=2 -fstack-protector-all" } */
+
+#include <stdarg.h>
+
+extern int *__errno_location (void);
+
+long
+sys_socketcall (int op, ...)
+{
+ long int res;
+ va_list ap;
+ va_start (ap, op);
+ asm volatile ("push %%ebx; movl %2, %%ebx; int $0x80; pop %%ebx"
+ /* { dg-warning "listing the stack pointer register" "" { target *-*-* } .-1 } */
+ : "=a" (res) : "0" (102), "ri" (16), "c" (ap) : "memory", "esp");
+ if (__builtin_expect (res > 4294963200UL, 0))
+ *__errno_location () = -res;
+ va_end (ap);
+ return res;
+}
+
+/* { dg-final { scan-assembler "call\[ \t\]*_?__errno_location" } } */
--
2.26.2
