[PATCH] Ignore the clobbered stack pointer in asm statment

[PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

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.

gcc/testsuite/

	PR target/97032
	* gcc.target/i386/pr97032.c: New test.
---
 gcc/cfgexpand.c                         | 45 ++++++++++++++++++-------
 gcc/testsuite/gcc.target/i386/pr97032.c | 23 +++++++++++++
 2 files changed, 55 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr97032.c

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index b334ea03c25..433e7889138 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2832,14 +2832,21 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_SET *clobbered_regs)
   return false;
 }
 
+enum asm_clobber_reg_kind
+{
+  asm_clobber_reg_valid,
+  asm_clobber_reg_invalid,
+  asm_clobber_reg_ignored
+};
+
 /* Check that the given REGNO spanning NREGS is a valid
    asm clobber operand.  Some HW registers cannot be
    saved/restored, hence they should not be clobbered by
    asm statements.  */
-static bool
-asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
+static asm_clobber_reg_kind
+get_asm_clobber_reg_kind (int regno, int nregs, const char *regname)
 {
-  bool is_valid = true;
+  asm_clobber_reg_kind kind = asm_clobber_reg_valid;
   HARD_REG_SET regset;
 
   CLEAR_HARD_REG_SET (regset);
@@ -2852,7 +2859,7 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
     {
       /* ??? Diagnose during gimplification?  */
       error ("PIC register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
+      kind = asm_clobber_reg_invalid;
     }
   else if (!in_hard_reg_set_p
 	   (accessible_reg_set, reg_raw_mode[regno], regno))
@@ -2860,7 +2867,7 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
       /* ??? Diagnose during gimplification?  */
       error ("the register %qs cannot be clobbered in %<asm%>"
 	     " for the current target", regname);
-      is_valid = false;
+      kind = asm_clobber_reg_invalid;
     }
 
   /* Clobbering the stack pointer register is deprecated.  GCC expects
@@ -2868,13 +2875,18 @@ 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))
+      {
+	kind = asm_clobber_reg_ignored;
+	if (warning (OPT_Wdeprecated, "listing the stack pointer register"
+		     " %qs in a clobber list is deprecated and ignored",
+		     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;
+  return kind;
 }
 
 /* Generate RTL for an asm statement with arguments.
@@ -3009,8 +3021,15 @@ expand_asm_stmt (gasm *stmt)
 	  else
 	    for (int reg = j; reg < j + nregs; reg++)
 	      {
-		if (!asm_clobber_reg_is_valid (reg, nregs, regname))
-		  return;
+		switch (get_asm_clobber_reg_kind (reg, nregs, regname))
+		  {
+		  case asm_clobber_reg_valid:
+		    break;
+		  case asm_clobber_reg_invalid:
+		    return;
+		  case asm_clobber_reg_ignored:
+		    continue;
+		  }
 
 	        SET_HARD_REG_BIT (clobbered_regs, reg);
 	        rtx x = gen_rtx_REG (reg_raw_mode[reg], reg);
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

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Florian Weimer]

* H. J. Lu via Gcc-patches:

> +	  inform (input_location, "the value of the stack pointer after"
> +		  " an %<asm%> statement must be the same as it was"
> +		  " before the statement");

Would it make sense to generate a stronger worded warning when
generating asynchronous unwind tables?  If an asm statement changes
the stackpointer even temporarily, the unwind information won't be
correct.

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

On Sat, Sep 12, 2020 at 10:37 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * H. J. Lu via Gcc-patches:
>
> > +       inform (input_location, "the value of the stack pointer after"
> > +               " an %<asm%> statement must be the same as it was"
> > +               " before the statement");
>
> Would it make sense to generate a stronger worded warning when
> generating asynchronous unwind tables?  If an asm statement changes
> the stackpointer even temporarily, the unwind information won't be
> correct.

Is this the right place for such warning?  asm statement may touch
stack pointer without clobbering it.

-- 
H.J.

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Jakub Jelinek]

On Sat, Sep 12, 2020 at 07:37:36PM +0200, Florian Weimer wrote:
> * H. J. Lu via Gcc-patches:
> 
> > +	  inform (input_location, "the value of the stack pointer after"
> > +		  " an %<asm%> statement must be the same as it was"
> > +		  " before the statement");
> 
> Would it make sense to generate a stronger worded warning when
> generating asynchronous unwind tables?  If an asm statement changes
> the stackpointer even temporarily, the unwind information won't be
> correct.

It can be, at least when using .cfi_* directives, the inline asm writers
have the possibility to support that (they can check a macro if compiler is
emitting .cfi_* directives and use .cfi_* directives in their snippets.

	Jakub

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Florian Weimer]

* Jakub Jelinek:

> On Sat, Sep 12, 2020 at 07:37:36PM +0200, Florian Weimer wrote:
>> * H. J. Lu via Gcc-patches:
>> 
>> > +	  inform (input_location, "the value of the stack pointer after"
>> > +		  " an %<asm%> statement must be the same as it was"
>> > +		  " before the statement");
>> 
>> Would it make sense to generate a stronger worded warning when
>> generating asynchronous unwind tables?  If an asm statement changes
>> the stackpointer even temporarily, the unwind information won't be
>> correct.
>
> It can be, at least when using .cfi_* directives, the inline asm writers
> have the possibility to support that (they can check a macro if compiler is
> emitting .cfi_* directives and use .cfi_* directives in their snippets.

Is it possible to write generic .cfi_* directives that apply both to
the frame-pointer non-frame-pointer cases?

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Jakub Jelinek]

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)?

	Jakub

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

On Mon, Sep 14, 2020 at 7:35 AM Jakub Jelinek <jakub@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.

-- 
H.J.

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

On Mon, Sep 14, 2020 at 8:12 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Sep 14, 2020 at 7:35 AM Jakub Jelinek <jakub@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.

[-- Attachment #2: 0001-GCC-9-rtl_data-Add-sp_is_clobbered_by_asm.patch --]
[-- Type: text/x-patch, Size: 4511 bytes --]

From 72c62a2576808d715c1232cc6816483229d542e9 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@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

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Jakub Jelinek]

On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
> Something like this for GCC 8 and 9.

Guess my preference would be to do this everywhere and then let's discuss if
we change the warning into error there or keep it being deprecated.

Though, let's see what others want to say about this.

> 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.

	Jakub

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

On Mon, Sep 14, 2020 at 10:05 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
> > Something like this for GCC 8 and 9.
>
> Guess my preference would be to do this everywhere and then let's discuss if

The same patch also works on master branch.

> we change the warning into error there or keep it being deprecated.
>
> Though, let's see what others want to say about this.
>
> > 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.
>
>         Jakub
>


-- 
H.J.

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Richard Sandiford]

Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
>> Something like this for GCC 8 and 9.
>
> Guess my preference would be to do this everywhere and then let's discuss if
> we change the warning into error there or keep it being deprecated.

Agreed FWIW.  On turning it into an error: I think it might be better
to wait a bit longer if we can.

Richard


>
> Though, let's see what others want to say about this.
>
>> 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.
>
> 	Jakub

Re: [PATCH] Ignore the clobbered stack pointer in asm statment

[Jakub Jelinek]

On Wed, Sep 16, 2020 at 12:34:50PM +0100, Richard Sandiford wrote:
> Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
> >> Something like this for GCC 8 and 9.
> >
> > Guess my preference would be to do this everywhere and then let's discuss if
> > we change the warning into error there or keep it being deprecated.
> 
> Agreed FWIW.  On turning it into an error: I think it might be better
> to wait a bit longer if we can.

Ok.  The patch is ok for trunk and affected release branches after a week.

> > Though, let's see what others want to say about this.
> >
> >> 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.

	Jakub

[GCC 8] [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Wed, Sep 16, 2020 at 4:47 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Sep 16, 2020 at 12:34:50PM +0100, Richard Sandiford wrote:
> > Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > > On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
> > >> Something like this for GCC 8 and 9.
> > >
> > > Guess my preference would be to do this everywhere and then let's discuss if
> > > we change the warning into error there or keep it being deprecated.
> >
> > Agreed FWIW.  On turning it into an error: I think it might be better
> > to wait a bit longer if we can.
>
> Ok.  The patch is ok for trunk and affected release branches after a week.
>

I cherry-picked it to GCC 9 and 10 branches.   GCC 8 needs some
changes.  I am enclosing the backported patch for GCC 8.  I will check
it in if there are no regressions on Linux/x86-64.

Thanks.

H.J.

[-- Attachment #2: 0001-rtl_data-Add-sp_is_clobbered_by_asm.patch --]
[-- Type: text/x-patch, Size: 3709 bytes --]

From 97c34eb5f57bb1d37f3feddefefa5f553bcea9fc Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 14 Sep 2020 08:52:27 -0700
Subject: [PATCH] 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 (expand_asm_stmt): 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.

(cherry picked from commit 453a20c65722719b9e2d84339f215e7ec87692dc)
---
 gcc/cfgexpand.c                         |  3 +++
 gcc/config/i386/i386.c                  |  6 ++++--
 gcc/emit-rtl.h                          |  3 +++
 gcc/testsuite/gcc.target/i386/pr97032.c | 22 ++++++++++++++++++++++
 4 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr97032.c

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 18565bf1dab..dcf491954f1 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2972,6 +2972,9 @@ expand_asm_stmt (gasm *stmt)
 			   regname);
 		    return;
 		  }
+		/* Clobbering the stack pointer register.  */
+		else if (reg == (int) STACK_POINTER_REGNUM)
+		  crtl->sp_is_clobbered_by_asm = true;
 
 	        SET_HARD_REG_BIT (clobbered_regs, reg);
 	        rtx x = gen_rtx_REG (reg_raw_mode[reg], reg);
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index f3c722b51e9..ce20bc2ab4e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12528,10 +12528,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 4e7bd1ec26d..55dc3e84e9c 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -265,6 +265,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..b9ef2ad0c05
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr97032.c
@@ -0,0 +1,22 @@
+/* { 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"
+		: "=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

Re: [GCC 8] [PATCH] Ignore the clobbered stack pointer in asm statment

[H.J. Lu]

On Thu, Sep 24, 2020 at 9:48 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Sep 16, 2020 at 4:47 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Wed, Sep 16, 2020 at 12:34:50PM +0100, Richard Sandiford wrote:
> > > Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > > > On Mon, Sep 14, 2020 at 08:57:18AM -0700, H.J. Lu via Gcc-patches wrote:
> > > >> Something like this for GCC 8 and 9.
> > > >
> > > > Guess my preference would be to do this everywhere and then let's discuss if
> > > > we change the warning into error there or keep it being deprecated.
> > >
> > > Agreed FWIW.  On turning it into an error: I think it might be better
> > > to wait a bit longer if we can.
> >
> > Ok.  The patch is ok for trunk and affected release branches after a week.
> >
>
> I cherry-picked it to GCC 9 and 10 branches.   GCC 8 needs some
> changes.  I am enclosing the backported patch for GCC 8.  I will check
> it in if there are no regressions on Linux/x86-64.
>

No regression.  I am checking it into GCC 8 branch.

-- 
H.J.