[PATCH] [RFC] PR target/52813 and target/11807

[PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

I have tested this fix on x86_64 host, and found no regression in the C
and C++ testsuites.  I'm marking this patch as RFC simply because I don't
have experience with other architectures, and I don't have a setup to
test all architectures supported by GCC.

gcc/ChangeLog:

2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>

	* cfgexpand.c (asm_clobber_reg_is_valid): Also produce
	error when stack pointer is clobbered.
	(expand_asm_stmt): Refactor clobber check in separate function.

gcc/testsuite/ChangeLog:

2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>

	* gcc.target/i386/pr52813.c: New test.

Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
---
 gcc/cfgexpand.c                         | 42 ++++++++++++++++++++++++++-------
 gcc/testsuite/gcc.target/i386/pr52813.c |  9 +++++++
 2 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr52813.c

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 5e23bc242b9..8474372a216 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2845,6 +2845,38 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_SET *clobbered_regs)
   return false;
 }
 
+/* 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)
+{
+  bool is_valid = true;
+  HARD_REG_SET regset;
+
+  CLEAR_HARD_REG_SET (regset);
+
+  add_range_to_hard_reg_set (&regset, regno, nregs);
+
+  /* Clobbering the PIC register is an error.  */
+  if (PIC_OFFSET_TABLE_REGNUM != INVALID_REGNUM
+      && overlaps_hard_reg_set_p (regset, Pmode, PIC_OFFSET_TABLE_REGNUM))
+    {
+      /* ??? Diagnose during gimplification?  */
+      error ("PIC register clobbered by %qs in %<asm%>", regname);
+      is_valid = false;
+    }
+  /* Clobbering the STACK POINTER register is an error.  */
+  if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
+    {
+      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
+      is_valid = false;
+    }
+
+  return is_valid;
+}
+
 /* Generate RTL for an asm statement with arguments.
    STRING is the instruction template.
    OUTPUTS is a list of output arguments (lvalues); INPUTS a list of inputs.
@@ -2977,14 +3009,8 @@ expand_asm_stmt (gasm *stmt)
 	  else
 	    for (int reg = j; reg < j + nregs; reg++)
 	      {
-		/* Clobbering the PIC register is an error.  */
-		if (reg == (int) PIC_OFFSET_TABLE_REGNUM)
-		  {
-		    /* ??? Diagnose during gimplification?  */
-		    error ("PIC register clobbered by %qs in %<asm%>",
-			   regname);
-		    return;
-		  }
+		if (!asm_clobber_reg_is_valid (reg, nregs, regname))
+		  return;
 
 	        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/pr52813.c b/gcc/testsuite/gcc.target/i386/pr52813.c
new file mode 100644
index 00000000000..154ebbfc423
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr52813.c
@@ -0,0 +1,9 @@
+/* Ensure that stack pointer cannot be an asm clobber.  */
+/* { dg-do compile { target { ! ia32 } } } */
+/* { dg-options "-O2" } */
+
+void
+test1 (void)
+{
+  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+}
-- 
2.11.0

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Dimitar Dimitrov <dimitar@dinux.eu> writes:
> I have tested this fix on x86_64 host, and found no regression in the C
> and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> have experience with other architectures, and I don't have a setup to
> test all architectures supported by GCC.
>
> gcc/ChangeLog:
>
> 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
>
> 	* cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> 	error when stack pointer is clobbered.
> 	(expand_asm_stmt): Refactor clobber check in separate function.
>
> gcc/testsuite/ChangeLog:
>
> 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
>
> 	* gcc.target/i386/pr52813.c: New test.
>
> Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>

LGTM.  Do you have a copyright assignment on file?  'Fraid this is
probably big enough to need one.

Thanks,
Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > I have tested this fix on x86_64 host, and found no regression in the C
> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > have experience with other architectures, and I don't have a setup to
> > test all architectures supported by GCC.
> > 
> > gcc/ChangeLog:
> > 
> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > 
> > 	* cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > 	error when stack pointer is clobbered.
> > 	(expand_asm_stmt): Refactor clobber check in separate function.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > 
> > 	* gcc.target/i386/pr52813.c: New test.
> > 
> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> 
> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> probably big enough to need one.
Yes, I have copyright assignment.

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Dimitar Dimitrov <dimitar@dinux.eu> writes:
> On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>> > I have tested this fix on x86_64 host, and found no regression in the C
>> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
>> > have experience with other architectures, and I don't have a setup to
>> > test all architectures supported by GCC.
>> > 
>> > gcc/ChangeLog:
>> > 
>> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
>> > 
>> > 	* cfgexpand.c (asm_clobber_reg_is_valid): Also produce
>> > 	error when stack pointer is clobbered.
>> > 	(expand_asm_stmt): Refactor clobber check in separate function.
>> > 
>> > gcc/testsuite/ChangeLog:
>> > 
>> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
>> > 
>> > 	* gcc.target/i386/pr52813.c: New test.
>> > 
>> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
>> 
>> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
>> probably big enough to need one.
> Yes, I have copyright assignment.

OK, great.  I went ahead and applied the patch.

Thanks,
Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> >> > I have tested this fix on x86_64 host, and found no regression in the C
> >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> >> > have experience with other architectures, and I don't have a setup to
> >> > test all architectures supported by GCC.
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> >> >
> >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> >> >    error when stack pointer is clobbered.
> >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> >> >
> >> > gcc/testsuite/ChangeLog:
> >> >
> >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> >> >
> >> >    * gcc.target/i386/pr52813.c: New test.
> >> >
> >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> >>
> >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> >> probably big enough to need one.
> > Yes, I have copyright assignment.
>
> OK, great.  I went ahead and applied the patch.
>

Hi,

This patch introduces a regression on arm:
FAIL: gcc.target/arm/pr77904.c (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
register clobbered by 'sp' in 'asm'

Indeed the testcase has an explicit:
  __asm volatile ("" : : : "sp");
which is now rejected.

Thomas, is that mandatory to test your code to fix pr77904?

Thanks,

Christophe

> Thanks,
> Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

On Wed, 12 Dec 2018 at 10:42, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > >> > have experience with other architectures, and I don't have a setup to
> > >> > test all architectures supported by GCC.
> > >> >
> > >> > gcc/ChangeLog:
> > >> >
> > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > >> >
> > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > >> >    error when stack pointer is clobbered.
> > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > >> >
> > >> > gcc/testsuite/ChangeLog:
> > >> >
> > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > >> >
> > >> >    * gcc.target/i386/pr52813.c: New test.
> > >> >
> > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > >>
> > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > >> probably big enough to need one.
> > > Yes, I have copyright assignment.
> >
> > OK, great.  I went ahead and applied the patch.
> >
>
> Hi,
>
> This patch introduces a regression on arm:
> FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> Excess errors:
> /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> register clobbered by 'sp' in 'asm'
>
> Indeed the testcase has an explicit:
>   __asm volatile ("" : : : "sp");
> which is now rejected.
>
> Thomas, is that mandatory to test your code to fix pr77904?
>
> Thanks,
>
> Christophe
>

And just noticed it causes a failure to build GDB for x86_64:
gdb-8.1-release/gdb/nat/linux-ptrace.c: In function 'void
linux_ptrace_init_warnings()':
gdb-8.1-release/gdb/nat/linux-ptrace.c:149:23: error: Stack Pointer
register clobbered by '%rsp' in 'asm'
  149 |    : "%rsp", "memory");
      |                       ^
Makefile:1640: recipe for target 'linux-ptrace.o' failed

I didn't check if the GDB code is legitimate though....

> > Thanks,
> > Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Thomas Preudhomme]

Hi Christophe,

That PR was about a bug occuring when sp was clobbered so if it cannot
be clobbered anymore the whole commit (r242693) can be removed. Let me
check the original code that lead to the PR why it's clobbering sp
though.

Best regards,

Thomas
On Wed, 12 Dec 2018 at 09:43, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > >> > have experience with other architectures, and I don't have a setup to
> > >> > test all architectures supported by GCC.
> > >> >
> > >> > gcc/ChangeLog:
> > >> >
> > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > >> >
> > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > >> >    error when stack pointer is clobbered.
> > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > >> >
> > >> > gcc/testsuite/ChangeLog:
> > >> >
> > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > >> >
> > >> >    * gcc.target/i386/pr52813.c: New test.
> > >> >
> > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > >>
> > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > >> probably big enough to need one.
> > > Yes, I have copyright assignment.
> >
> > OK, great.  I went ahead and applied the patch.
> >
>
> Hi,
>
> This patch introduces a regression on arm:
> FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> Excess errors:
> /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> register clobbered by 'sp' in 'asm'
>
> Indeed the testcase has an explicit:
>   __asm volatile ("" : : : "sp");
> which is now rejected.
>
> Thomas, is that mandatory to test your code to fix pr77904?
>
> Thanks,
>
> Christophe
>
> > Thanks,
> > Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Thomas Preudhomme]

So my understanding is that the original code (CMSIS library) used to
clobber sp because the asm statement was actually changing the sp.
That in turn led GCC to try to save and restore sp which is not what
CMSIS was expecting to happen. Changing sp without clobber as done now
is probably the right solution and r242693 can be reverted. That will
remove the failing test.

Best regards,

Thomas
On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
>
> Hi Christophe,
>
> That PR was about a bug occuring when sp was clobbered so if it cannot
> be clobbered anymore the whole commit (r242693) can be removed. Let me
> check the original code that lead to the PR why it's clobbering sp
> though.
>
> Best regards,
>
> Thomas
> On Wed, 12 Dec 2018 at 09:43, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> >
> > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> > >
> > > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > > >> > have experience with other architectures, and I don't have a setup to
> > > >> > test all architectures supported by GCC.
> > > >> >
> > > >> > gcc/ChangeLog:
> > > >> >
> > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > >> >
> > > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > > >> >    error when stack pointer is clobbered.
> > > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > > >> >
> > > >> > gcc/testsuite/ChangeLog:
> > > >> >
> > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > >> >
> > > >> >    * gcc.target/i386/pr52813.c: New test.
> > > >> >
> > > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > > >>
> > > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > > >> probably big enough to need one.
> > > > Yes, I have copyright assignment.
> > >
> > > OK, great.  I went ahead and applied the patch.
> > >
> >
> > Hi,
> >
> > This patch introduces a regression on arm:
> > FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> > Excess errors:
> > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> > register clobbered by 'sp' in 'asm'
> >
> > Indeed the testcase has an explicit:
> >   __asm volatile ("" : : : "sp");
> > which is now rejected.
> >
> > Thomas, is that mandatory to test your code to fix pr77904?
> >
> > Thanks,
> >
> > Christophe
> >
> > > Thanks,
> > > Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Andreas Schwab]

This breaks ia64:

In file included from ../../../libgomp/config/linux/wait.h:46,
                 from ../../../libgomp/config/linux/mutex.c:30:
../../../libgomp/config/linux/ia64/futex.h: In function 'gomp_mutex_lock_slow':
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~
../../../libgomp/config/linux/ia64/futex.h: In function 'gomp_mutex_unlock_slow':
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~
../../../libgomp/config/linux/ia64/futex.h:43:3: error: Stack Pointer register clobbered by 'r12' in 'asm'
   43 |   __asm __volatile ("break 0x100000"
      |   ^~~~~

Installed as obvious.

Andreas.

	* config/linux/ia64/futex.h (sys_futex0): Don't mark r12 as
	clobbered.
---
 libgomp/config/linux/ia64/futex.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libgomp/config/linux/ia64/futex.h b/libgomp/config/linux/ia64/futex.h
index 6efec3c813..df450f8def 100644
--- a/libgomp/config/linux/ia64/futex.h
+++ b/libgomp/config/linux/ia64/futex.h
@@ -45,8 +45,8 @@ sys_futex0(int *addr, int op, int val)
 	  "=r"(r8), "=r"(r10)
 	: "r"(r15), "r"(out0), "r"(out1), "r"(out2), "r"(out3)
 	: "memory", "out4", "out5", "out6", "out7",
-	  /* Non-stacked integer registers, minus r8, r10, r15.  */
-	  "r2", "r3", "r9", "r11", "r12", "r13", "r14", "r16", "r17", "r18",
+	  /* Non-stacked integer registers, minus r8, r10, r12, r15.  */
+	  "r2", "r3", "r9", "r11", "r13", "r14", "r16", "r17", "r18",
 	  "r19", "r20", "r21", "r22", "r23", "r24", "r25", "r26", "r27",
 	  "r28", "r29", "r30", "r31",
 	  /* Predicate registers.  */
-- 
2.20.0

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
<thomas.preudhomme@linaro.org> wrote:
>
> So my understanding is that the original code (CMSIS library) used to
> clobber sp because the asm statement was actually changing the sp.
> That in turn led GCC to try to save and restore sp which is not what
> CMSIS was expecting to happen. Changing sp without clobber as done now
> is probably the right solution and r242693 can be reverted. That will
> remove the failing test.
>

OK, I read PR52813 too, but I'm not sure to fully understand the new status.
My understanding is that since this patch was committed, if an asm statement
clobbers sp, it is now allowed to actually declare it as clobber (this patch
generates an error in such a case).
So the user is now expected to lie to the compiler when writing to
this kind of register (sp, pic register), by not declaring it as "clobber"?


> Best regards,
>
> Thomas
> On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme
> <thomas.preudhomme@linaro.org> wrote:
> >
> > Hi Christophe,
> >
> > That PR was about a bug occuring when sp was clobbered so if it cannot
> > be clobbered anymore the whole commit (r242693) can be removed. Let me
> > check the original code that lead to the PR why it's clobbering sp
> > though.
> >
> > Best regards,
> >
> > Thomas
> > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon
> > <christophe.lyon@linaro.org> wrote:
> > >
> > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> > > <richard.sandiford@arm.com> wrote:
> > > >
> > > > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > > > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > > > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > > > >> > have experience with other architectures, and I don't have a setup to
> > > > >> > test all architectures supported by GCC.
> > > > >> >
> > > > >> > gcc/ChangeLog:
> > > > >> >
> > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > >> >
> > > > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > > > >> >    error when stack pointer is clobbered.
> > > > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > > > >> >
> > > > >> > gcc/testsuite/ChangeLog:
> > > > >> >
> > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > >> >
> > > > >> >    * gcc.target/i386/pr52813.c: New test.
> > > > >> >
> > > > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > > > >>
> > > > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > > > >> probably big enough to need one.
> > > > > Yes, I have copyright assignment.
> > > >
> > > > OK, great.  I went ahead and applied the patch.
> > > >
> > >
> > > Hi,
> > >
> > > This patch introduces a regression on arm:
> > > FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> > > Excess errors:
> > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> > > register clobbered by 'sp' in 'asm'
> > >
> > > Indeed the testcase has an explicit:
> > >   __asm volatile ("" : : : "sp");
> > > which is now rejected.
> > >
> > > Thomas, is that mandatory to test your code to fix pr77904?
> > >
> > > Thanks,
> > >
> > > Christophe
> > >
> > > > Thanks,
> > > > Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

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

On Wed, 12 Dec 2018 at 14:19, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
> <thomas.preudhomme@linaro.org> wrote:
> >
> > So my understanding is that the original code (CMSIS library) used to
> > clobber sp because the asm statement was actually changing the sp.
> > That in turn led GCC to try to save and restore sp which is not what
> > CMSIS was expecting to happen. Changing sp without clobber as done now
> > is probably the right solution and r242693 can be reverted. That will
> > remove the failing test.
> >
>
> OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> My understanding is that since this patch was committed, if an asm statement
> clobbers sp, it is now allowed to actually declare it as clobber (this patch
> generates an error in such a case).
> So the user is now expected to lie to the compiler when writing to
> this kind of register (sp, pic register), by not declaring it as "clobber"?
>

I'm attaching a small patch which adds a more verbose error message
along the lines of what I understand of the current status.
I'm pretty sure I got (at least) the formatting wrong :)

Christophe

>
> > Best regards,
> >
> > Thomas
> > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme
> > <thomas.preudhomme@linaro.org> wrote:
> > >
> > > Hi Christophe,
> > >
> > > That PR was about a bug occuring when sp was clobbered so if it cannot
> > > be clobbered anymore the whole commit (r242693) can be removed. Let me
> > > check the original code that lead to the PR why it's clobbering sp
> > > though.
> > >
> > > Best regards,
> > >
> > > Thomas
> > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon
> > > <christophe.lyon@linaro.org> wrote:
> > > >
> > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> > > > <richard.sandiford@arm.com> wrote:
> > > > >
> > > > > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > > > > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > > > > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > > > > >> > have experience with other architectures, and I don't have a setup to
> > > > > >> > test all architectures supported by GCC.
> > > > > >> >
> > > > > >> > gcc/ChangeLog:
> > > > > >> >
> > > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > > >> >
> > > > > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > > > > >> >    error when stack pointer is clobbered.
> > > > > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > > > > >> >
> > > > > >> > gcc/testsuite/ChangeLog:
> > > > > >> >
> > > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > > >> >
> > > > > >> >    * gcc.target/i386/pr52813.c: New test.
> > > > > >> >
> > > > > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > > > > >>
> > > > > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > > > > >> probably big enough to need one.
> > > > > > Yes, I have copyright assignment.
> > > > >
> > > > > OK, great.  I went ahead and applied the patch.
> > > > >
> > > >
> > > > Hi,
> > > >
> > > > This patch introduces a regression on arm:
> > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> > > > Excess errors:
> > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> > > > register clobbered by 'sp' in 'asm'
> > > >
> > > > Indeed the testcase has an explicit:
> > > >   __asm volatile ("" : : : "sp");
> > > > which is now rejected.
> > > >
> > > > Thomas, is that mandatory to test your code to fix pr77904?
> > > >
> > > > Thanks,
> > > >
> > > > Christophe
> > > >
> > > > > Thanks,
> > > > > Richard

[-- Attachment #2: pr52813.chlog.txt --]
[-- Type: text/plain, Size: 148 bytes --]

2018-12-12  Christophe Lyon  <christophe.lyon@linaro.org>

	gcc/
	* cfgexpand.c (asm_clobber_reg_is_valid): Add a more descriptive
	error message.


[-- Attachment #3: pr52813.patch.txt --]
[-- Type: text/plain, Size: 1072 bytes --]

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 0d04bbc..e1f2bff 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2870,12 +2870,20 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
     {
       /* ??? Diagnose during gimplification?  */
       error ("PIC register clobbered by %qs in %<asm%>", regname);
+      error ("Such clobbers are not supported by GCC. "
+	     "If you really want to overwrite the PIC register, "
+	     "remove it from the clobber list in the %<asm%> at your own risk: "
+	     "GCC will not save it.");
       is_valid = false;
     }
   /* Clobbering the STACK POINTER register is an error.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
       error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
+      error ("Such clobbers are not supported by GCC. "
+	     "If you really want to overwrite the Stack Pointer, "
+	     "remove it from the clobber list in the %<asm%> at your own risk: "
+	     "GCC will not save it.");
       is_valid = false;
     }
 

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Thomas Preudhomme]

[resending from the right address]

Hi Christophe,

Why not simply: "Clobber of <REGISTER> unsupported" with an
accompanying change of the documentation to state the extra bit you
wanted to put in that error message? Perhaps even add a reference to
the section of the documentation in the error message.


Best regards,


Thomas
On Wed, 12 Dec 2018 at 15:13, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Wed, 12 Dec 2018 at 14:19, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> >
> > On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
> > <thomas.preudhomme@linaro.org> wrote:
> > >
> > > So my understanding is that the original code (CMSIS library) used to
> > > clobber sp because the asm statement was actually changing the sp.
> > > That in turn led GCC to try to save and restore sp which is not what
> > > CMSIS was expecting to happen. Changing sp without clobber as done now
> > > is probably the right solution and r242693 can be reverted. That will
> > > remove the failing test.
> > >
> >
> > OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> > My understanding is that since this patch was committed, if an asm statement
> > clobbers sp, it is now allowed to actually declare it as clobber (this patch
> > generates an error in such a case).
> > So the user is now expected to lie to the compiler when writing to
> > this kind of register (sp, pic register), by not declaring it as "clobber"?
> >
>
> I'm attaching a small patch which adds a more verbose error message
> along the lines of what I understand of the current status.
> I'm pretty sure I got (at least) the formatting wrong :)
>
> Christophe
>
> >
> > > Best regards,
> > >
> > > Thomas
> > > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme
> > > <thomas.preudhomme@linaro.org> wrote:
> > > >
> > > > Hi Christophe,
> > > >
> > > > That PR was about a bug occuring when sp was clobbered so if it cannot
> > > > be clobbered anymore the whole commit (r242693) can be removed. Let me
> > > > check the original code that lead to the PR why it's clobbering sp
> > > > though.
> > > >
> > > > Best regards,
> > > >
> > > > Thomas
> > > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon
> > > > <christophe.lyon@linaro.org> wrote:
> > > > >
> > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> > > > > <richard.sandiford@arm.com> wrote:
> > > > > >
> > > > > > Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford wrote:
> > > > > > >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > > > > > >> > I have tested this fix on x86_64 host, and found no regression in the C
> > > > > > >> > and C++ testsuites.  I'm marking this patch as RFC simply because I don't
> > > > > > >> > have experience with other architectures, and I don't have a setup to
> > > > > > >> > test all architectures supported by GCC.
> > > > > > >> >
> > > > > > >> > gcc/ChangeLog:
> > > > > > >> >
> > > > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > > > >> >
> > > > > > >> >    * cfgexpand.c (asm_clobber_reg_is_valid): Also produce
> > > > > > >> >    error when stack pointer is clobbered.
> > > > > > >> >    (expand_asm_stmt): Refactor clobber check in separate function.
> > > > > > >> >
> > > > > > >> > gcc/testsuite/ChangeLog:
> > > > > > >> >
> > > > > > >> > 2018-12-07  Dimitar Dimitrov  <dimitar@dinux.eu>
> > > > > > >> >
> > > > > > >> >    * gcc.target/i386/pr52813.c: New test.
> > > > > > >> >
> > > > > > >> > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
> > > > > > >>
> > > > > > >> LGTM.  Do you have a copyright assignment on file?  'Fraid this is
> > > > > > >> probably big enough to need one.
> > > > > > > Yes, I have copyright assignment.
> > > > > >
> > > > > > OK, great.  I went ahead and applied the patch.
> > > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > This patch introduces a regression on arm:
> > > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors)
> > > > > Excess errors:
> > > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer
> > > > > register clobbered by 'sp' in 'asm'
> > > > >
> > > > > Indeed the testcase has an explicit:
> > > > >   __asm volatile ("" : : : "sp");
> > > > > which is now rejected.
> > > > >
> > > > > Thomas, is that mandatory to test your code to fix pr77904?
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Christophe
> > > > >
> > > > > > Thanks,
> > > > > > Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Wed, 12 Dec 2018 at 14:19:27 EET Christophe Lyon wrote:
> On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
> 
> <thomas.preudhomme@linaro.org> wrote:
> > So my understanding is that the original code (CMSIS library) used to
> > clobber sp because the asm statement was actually changing the sp.
> > That in turn led GCC to try to save and restore sp which is not what
> > CMSIS was expecting to happen. Changing sp without clobber as done now
> > is probably the right solution and r242693 can be reverted. That will
> > remove the failing test.
> 
> OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> My understanding is that since this patch was committed, if an asm
> statement clobbers sp, it is now allowed to actually declare it as clobber
> (this patch generates an error in such a case).
> So the user is now expected to lie to the compiler when writing to
> this kind of register (sp, pic register), by not declaring it as "clobber"?

Disclosure: I'm a GCC newbie.

I expect that if I mark a HW register as "clobber", compiler would save its 
contents before executing the asm statement, and after that it would restore 
its contents. This is the GCC behaviour for all but the SP and PIC registers. 
That is why I believe that PR52813 is a valid bug. 


I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered in 
such a way that GCC will not notice (e.g. thread switching), then why should 
GCC know about it in the first place?

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Wed, 12 Dec 2018 at 11:03:24 EET Christophe Lyon wrote:
> And just noticed it causes a failure to build GDB for x86_64:
> gdb-8.1-release/gdb/nat/linux-ptrace.c: In function 'void
> linux_ptrace_init_warnings()':
> gdb-8.1-release/gdb/nat/linux-ptrace.c:149:23: error: Stack Pointer
> register clobbered by '%rsp' in 'asm'
>   149 |    : "%rsp", "memory");
> 
> | ^
> 
> Makefile:1640: recipe for target 'linux-ptrace.o' failed
> 
> I didn't check if the GDB code is legitimate though....

Sorry about this. I had checked the Linux x86 kernel for SP clobbers, but 
forgot that GDB could also use such magic.

I'll try to fix it and send a patch to GDB. It will likely take me a few days, 
so I hope that this breakage is not considered a P0 bug.

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote:
> I expect that if I mark a HW register as "clobber", compiler would save its 
> contents before executing the asm statement, and after that it would restore 
> its contents. This is the GCC behaviour for all but the SP and PIC registers. 
> That is why I believe that PR52813 is a valid bug. 

It won't do it for *any* fixed registers.  But you do not want to error
or even warn for some fixed registers, for example the "flags" register
on x86 is *always* written to by asm.

But you never want to warn for non-fixed registers, and e.g.
PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0
for example).

> I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered in 
> such a way that GCC will not notice (e.g. thread switching), then why should 
> GCC know about it in the first place?

Up until today, GCC has always just ignored it if you claimed to clobber
the stack pointer.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Thu, Dec 13, 2018 at 8:48:38 EET Segher Boessenkool wrote:
> On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote:
> > I expect that if I mark a HW register as "clobber", compiler would save
> > its
> > contents before executing the asm statement, and after that it would
> > restore its contents. This is the GCC behaviour for all but the SP and
> > PIC registers. That is why I believe that PR52813 is a valid bug.
> 
> It won't do it for *any* fixed registers.  But you do not want to error
> or even warn for some fixed registers, for example the "flags" register
> on x86 is *always* written to by asm.

Yes, you are correct.

> 
> But you never want to warn for non-fixed registers, and e.g.
> PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0
> for example).
I  could not trace how PIC_OFFSET_TABLE_REGNUM on i386 gets marked as fixed 
register. I'll dig more through the source.

> 
> > I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered
> > in such a way that GCC will not notice (e.g. thread switching), then why
> > should GCC know about it in the first place?
> 
> Up until today, GCC has always just ignored it if you claimed to clobber
> the stack pointer.

My point is that the silent ignoring is confusing to users, as shown by 
PR52813. How would you suggest me to proceed:
 - Leave patch as-is.
 - Revert patch. Update documentation to point that clobber marker for fixed 
registers is ignored by GCC. Close PR52813 as invalid.
 - Revert patch. Discuss more broadly and specify behaviour of asm clobber for 
fixed registers (and SP in particular).

Thanks,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Thu, Dec 13, 2018 at 11:26:40PM +0200, Dimitar Dimitrov wrote:
> On Thu, Dec 13, 2018 at 8:48:38 EET Segher Boessenkool wrote:
> > On Wed, Dec 12, 2018 at 06:26:10PM +0200, Dimitar Dimitrov wrote:
> > > I expect that if I mark a HW register as "clobber", compiler would save
> > > its
> > > contents before executing the asm statement, and after that it would
> > > restore its contents. This is the GCC behaviour for all but the SP and
> > > PIC registers. That is why I believe that PR52813 is a valid bug.
> > 
> > It won't do it for *any* fixed registers.  But you do not want to error
> > or even warn for some fixed registers, for example the "flags" register
> > on x86 is *always* written to by asm.
> 
> Yes, you are correct.
> 
> > But you never want to warn for non-fixed registers, and e.g.
> > PIC_OFFSET_TABLE_REGNUM isn't always a fixed register (when flag_pic is 0
> > for example).
> I  could not trace how PIC_OFFSET_TABLE_REGNUM on i386 gets marked as fixed 
> register. I'll dig more through the source.
> 
> > > I'm not sure how GCC could recover if SP is clobbered. If SP is clobbered
> > > in such a way that GCC will not notice (e.g. thread switching), then why
> > > should GCC know about it in the first place?
> > 
> > Up until today, GCC has always just ignored it if you claimed to clobber
> > the stack pointer.
> 
> My point is that the silent ignoring is confusing to users, as shown by 
> PR52813. How would you suggest me to proceed:
>  - Leave patch as-is.
>  - Revert patch. Update documentation to point that clobber marker for fixed 
> registers is ignored by GCC. Close PR52813 as invalid.
>  - Revert patch. Discuss more broadly and specify behaviour of asm clobber for 
> fixed registers (and SP in particular).

You need a few tweaks to what you committed.  Or just one perhaps: if
flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is
meaningless in that case.  I'm not sure if you need to check whether the
register is fixed or not.

But there are many more regs than just the PIC reg and the stack pointer
that you would want to similarly warn about, because overwriting those
registers is just as fatal: the frame pointer, the program counter, etc.
_Most_ fixed registers, but not _all_.

So maybe it should be a target hook?  OTOH that is a lot of work for such
a trivial warning, that isn't very useful anyway (a better warning for
any asm is: "Are you sure?" :-) )

So I don't know what is best to do (except that flag_pic part).  Sorry.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

(Maybe the discussion has moved on from this already -- sorry if so)

Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
> <thomas.preudhomme@linaro.org> wrote:
>>
>> So my understanding is that the original code (CMSIS library) used to
>> clobber sp because the asm statement was actually changing the sp.
>> That in turn led GCC to try to save and restore sp which is not what
>> CMSIS was expecting to happen. Changing sp without clobber as done now
>> is probably the right solution and r242693 can be reverted. That will
>> remove the failing test.
>>
>
> OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> My understanding is that since this patch was committed, if an asm statement
> clobbers sp, it is now allowed to actually declare it as clobber (this patch
> generates an error in such a case).
> So the user is now expected to lie to the compiler when writing to
> this kind of register (sp, pic register), by not declaring it as "clobber"?

The user isn't expected to lie.  The point is that GCC simply doesn't
support asms that leave the stack pointer with a different value from
before, and IMO never has.  If that happened to work in some cases then
it was purely an accident.

The PRs also show disagreement about what GCC should do for an asm like
that.  The asm in PR52813 temporarily changed the stack pointer and the
bug was that GCC didn't restore the original value afterwards.  The asm
in PR77904 was trying to set the stack pointer to an entirely new value
and the bug was the GCC did restore the original value afterwards,
defeating the point.

This wouldn't be the first time that there's disagreement about what
the behaviour should be.  But IMO we can't support either reliably.
Spilling sp is dangerous in general because we might need the stack
for the reload, or we might accidentally try to reload something else
before restoring the stack pointer.  And continuing with a new sp
provided by the asm could lead to all sorts of problems.  (AIUI, the
point of PR77904 was that it would also be wrong for GCC to set up a
frame pointer and restore the sp from that frame pointer on function
exit.  The new sp value was supposed to survive.  So the answer isn't
simply "use a frame pointer".)

Thanks,
Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Fri, Dec 14, 2018 at 01:49:40PM +0000, Richard Sandiford wrote:
> > OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> > My understanding is that since this patch was committed, if an asm statement
> > clobbers sp, it is now allowed to actually declare it as clobber (this patch
> > generates an error in such a case).
> > So the user is now expected to lie to the compiler when writing to
> > this kind of register (sp, pic register), by not declaring it as "clobber"?
> 
> The user isn't expected to lie.  The point is that GCC simply doesn't
> support asms that leave the stack pointer with a different value from
> before, and IMO never has.  If that happened to work in some cases then
> it was purely an accident.

Yup.


It now errors for

void f(void)
{
      asm("ohmy" ::: "sp");
}

but not for

void f(void)
{
        register long x asm("sp");
        asm("ohmy %0" : "=r"(x));
}

which is the same problem.

(I would be happier if it was a warning instead of an error btw, since
there apparently is existing code that uses a clobber of sp, and GCC has
always worked with that, accidentally or not).

> The PRs also show disagreement about what GCC should do for an asm like
> that.  The asm in PR52813 temporarily changed the stack pointer and the
> bug was that GCC didn't restore the original value afterwards.  The asm
> in PR77904 was trying to set the stack pointer to an entirely new value
> and the bug was the GCC did restore the original value afterwards,
> defeating the point.
> 
> This wouldn't be the first time that there's disagreement about what
> the behaviour should be.  But IMO we can't support either reliably.
> Spilling sp is dangerous in general because we might need the stack
> for the reload, or we might accidentally try to reload something else
> before restoring the stack pointer.  And continuing with a new sp
> provided by the asm could lead to all sorts of problems.  (AIUI, the
> point of PR77904 was that it would also be wrong for GCC to set up a
> frame pointer and restore the sp from that frame pointer on function
> exit.  The new sp value was supposed to survive.  So the answer isn't
> simply "use a frame pointer".)

Inline asm cannot do things that are not allowed by the ABI, or that
touch other things in the execution environment you shouldn't touch.

Apparently some people really try to clobber sp, and this new error
will help them a bit.  I don't know how useful that is, is it better
to scare people away from using inline asm?  It might well be safer
for everyone...


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Fri, Dec 14 2018 2:52:17 EET Segher Boessenkool wrote:
> You need a few tweaks to what you committed.  Or just one perhaps: if
> flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is
> meaningless in that case.  I'm not sure if you need to check whether the
> register is fixed or not.
The flag_pic flag is already checked by the PIC_OFFSET_TABLE_REGNUM macro. It 
will return INVALID_REGNUM if flag_pic is false, so no error will be printed.

Note that the PIC_OFFSET_TABLE_REGNUM behaviour is not changed by my patch. I 
merely moved the existing check into a separate function.

> 
> But there are many more regs than just the PIC reg and the stack pointer
> that you would want to similarly warn about, because overwriting those
> registers is just as fatal: the frame pointer, the program counter, etc.
> _Most_ fixed registers, but not _all_.
> 
> So maybe it should be a target hook?  OTOH that is a lot of work for such
> a trivial warning, that isn't very useful anyway (a better warning for
> any asm is: "Are you sure?" :-) )
I'll think about a more generic solution. But in light of the several filed 
PRs I think it's worth having a simple check for SP.

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Sun, Dec 16, 2018 at 10:43:47AM +0200, Dimitar Dimitrov wrote:
> On Fri, Dec 14 2018 2:52:17 EET Segher Boessenkool wrote:
> > You need a few tweaks to what you committed.  Or just one perhaps: if
> > flag_pic is not set, you should not check PIC_OFFSET_TABLE_REGNUM, it is
> > meaningless in that case.  I'm not sure if you need to check whether the
> > register is fixed or not.
> The flag_pic flag is already checked by the PIC_OFFSET_TABLE_REGNUM macro. It 
> will return INVALID_REGNUM if flag_pic is false, so no error will be printed.

No, it is not.  On at least six targets the macro is simply defined as a
register number.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Fri, 11 Jan 2019 at 23:59, Jeff Law <law@redhat.com> wrote:
>>
>> On 1/8/19 5:03 AM, Richard Sandiford wrote:
>> > Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> >> On 1/7/19 10:23 AM, Jakub Jelinek wrote:
>> >>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
>> >>>> -  /* Clobbering the STACK POINTER register is an error.  */
>> >>>> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
>> >>>> +     which is often unexpected by users.  See PR52813.  */
>> >>>>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
>> >>>>      {
>> >>>> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
>> >>>> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
>> >>>> +         regname);
>> >>>> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
>> >>>
>> >>> Why do we write Stack Pointer rather than stack pointer?  That is really
>> >>> weird.  The second warning would be a note based on the first one, i.e.
>> >>> if (warning ()) note ();
>> >>> and better have some -W* option to silence the warning.
>> >>>
>> >>
>> >> Yes, thanks for this suggestion.
>> >>
>> >> Meanwhile I found out, that the stack clobber has only been ignored up to
>> >> gcc-5 (at least with lra targets, not really sure about reload targets).
>> >> From gcc-6 on, with the exception of PR arm/77904 which was a regression due
>> >> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
>> >> this works for all targets I tried so far.
>> >>
>> >> To me, it starts to look like a rather unique and useful feature, that I would
>> >> like to keep working.
>> >
>> > Not sure what you mean by "unique".  But forcing a frame is a bit of
>> > a slippery concept.  Force it where?  For the asm only, or the whole
>> > function?  This depends on optimisation and hasn't been consistent
>> > across GCC versions, since it depends on the shrink-wrapping
>> > optimisation.  (There was a similar controversy a while ago about
>> > to what extent -fno-omit-frame-pointer should "force a frame".)
>> >
>> > The effect on the redzone seems like something that should be specified
>> > explicitly rather than as an (accidental?) side effect of listing the
>> > sp in the clobber list.  Maybe this would be another use for the "asm
>> > attributes" proposal.  "noreturn" was another attribute suggested on
>> > IRC yesterday.
>> >
>> > But either way, the general feeling seems to be that going straight to a
>> > hard error is too harsh, since there's quite a bit of existing code that
>> > has the clobber.  This patch implements the compromise discussed on IRC
>> > yesterday of making it a -Wdeprecated warning instead.
>> >
>> > Tested on x86_64-linux-gnu and aarch64-linux-gnu.  OK to install?
>> >
>> > Richard
>> >
>> > Dimitar: sorry the run-around on this patch, and thanks for the
>> > submission.  It looks from all the controversy like it was a
>> > long-festering PR for a reason. :-/
>> >
>> >
>> > 2019-01-07  Richard Sandiford  <richard.sandiford@arm.com>
>> >
>> > gcc/
>> >       PR inline-asm/52813
>> >       * doc/extend.texi: Document that listing the stack pointer in the
>> >       clobber list of an asm is a deprecated feature.
>> >       * common.opt (Wdeprecated): Moved from c-family/c.opt.
>> >       * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated
>> >       warning instead of an error for clobbers of the stack pointer.
>> >       Add a note explaining why.
>> >
>> > gcc/c-family/
>> >       PR inline-asm/52813
>> >       * c.opt (Wdeprecated): Move documentation and variable to common.opt.
>> >
>> > gcc/d/
>> >       PR inline-asm/52813
>> >       * lang.opt (Wdeprecated): Reference common.opt instead of c.opt.
>> >
>> > gcc/testsuite/
>> >       PR inline-asm/52813
>> >       * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a
>> >       -Wdeprecated warning and expect a following note:.
>> OK.
>>
>> FWIW the number of packages affected in Fedora was in single digits,
>> some of which have already been fixed.
>>
>> But if folks want to go with a deprecated warning instead of straight to
>> an error, I won't complain.
>>
>> jeff
>
>
> Hi,
>
> I originally complained because the arm test for pr77904.c was failing.
> Since Richard's change that test emits a warning rather than an error,
> but still fails. This small patch adds the missing dg-warning.
>
> OK?
>
> Thanks,
>
> Christophe
>
> 2019-01-17  Christophe Lyon  <christophe.lyon@linaro.org>
>
> 	* gcc.target/arm/pr77904.c: Add dg-warning for sp clobber.

OK, thanks.

Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

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

On Fri, 11 Jan 2019 at 23:59, Jeff Law <law@redhat.com> wrote:
>
> On 1/8/19 5:03 AM, Richard Sandiford wrote:
> > Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >> On 1/7/19 10:23 AM, Jakub Jelinek wrote:
> >>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
> >>>> -  /* Clobbering the STACK POINTER register is an error.  */
> >>>> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
> >>>> +     which is often unexpected by users.  See PR52813.  */
> >>>>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
> >>>>      {
> >>>> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
> >>>> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
> >>>> +         regname);
> >>>> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
> >>>
> >>> Why do we write Stack Pointer rather than stack pointer?  That is really
> >>> weird.  The second warning would be a note based on the first one, i.e.
> >>> if (warning ()) note ();
> >>> and better have some -W* option to silence the warning.
> >>>
> >>
> >> Yes, thanks for this suggestion.
> >>
> >> Meanwhile I found out, that the stack clobber has only been ignored up to
> >> gcc-5 (at least with lra targets, not really sure about reload targets).
> >> From gcc-6 on, with the exception of PR arm/77904 which was a regression due
> >> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
> >> this works for all targets I tried so far.
> >>
> >> To me, it starts to look like a rather unique and useful feature, that I would
> >> like to keep working.
> >
> > Not sure what you mean by "unique".  But forcing a frame is a bit of
> > a slippery concept.  Force it where?  For the asm only, or the whole
> > function?  This depends on optimisation and hasn't been consistent
> > across GCC versions, since it depends on the shrink-wrapping
> > optimisation.  (There was a similar controversy a while ago about
> > to what extent -fno-omit-frame-pointer should "force a frame".)
> >
> > The effect on the redzone seems like something that should be specified
> > explicitly rather than as an (accidental?) side effect of listing the
> > sp in the clobber list.  Maybe this would be another use for the "asm
> > attributes" proposal.  "noreturn" was another attribute suggested on
> > IRC yesterday.
> >
> > But either way, the general feeling seems to be that going straight to a
> > hard error is too harsh, since there's quite a bit of existing code that
> > has the clobber.  This patch implements the compromise discussed on IRC
> > yesterday of making it a -Wdeprecated warning instead.
> >
> > Tested on x86_64-linux-gnu and aarch64-linux-gnu.  OK to install?
> >
> > Richard
> >
> > Dimitar: sorry the run-around on this patch, and thanks for the
> > submission.  It looks from all the controversy like it was a
> > long-festering PR for a reason. :-/
> >
> >
> > 2019-01-07  Richard Sandiford  <richard.sandiford@arm.com>
> >
> > gcc/
> >       PR inline-asm/52813
> >       * doc/extend.texi: Document that listing the stack pointer in the
> >       clobber list of an asm is a deprecated feature.
> >       * common.opt (Wdeprecated): Moved from c-family/c.opt.
> >       * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated
> >       warning instead of an error for clobbers of the stack pointer.
> >       Add a note explaining why.
> >
> > gcc/c-family/
> >       PR inline-asm/52813
> >       * c.opt (Wdeprecated): Move documentation and variable to common.opt.
> >
> > gcc/d/
> >       PR inline-asm/52813
> >       * lang.opt (Wdeprecated): Reference common.opt instead of c.opt.
> >
> > gcc/testsuite/
> >       PR inline-asm/52813
> >       * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a
> >       -Wdeprecated warning and expect a following note:.
> OK.
>
> FWIW the number of packages affected in Fedora was in single digits,
> some of which have already been fixed.
>
> But if folks want to go with a deprecated warning instead of straight to
> an error, I won't complain.
>
> jeff


Hi,

I originally complained because the arm test for pr77904.c was failing.
Since Richard's change that test emits a warning rather than an error,
but still fails. This small patch adds the missing dg-warning.

OK?

Thanks,

Christophe

[-- Attachment #2: pr77904.chlog.txt --]
[-- Type: text/plain, Size: 120 bytes --]

2019-01-17  Christophe Lyon  <christophe.lyon@linaro.org>

	* gcc.target/arm/pr77904.c: Add dg-warning for sp clobber.


[-- Attachment #3: pr77904.patch.txt --]
[-- Type: text/plain, Size: 470 bytes --]

diff --git a/gcc/testsuite/gcc.target/arm/pr77904.c b/gcc/testsuite/gcc.target/arm/pr77904.c
index 76728c0..f279ec8 100644
--- a/gcc/testsuite/gcc.target/arm/pr77904.c
+++ b/gcc/testsuite/gcc.target/arm/pr77904.c
@@ -4,7 +4,8 @@
 __attribute__ ((noinline, noclone)) void
 clobber_sp (void)
 {
-  __asm volatile ("" : : : "sp");
+  __asm volatile ("" : : : "sp"); /* { dg-warning "listing the stack pointer register 'sp' in a clobber list is deprecated" } */
+
 }
 
 int

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Jeff Law]

On 1/8/19 5:03 AM, Richard Sandiford wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> On 1/7/19 10:23 AM, Jakub Jelinek wrote:
>>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
>>>> -  /* Clobbering the STACK POINTER register is an error.  */
>>>> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
>>>> +     which is often unexpected by users.  See PR52813.  */
>>>>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
>>>>      {
>>>> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
>>>> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
>>>> +	       regname);
>>>> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
>>>
>>> Why do we write Stack Pointer rather than stack pointer?  That is really
>>> weird.  The second warning would be a note based on the first one, i.e.
>>> if (warning ()) note ();
>>> and better have some -W* option to silence the warning.
>>>
>>
>> Yes, thanks for this suggestion.
>>
>> Meanwhile I found out, that the stack clobber has only been ignored up to
>> gcc-5 (at least with lra targets, not really sure about reload targets).
>> From gcc-6 on, with the exception of PR arm/77904 which was a regression due
>> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
>> this works for all targets I tried so far.
>>
>> To me, it starts to look like a rather unique and useful feature, that I would
>> like to keep working.
> 
> Not sure what you mean by "unique".  But forcing a frame is a bit of
> a slippery concept.  Force it where?  For the asm only, or the whole
> function?  This depends on optimisation and hasn't been consistent
> across GCC versions, since it depends on the shrink-wrapping
> optimisation.  (There was a similar controversy a while ago about
> to what extent -fno-omit-frame-pointer should "force a frame".)
> 
> The effect on the redzone seems like something that should be specified
> explicitly rather than as an (accidental?) side effect of listing the
> sp in the clobber list.  Maybe this would be another use for the "asm
> attributes" proposal.  "noreturn" was another attribute suggested on
> IRC yesterday.
> 
> But either way, the general feeling seems to be that going straight to a
> hard error is too harsh, since there's quite a bit of existing code that
> has the clobber.  This patch implements the compromise discussed on IRC
> yesterday of making it a -Wdeprecated warning instead.
> 
> Tested on x86_64-linux-gnu and aarch64-linux-gnu.  OK to install?
> 
> Richard
> 
> Dimitar: sorry the run-around on this patch, and thanks for the
> submission.  It looks from all the controversy like it was a
> long-festering PR for a reason. :-/
> 
> 
> 2019-01-07  Richard Sandiford  <richard.sandiford@arm.com>
> 
> gcc/
> 	PR inline-asm/52813
> 	* doc/extend.texi: Document that listing the stack pointer in the
> 	clobber list of an asm is a deprecated feature.
> 	* common.opt (Wdeprecated): Moved from c-family/c.opt.
> 	* cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated
> 	warning instead of an error for clobbers of the stack pointer.
> 	Add a note explaining why.
> 
> gcc/c-family/
> 	PR inline-asm/52813
> 	* c.opt (Wdeprecated): Move documentation and variable to common.opt.
> 
> gcc/d/
> 	PR inline-asm/52813
> 	* lang.opt (Wdeprecated): Reference common.opt instead of c.opt.
> 
> gcc/testsuite/
> 	PR inline-asm/52813
> 	* gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a
> 	-Wdeprecated warning and expect a following note:.
OK.

FWIW the number of packages affected in Fedora was in single digits,
some of which have already been fixed.

But if folks want to go with a deprecated warning instead of straight to
an error, I won't complain.

jeff

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Thu, Jan 10, 2019 at 09:56:28PM +0000, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Thu, Jan 10, 2019 at 09:23:27PM +0000, Richard Sandiford wrote:
> >> > "noreturn"...  What would that mean, *exactly*?  It cannot execute any
> >> > code the compiler can see, so such asm is better off as real asm anyway
> >> > (not inline asm).
> >> 
> >> "Exactly" is a strong word, and this wasn't my proposal, but...
> >> I think it would act like a noreturn call to an unknown function.
> >> Output operands wouldn't make sense, and arguably clobbers wouldn't
> >> either.
> >
> > "noreturn" asm can be done already now, just use
> > asm volatile ("..." ...);
> > __builtin_unreachable ();
> >
> > I think there is no need to add a new syntax for that.

Ah yes, the volatile makes this work.  Cool.  And as Richard says such
asm shouldn't have outputs anyway, so it will always be volatile.

> ISTR the point was that the PowerPC ABI places requirements on functions
> with noreturn calls

I don't think any of our ABIs do that?  Is this about the back chain?

> and the attribute would help GCC do the right thing
> in those circumstances.  So "noreturn" would imply a call that doesn't
> return, rather than just an infinite loop.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
>> I think it would act like a noreturn call to an unknown function.
>
> Except it won't behave like a call otherwise (on Power all calls force a
> stack frame, for example; and on all targets noreturn calls do the same
> currently I think?)

I agree no current asm would behave like a call (e.g. causing leaf_p
to be false, and more).  The point of adding the attribute would be to
change that.

Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Thu, Jan 10, 2019 at 09:23:27PM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Tue, Jan 08, 2019 at 12:03:06PM +0000, Richard Sandiford wrote:
> >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >> > Meanwhile I found out, that the stack clobber has only been ignored up to
> >> > gcc-5 (at least with lra targets, not really sure about reload targets).
> >> > From gcc-6 on, with the exception of PR arm/77904 which was a regression due
> >> > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
> >> > this works for all targets I tried so far.
> >> >
> >> > To me, it starts to look like a rather unique and useful feature, that I would
> >> > like to keep working.
> >> 
> >> Not sure what you mean by "unique".  But forcing a frame is a bit of
> >> a slippery concept.  Force it where?  For the asm only, or the whole
> >> function?  This depends on optimisation and hasn't been consistent
> >> across GCC versions, since it depends on the shrink-wrapping
> >> optimisation.  (There was a similar controversy a while ago about
> >> to what extent -fno-omit-frame-pointer should "force a frame".)
> >
> > It's not forcing a frame currently: it's just setting frame_pointer_needed.
> > Whatever happens from that is the target's business.
> 
> Do you mean the asm clobber or -fno-omit-frame-pointer?  If the option,
> then yeah, and that was exactly what was controversial :-)

I meant the asm clobber.  LRA sets frame_pointer_needed to 1 because the
stack pointer is clobbered, on many targets anyway:

          /* If we modify the source of an elimination rule, disable
             it.  Do the same if it is the destination and not the
             hard frame register.  */
          for (ep = reg_eliminate;
               ep < &reg_eliminate[NUM_ELIMINABLE_REGS];
               ep++)
            if (ep->from_rtx == XEXP (x, 0)
                || (ep->to_rtx == XEXP (x, 0)
                    && ep->to_rtx != hard_frame_pointer_rtx))
              setup_can_eliminate (ep, false);

and setup_can_eliminate has

  if (! value
      && ep->from == FRAME_POINTER_REGNUM && ep->to == STACK_POINTER_REGNUM)
    frame_pointer_needed = 1;

> >> The effect on the redzone seems like something that should be specified
> >> explicitly rather than as an (accidental?) side effect of listing the
> >> sp in the clobber list.  Maybe this would be another use for the "asm
> >> attributes" proposal.  "noreturn" was another attribute suggested on
> >> IRC yesterday.
> >
> > Redzone is target-dependent.
> 
> Right.  Target-dependent asm attributes wouldn't be a problem though.

It's harder to document, which means it is harder to get right (and get
people to _use_ it correctly), as well.

> Most other things about an asm are target-dependent anyway.

Very true.

> > "noreturn"...  What would that mean, *exactly*?  It cannot execute any
> > code the compiler can see, so such asm is better off as real asm anyway
> > (not inline asm).
> 
> "Exactly" is a strong word, and this wasn't my proposal, but...

"Exactly", as in, "please do enough hand-waving to cover all available
space" ;-)

There are many details that aren't quite obvious, but they do matter for
how usable and useful this extension would be.

> I think it would act like a noreturn call to an unknown function.

Except it won't behave like a call otherwise (on Power all calls force a
stack frame, for example; and on all targets noreturn calls do the same
currently I think?)

> Output operands wouldn't make sense, and arguably clobbers wouldn't
> either.

Yeah.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

On 1/10/19 10:23 PM, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Tue, Jan 08, 2019 at 12:03:06PM +0000, Richard Sandiford wrote:
>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>> Meanwhile I found out, that the stack clobber has only been ignored up to
>>>> gcc-5 (at least with lra targets, not really sure about reload targets).
>>>> From gcc-6 on, with the exception of PR arm/77904 which was a regression due
>>>> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
>>>> this works for all targets I tried so far.
>>>>
>>>> To me, it starts to look like a rather unique and useful feature, that I would
>>>> like to keep working.
>>>
>>> Not sure what you mean by "unique".  But forcing a frame is a bit of
>>> a slippery concept.  Force it where?  For the asm only, or the whole
>>> function?  This depends on optimisation and hasn't been consistent
>>> across GCC versions, since it depends on the shrink-wrapping
>>> optimisation.  (There was a similar controversy a while ago about
>>> to what extent -fno-omit-frame-pointer should "force a frame".)
>>
>> It's not forcing a frame currently: it's just setting frame_pointer_needed.
>> Whatever happens from that is the target's business.
> 
> Do you mean the asm clobber or -fno-omit-frame-pointer?  If the option,
> then yeah, and that was exactly what was controversial :-)
> 

Yes, what I meant is the asm clobber sets frame_pointer_needed,
on the function where this asm is used, that sounded to me like
it would have an impact on the frame pointer.

What I also expected, is that if an asm is accessing a local
via "m" then the a SP+x reference will be elimitated to a FP+x,
reference, which would allow the asm to push something on the
stack, and the memory references would remain valid,
as long as the stack is _restored_, again in the same asm.
I mean in case of register shortage.  I was not thinking about
noreturn at all.

But if -fno-omit-frame-pointer does the same, and that is not sufficient
to for forcing a frame pointer, because it is a target dependent, then I
wonder how ASAN is supposed to work on such a target.

But anyway I guess, your patch is fine.


Thanks
Bernd. 

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Thu, Jan 10, 2019 at 09:23:27PM +0000, Richard Sandiford wrote:
>> > "noreturn"...  What would that mean, *exactly*?  It cannot execute any
>> > code the compiler can see, so such asm is better off as real asm anyway
>> > (not inline asm).
>> 
>> "Exactly" is a strong word, and this wasn't my proposal, but...
>> I think it would act like a noreturn call to an unknown function.
>> Output operands wouldn't make sense, and arguably clobbers wouldn't
>> either.
>
> "noreturn" asm can be done already now, just use
> asm volatile ("..." ...);
> __builtin_unreachable ();
>
> I think there is no need to add a new syntax for that.

ISTR the point was that the PowerPC ABI places requirements on functions
with noreturn calls and the attribute would help GCC do the right thing
in those circumstances.  So "noreturn" would imply a call that doesn't
return, rather than just an infinite loop.

Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Jakub Jelinek]

On Thu, Jan 10, 2019 at 09:23:27PM +0000, Richard Sandiford wrote:
> > "noreturn"...  What would that mean, *exactly*?  It cannot execute any
> > code the compiler can see, so such asm is better off as real asm anyway
> > (not inline asm).
> 
> "Exactly" is a strong word, and this wasn't my proposal, but...
> I think it would act like a noreturn call to an unknown function.
> Output operands wouldn't make sense, and arguably clobbers wouldn't
> either.

"noreturn" asm can be done already now, just use
asm volatile ("..." ...);
__builtin_unreachable ();

I think there is no need to add a new syntax for that.

	Jakub

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Jan 08, 2019 at 12:03:06PM +0000, Richard Sandiford wrote:
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> > Meanwhile I found out, that the stack clobber has only been ignored up to
>> > gcc-5 (at least with lra targets, not really sure about reload targets).
>> > From gcc-6 on, with the exception of PR arm/77904 which was a regression due
>> > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
>> > this works for all targets I tried so far.
>> >
>> > To me, it starts to look like a rather unique and useful feature, that I would
>> > like to keep working.
>> 
>> Not sure what you mean by "unique".  But forcing a frame is a bit of
>> a slippery concept.  Force it where?  For the asm only, or the whole
>> function?  This depends on optimisation and hasn't been consistent
>> across GCC versions, since it depends on the shrink-wrapping
>> optimisation.  (There was a similar controversy a while ago about
>> to what extent -fno-omit-frame-pointer should "force a frame".)
>
> It's not forcing a frame currently: it's just setting frame_pointer_needed.
> Whatever happens from that is the target's business.

Do you mean the asm clobber or -fno-omit-frame-pointer?  If the option,
then yeah, and that was exactly what was controversial :-)

>> The effect on the redzone seems like something that should be specified
>> explicitly rather than as an (accidental?) side effect of listing the
>> sp in the clobber list.  Maybe this would be another use for the "asm
>> attributes" proposal.  "noreturn" was another attribute suggested on
>> IRC yesterday.
>
> Redzone is target-dependent.

Right.  Target-dependent asm attributes wouldn't be a problem though.
Most other things about an asm are target-dependent anyway.

> "noreturn"...  What would that mean, *exactly*?  It cannot execute any
> code the compiler can see, so such asm is better off as real asm anyway
> (not inline asm).

"Exactly" is a strong word, and this wasn't my proposal, but...
I think it would act like a noreturn call to an unknown function.
Output operands wouldn't make sense, and arguably clobbers wouldn't
either.

Thanks,
Richard

>> But either way, the general feeling seems to be that going straight to a
>> hard error is too harsh, since there's quite a bit of existing code that
>> has the clobber.  This patch implements the compromise discussed on IRC
>> yesterday of making it a -Wdeprecated warning instead.
>
> The patch looks fine to me.  Thanks!
>
>
> Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

Hi!

On Tue, Jan 08, 2019 at 12:03:06PM +0000, Richard Sandiford wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> > Meanwhile I found out, that the stack clobber has only been ignored up to
> > gcc-5 (at least with lra targets, not really sure about reload targets).
> > From gcc-6 on, with the exception of PR arm/77904 which was a regression due
> > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
> > this works for all targets I tried so far.
> >
> > To me, it starts to look like a rather unique and useful feature, that I would
> > like to keep working.
> 
> Not sure what you mean by "unique".  But forcing a frame is a bit of
> a slippery concept.  Force it where?  For the asm only, or the whole
> function?  This depends on optimisation and hasn't been consistent
> across GCC versions, since it depends on the shrink-wrapping
> optimisation.  (There was a similar controversy a while ago about
> to what extent -fno-omit-frame-pointer should "force a frame".)

It's not forcing a frame currently: it's just setting frame_pointer_needed.
Whatever happens from that is the target's business.

> The effect on the redzone seems like something that should be specified
> explicitly rather than as an (accidental?) side effect of listing the
> sp in the clobber list.  Maybe this would be another use for the "asm
> attributes" proposal.  "noreturn" was another attribute suggested on
> IRC yesterday.

Redzone is target-dependent.

"noreturn"...  What would that mean, *exactly*?  It cannot execute any
code the compiler can see, so such asm is better off as real asm anyway
(not inline asm).

> But either way, the general feeling seems to be that going straight to a
> hard error is too harsh, since there's quite a bit of existing code that
> has the clobber.  This patch implements the compromise discussed on IRC
> yesterday of making it a -Wdeprecated warning instead.

The patch looks fine to me.  Thanks!


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> On 1/7/19 10:23 AM, Jakub Jelinek wrote:
>> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
>>> -  /* Clobbering the STACK POINTER register is an error.  */
>>> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
>>> +     which is often unexpected by users.  See PR52813.  */
>>>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
>>>      {
>>> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
>>> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
>>> +	       regname);
>>> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
>> 
>> Why do we write Stack Pointer rather than stack pointer?  That is really
>> weird.  The second warning would be a note based on the first one, i.e.
>> if (warning ()) note ();
>> and better have some -W* option to silence the warning.
>> 
>
> Yes, thanks for this suggestion.
>
> Meanwhile I found out, that the stack clobber has only been ignored up to
> gcc-5 (at least with lra targets, not really sure about reload targets).
> From gcc-6 on, with the exception of PR arm/77904 which was a regression due
> to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
> this works for all targets I tried so far.
>
> To me, it starts to look like a rather unique and useful feature, that I would
> like to keep working.

Not sure what you mean by "unique".  But forcing a frame is a bit of
a slippery concept.  Force it where?  For the asm only, or the whole
function?  This depends on optimisation and hasn't been consistent
across GCC versions, since it depends on the shrink-wrapping
optimisation.  (There was a similar controversy a while ago about
to what extent -fno-omit-frame-pointer should "force a frame".)

The effect on the redzone seems like something that should be specified
explicitly rather than as an (accidental?) side effect of listing the
sp in the clobber list.  Maybe this would be another use for the "asm
attributes" proposal.  "noreturn" was another attribute suggested on
IRC yesterday.

But either way, the general feeling seems to be that going straight to a
hard error is too harsh, since there's quite a bit of existing code that
has the clobber.  This patch implements the compromise discussed on IRC
yesterday of making it a -Wdeprecated warning instead.

Tested on x86_64-linux-gnu and aarch64-linux-gnu.  OK to install?

Richard

Dimitar: sorry the run-around on this patch, and thanks for the
submission.  It looks from all the controversy like it was a
long-festering PR for a reason. :-/


2019-01-07  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	PR inline-asm/52813
	* doc/extend.texi: Document that listing the stack pointer in the
	clobber list of an asm is a deprecated feature.
	* common.opt (Wdeprecated): Moved from c-family/c.opt.
	* cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated
	warning instead of an error for clobbers of the stack pointer.
	Add a note explaining why.

gcc/c-family/
	PR inline-asm/52813
	* c.opt (Wdeprecated): Move documentation and variable to common.opt.

gcc/d/
	PR inline-asm/52813
	* lang.opt (Wdeprecated): Reference common.opt instead of c.opt.

gcc/testsuite/
	PR inline-asm/52813
	* gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a
	-Wdeprecated warning and expect a following note:.

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	2019-01-07 12:14:31.699490485 +0000
+++ gcc/doc/extend.texi	2019-01-08 11:40:20.807906878 +0000
@@ -9441,6 +9441,15 @@ When the compiler selects which register
 operands, it does not use any of the clobbered registers. As a result, 
 clobbered registers are available for any use in the assembler code.
 
+Another restriction is that the clobber list should not contain the
+stack pointer register.  This is because the compiler requires the
+value of the stack pointer to be the same after an @code{asm}
+statement as it was on entry to the statement.  However, previous
+versions of GCC did not enforce this rule and allowed the stack
+pointer to appear in the list, with unclear semantics.  This behavior
+is deprecated and listing the stack pointer may become an error in
+future versions of GCC@.
+
 Here is a realistic example for the VAX showing the use of clobbered 
 registers: 
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	2019-01-04 11:39:27.178246751 +0000
+++ gcc/common.opt	2019-01-08 11:40:20.803906912 +0000
@@ -579,6 +579,10 @@ Wattribute-warning
 Common Var(warn_attribute_warning) Init(1) Warning
 Warn about uses of __attribute__((warning)) declarations.
 
+Wdeprecated
+Common Var(warn_deprecated) Init(1) Warning
+Warn if a deprecated compiler feature, class, method, or field is used.
+
 Wdeprecated-declarations
 Common Var(warn_deprecated_decl) Init(1) Warning
 Warn about uses of __attribute__((deprecated)) declarations.
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	2019-01-07 12:14:32.031487693 +0000
+++ gcc/cfgexpand.c	2019-01-08 11:40:20.803906912 +0000
@@ -2872,12 +2872,16 @@ asm_clobber_reg_is_valid (int regno, int
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
-  if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
-    {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
-    }
+  /* Clobbering the stack pointer register is deprecated.  GCC expects
+     the value of the stack pointer after an asm statement to be the same
+     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");
 
   return is_valid;
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	2019-01-04 11:39:24.538269283 +0000
+++ gcc/c-family/c.opt	2019-01-08 11:40:20.799906946 +0000
@@ -477,8 +477,8 @@ C++ ObjC++ Var(warn_delnonvdtor) Warning
 Warn about deleting polymorphic objects with non-virtual destructors.
 
 Wdeprecated
-C C++ ObjC ObjC++ CPP(cpp_warn_deprecated) CppReason(CPP_W_DEPRECATED) Var(warn_deprecated) Init(1) Warning
-Warn if a deprecated compiler feature, class, method, or field is used.
+C C++ ObjC ObjC++ CPP(cpp_warn_deprecated) CppReason(CPP_W_DEPRECATED)
+; Documented in common.opt
 
 Wdeprecated-copy
 C++ ObjC++ Var(warn_deprecated_copy) Warning LangEnabledBy(C++ ObjC++, Wextra)
Index: gcc/d/lang.opt
===================================================================
--- gcc/d/lang.opt	2019-01-04 11:39:24.702267882 +0000
+++ gcc/d/lang.opt	2019-01-08 11:40:20.803906912 +0000
@@ -124,7 +124,7 @@ Warn about casts that will produce a nul
 
 Wdeprecated
 D
-; Documented in C
+; Documented in common.opt
 
 Werror
 D
Index: gcc/testsuite/gcc.target/i386/pr52813.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr52813.c	2018-12-11 15:50:44.668823294 +0000
+++ gcc/testsuite/gcc.target/i386/pr52813.c	2019-01-08 11:40:20.807906878 +0000
@@ -5,5 +5,6 @@
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%esp"); /* { dg-warning "listing the stack pointer register '%esp' in a clobber list is deprecated" } */
+  /* { dg-message "note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement" "" { target *-*-* } .-1 } */
 }

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

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

On 1/7/19 10:23 AM, Jakub Jelinek wrote:
> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
>> -  /* Clobbering the STACK POINTER register is an error.  */
>> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
>> +     which is often unexpected by users.  See PR52813.  */
>>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
>>      {
>> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
>> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
>> +	       regname);
>> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
> 
> Why do we write Stack Pointer rather than stack pointer?  That is really
> weird.  The second warning would be a note based on the first one, i.e.
> if (warning ()) note ();
> and better have some -W* option to silence the warning.
> 

Yes, thanks for this suggestion.

Meanwhile I found out, that the stack clobber has only been ignored up to
gcc-5 (at least with lra targets, not really sure about reload targets).
From gcc-6 on, with the exception of PR arm/77904 which was a regression due
to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0,
this works for all targets I tried so far.

To me, it starts to look like a rather unique and useful feature, that I would
like to keep working.


Attached is an updated version if my patch, using the suggested warning option,
and a note with the details.


Bootstrapped on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr52813.diff --]
[-- Type: text/x-patch; name="patch-pr52813.diff", Size: 5703 bytes --]

2018-01-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wstack-clobber.
	* common.opt (-Wstack-clobber): New default-enabled warning.
	* cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together
	with an informative note when the stack pointer is clobbered.

testsuite:
2018-07-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.target/arm/pr77904.c: Adjust test.
	* gcc.target/i386/pr52813.c: Adjust test.


Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 267653)
+++ gcc/cfgexpand.c	(working copy)
@@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S
    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)
 {
@@ -2872,11 +2873,22 @@ asm_clobber_reg_is_valid (int regno, int nregs, co
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
+  /* Clobbering the STACK POINTER register is likely an error.
+     However it is useful to force the use of frame pointer and prevent
+     the use of red zone.  Thus without this clobber, pushing temporary
+     values onto the stack might clobber the red zone or make stack based
+     memory references invalid.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
+      if (warning (OPT_Wstack_clobber,
+		   "stack pointer register clobbered by %qs in %<asm%>",
+		   regname))
+	inform (input_location,
+		"This does likely not do what you would expect."
+		" The stack pointer register still has to be restored to"
+		" the previous value, however it is safe to push values onto"
+		" the stack, when they are popped again from the stack"
+		" before the asm statement terminates");
     }
 
   return is_valid;
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 267653)
+++ gcc/common.opt	(working copy)
@@ -702,6 +702,10 @@ Warn when one local variable shadows another local
 Wshadow-compatible-local
 Common Warning Undocumented Alias(Wshadow=compatible-local)
 
+Wstack-clobber
+Common Warning Var(warn_stack_clobber) Init(1)
+Warn when asm statements try to clobber the stack pointer register.
+
 Wstack-protector
 Common Var(warn_stack_protect) Warning
 Warn when not issuing stack smashing protection for some reason.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 267653)
+++ gcc/doc/invoke.texi	(working copy)
@@ -339,7 +339,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wshift-count-negative  -Wshift-count-overflow  -Wshift-negative-value @gol
 -Wsign-compare  -Wsign-conversion  -Wfloat-conversion @gol
 -Wno-scalar-storage-order  -Wsizeof-pointer-div @gol
--Wsizeof-pointer-memaccess  -Wsizeof-array-argument @gol
+-Wsizeof-pointer-memaccess  -Wsizeof-array-argument  -Wstack-clobber @gol
 -Wstack-protector  -Wstack-usage=@var{byte-size}  -Wstrict-aliasing @gol
 -Wstrict-aliasing=n  -Wstrict-overflow  -Wstrict-overflow=@var{n} @gol
 -Wstringop-overflow=@var{n}  -Wstringop-truncation  -Wsubobject-linkage @gol
@@ -7560,6 +7560,20 @@ This option is only supported for C and Objective-
 @option{-Wall} and by @option{-Wpedantic}, which can be disabled with
 @option{-Wno-pointer-sign}.
 
+@item -Wstack-clobber
+@opindex Wstack-clobber
+@opindex Wno-stack-clobber
+Warn for asm statements that try to clobber the stack pointer.
+Prior to gcc-6 this clobber was ignored, but from gcc-6 on
+this was changed to force the use of the frame pointer in the
+current function even if @option{-fomit-frame-pointer} is in
+effect, additionally this clobber prevents the use of a red-zone
+on some targets.  The stack pointer register still has to be
+restored to the previous value, however it is safe to push values
+onto the stack, when they are popped again from the stack before
+the asm statement terminates.  Since this might be unexpected,
+the warning is enabled by default.
+
 @item -Wstack-protector
 @opindex Wstack-protector
 @opindex Wno-stack-protector
Index: gcc/testsuite/gcc.target/arm/pr77904.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr77904.c	(revision 267653)
+++ gcc/testsuite/gcc.target/arm/pr77904.c	(working copy)
@@ -4,7 +4,7 @@
 __attribute__ ((noinline, noclone)) void
 clobber_sp (void)
 {
-  __asm volatile ("" : : : "sp");
+  __asm volatile ("" : : : "sp"); /* { dg-warning "stack pointer register clobbered" } */
 }
 
 int
Index: gcc/testsuite/gcc.target/i386/pr52813.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr52813.c	(revision 267653)
+++ gcc/testsuite/gcc.target/i386/pr52813.c	(working copy)
@@ -1,9 +1,10 @@
 /* Ensure that stack pointer cannot be an asm clobber.  */
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O3 -fomit-frame-pointer" } */
 
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%rsp"); /* { dg-warning "stack pointer register clobbered" } */
 }
+/* { dg-final { scan-assembler "(?n)pushq.*%rbp" } } */

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Jakub Jelinek]

On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote:
> -  /* Clobbering the STACK POINTER register is an error.  */
> +  /* Clobbered STACK POINTER register is not saved/restored by GCC,
> +     which is often unexpected by users.  See PR52813.  */
>    if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
>      {
> -      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
> +      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
> +	       regname);
> +      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");

Why do we write Stack Pointer rather than stack pointer?  That is really
weird.  The second warning would be a note based on the first one, i.e.
if (warning ()) note ();
and better have some -W* option to silence the warning.

>        is_valid = false;
>      }
>  
> diff --git a/gcc/testsuite/gcc.target/i386/pr52813.c b/gcc/testsuite/gcc.target/i386/pr52813.c
> index 154ebbfc423..644fef15fef 100644
> --- a/gcc/testsuite/gcc.target/i386/pr52813.c
> +++ b/gcc/testsuite/gcc.target/i386/pr52813.c
> @@ -5,5 +5,5 @@
>  void
>  test1 (void)
>  {
> -  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
> +  asm volatile ("" : : : "%esp"); /* { dg-warning "Stack Pointer register clobbered.\+GCC has always ignored Stack Pointer 'asm' clobbers" } */
>  }
> -- 
> 2.11.0
> 


	Jakub

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Wed, Dec 19, 2018 at 08:40:13AM +0200, Dimitar Dimitrov wrote:
> On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote:
> > out of curiosity I looked at the clobber statement in
> > gdb/nat/linux-ptrace.c:
> > 
> >            asm volatile ("pushq %0;"
> >                          ".globl linux_ptrace_test_ret_to_nx_instr;"
> >                          "linux_ptrace_test_ret_to_nx_instr:"
> >                          "ret"
> >                          : : "r" ((uint64_t) (uintptr_t) return_address)
> >                          : "%rsp", "memory");
> > 
> > it turns out to be a far jump, instruction.
> 
> GDB functionality should not be affected if SP clobber is removed, even if the 
> generated code is slightly different. Please see this comment:
> http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html
> 
> As I understand it, this particular code is never meant to return. It should 
> either stop due to the NX mapping of return_address/%0, or hit the breakpoint 
> placed at return_address/%0.

If it doesn't return it is undefined behaviour, so anything might happen
and that is perfectly alright.

Defining labels is an asm is undefined, too.

Maybe real assembler code is wanted here?  I.e. a .s file.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

On Mon, Dec 17 2018 20:15:02 EET Bernd Edlinger wrote:
> out of curiosity I looked at the clobber statement in
> gdb/nat/linux-ptrace.c:
> 
>            asm volatile ("pushq %0;"
>                          ".globl linux_ptrace_test_ret_to_nx_instr;"
>                          "linux_ptrace_test_ret_to_nx_instr:"
>                          "ret"
>                          : : "r" ((uint64_t) (uintptr_t) return_address)
>                          : "%rsp", "memory");
> 
> it turns out to be a far jump, instruction.

GDB functionality should not be affected if SP clobber is removed, even if the 
generated code is slightly different. Please see this comment:
http://sourceware.org/ml/gdb-patches/2018-12/msg00204.html

As I understand it, this particular code is never meant to return. It should 
either stop due to the NX mapping of return_address/%0, or hit the breakpoint 
placed at return_address/%0.

Regards,
Dimitar

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

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

On 12/18/18 3:16 PM, Bernd Edlinger wrote:
> Hi,
> 
> while I looked closely at the asm statement in the gdb,
> I realized that the SP clobber forces the function to use
> the frame pointer, and prevents the red zone.  That
> makes the push / pop sequence in the asm statement safe
> to use, as long as the stack is restored to the original
> value.  That can be a quite useful feature.  And that might
> have been the reason why the rsp clobber was chosen in the
> first place.
> 
> This seems to work for all targets, but it started to work
> this way with gcc-6, all versions before that do ignore
> this clobber stmt (as confirmed by godbolt).
> 
> The clobber stmt make the LRA register allocator switch
> frame_pointer_needed to 1, and therefore in all likelihood,
> all targets should use that consistently.
> 
> On 12/17/18 12:47 PM, Richard Sandiford wrote:
>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> if I understood that right, then clobbering sp is and has always been
>>>> ignored.
>>
>> PR77904 was about the clobber not being ignored, so the behaviour
>> hasn't been consistent.
>>
> 
> I think 77904 was a fall-out from the change in the LRA register allocator.
> The patch referenced in the PR does simply honor frame_pointer_needed,
> which changed with gcc-6, and caused a regression on arm.
> 
>> I'm also not sure it was always ignored in recent sources.  The clobber
>> does get added to the associated rtl insn, and it'd be surprising if
>> that never had an effect.
>>
>>>> If that is right, then I would much prefer a warning, that says exactly
>>>> that, because that would also help to understand why removing that clobber
>>>> statement is safe even for old gcc versions.
>>
>> If the asm does leave sp with a different value, then it's never been safe,
>> regardless of the gcc version.  That's why an error seems more appropriate.
>>
>>> Thank you. Looks like general consensus is to have a warning. See attached
>>> patch that switches the error to a warning.
>>
>> I don't think there's a good reason to treat this differently from the
>> preexisting PIC register error.  If the argument for making it a warning
>> rather than an error is that the asm might happen to work by accident,
>> then the same is true for the PIC register.
>>
> 
> In the light of my findings, I believe with a good warning message that
> explains that the SP needs to be restored to the previous value, that
> is a useful feature, that enables the asm statement to push temporary
> values on the stack which would not be safe otherwise.
> 
> Therefore I propose not to rip it out at this time.
> See my proposed patch.  What do you think?
> 
> Is it OK?
> 
> 

Oops, previous version missed the fix of the PR77904 test case, which is
currently broken too.


Bernd-

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr52813.diff --]
[-- Type: text/x-patch; name="patch-pr52813.diff", Size: 3231 bytes --]

2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together
	with an information message when the stack pointer is clobbered.

testsuite:
2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.target/arm/pr77904.c: Adjust test.
	* gcc.target/i386/pr52813.c: Adjust test.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 267164)
+++ gcc/cfgexpand.c	(working copy)
@@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S
    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)
 {
@@ -2872,11 +2873,23 @@ asm_clobber_reg_is_valid (int regno, int nregs, co
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
+  /* Clobbering the STACK POINTER register is likely an error.
+     However it is useful to force the use of frame pointer and prevent
+     the use of red zone.  Thus without this clobber, pushing temporary
+     values onto the stack might clobber the red zone or make stack based
+     memory references invalid.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
+      if (warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
+		   regname))
+	{
+	  inform (input_location,
+		  "This does likely not do what you would expect."
+		  " The Stack Pointer register still needs to be restored to"
+		  " the previous value, however it is safe to push values onto"
+		  " the stack, when they are popped again from the stack"
+		  " before the asm statement terminates");
+	}
     }
 
   return is_valid;
Index: gcc/testsuite/gcc.target/arm/pr77904.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr77904.c	(revision 267164)
+++ gcc/testsuite/gcc.target/arm/pr77904.c	(working copy)
@@ -4,7 +4,7 @@
 __attribute__ ((noinline, noclone)) void
 clobber_sp (void)
 {
-  __asm volatile ("" : : : "sp");
+  __asm volatile ("" : : : "sp"); /* { dg-warning "Stack Pointer register clobbered" } */
 }
 
 int
Index: gcc/testsuite/gcc.target/i386/pr52813.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr52813.c	(revision 267164)
+++ gcc/testsuite/gcc.target/i386/pr52813.c	(working copy)
@@ -1,9 +1,10 @@
 /* Ensure that stack pointer cannot be an asm clobber.  */
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O3 -fomit-frame-pointer" } */
 
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%rsp"); /* { dg-warning "Stack Pointer register clobbered" } */
 }
+/* { dg-final { scan-assembler "(?n)pushq.*%rbp" } } */

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

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

Hi,

while I looked closely at the asm statement in the gdb,
I realized that the SP clobber forces the function to use
the frame pointer, and prevents the red zone.  That
makes the push / pop sequence in the asm statement safe
to use, as long as the stack is restored to the original
value.  That can be a quite useful feature.  And that might
have been the reason why the rsp clobber was chosen in the
first place.

This seems to work for all targets, but it started to work
this way with gcc-6, all versions before that do ignore
this clobber stmt (as confirmed by godbolt).

The clobber stmt make the LRA register allocator switch
frame_pointer_needed to 1, and therefore in all likelihood,
all targets should use that consistently.

On 12/17/18 12:47 PM, Richard Sandiford wrote:
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>> Hi,
>>>
>>> if I understood that right, then clobbering sp is and has always been
>>> ignored.
> 
> PR77904 was about the clobber not being ignored, so the behaviour
> hasn't been consistent.
> 

I think 77904 was a fall-out from the change in the LRA register allocator.
The patch referenced in the PR does simply honor frame_pointer_needed,
which changed with gcc-6, and caused a regression on arm.

> I'm also not sure it was always ignored in recent sources.  The clobber
> does get added to the associated rtl insn, and it'd be surprising if
> that never had an effect.
> 
>>> If that is right, then I would much prefer a warning, that says exactly
>>> that, because that would also help to understand why removing that clobber
>>> statement is safe even for old gcc versions.
> 
> If the asm does leave sp with a different value, then it's never been safe,
> regardless of the gcc version.  That's why an error seems more appropriate.
> 
>> Thank you. Looks like general consensus is to have a warning. See attached
>> patch that switches the error to a warning.
> 
> I don't think there's a good reason to treat this differently from the
> preexisting PIC register error.  If the argument for making it a warning
> rather than an error is that the asm might happen to work by accident,
> then the same is true for the PIC register.
> 

In the light of my findings, I believe with a good warning message that
explains that the SP needs to be restored to the previous value, that
is a useful feature, that enables the asm statement to push temporary
values on the stack which would not be safe otherwise.

Therefore I propose not to rip it out at this time.
See my proposed patch.  What do you think?

Is it OK?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr52813.diff --]
[-- Type: text/x-patch; name="patch-pr52813.diff", Size: 2726 bytes --]

2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together
	with an information message when the stack pointer is clobbered.

testsuite:
2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.target/i386/pr52813.c: Adjust test.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 267164)
+++ gcc/cfgexpand.c	(working copy)
@@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S
    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)
 {
@@ -2872,11 +2873,23 @@ asm_clobber_reg_is_valid (int regno, int nregs, co
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
+  /* Clobbering the STACK POINTER register is likely an error.
+     However it is useful to force the use of frame pointer and prevent
+     the use of red zone.  Thus without this clobber, pushing temporary
+     values onto the stack might clobber the red zone or make stack based
+     memory references invalid.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
+      if (warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
+		   regname))
+	{
+	  inform (input_location,
+		  "This does likely not do what you would expect."
+		  " The Stack Pointer register still needs to be restored to"
+		  " the previous value, however it is safe to push values onto"
+		  " the stack, when they are popped again from the stack"
+		  " before the asm statement terminates");
+	}
     }
 
   return is_valid;
Index: gcc/testsuite/gcc.target/i386/pr52813.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr52813.c	(revision 267164)
+++ gcc/testsuite/gcc.target/i386/pr52813.c	(working copy)
@@ -1,9 +1,10 @@
 /* Ensure that stack pointer cannot be an asm clobber.  */
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O3 -fomit-frame-pointer" } */
 
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%rsp"); /* { dg-warning "Stack Pointer register clobbered" } */
 }
+/* { dg-final { scan-assembler "(?n)pushq.*%rbp" } } */

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

On 12/17/18 7:46 PM, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> On Mon, Dec 17, 2018 at 11:47:42AM +0000, Richard Sandiford wrote:
>>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>>>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> if I understood that right, then clobbering sp is and has always been
>>>>> ignored.
>>>
>>> PR77904 was about the clobber not being ignored, so the behaviour
>>> hasn't been consistent.
>>>
>>> I'm also not sure it was always ignored in recent sources.  The clobber
>>> does get added to the associated rtl insn, and it'd be surprising if
>>> that never had an effect.
>>
>> Yes, you will usually get a frame pointer.  My point was that the epilogue
>> will restore your stack pointer both with and without the asm clobber.
> 
> I'm not confident that's the only effect though.
> 
> Also, we didn't use a frame in PR77904, and using a frame would have
> been the wrong thing to do.
> 
>>> I don't think there's a good reason to treat this differently from the
>>> preexisting PIC register error.  If the argument for making it a warning
>>> rather than an error is that the asm might happen to work by accident,
>>> then the same is true for the PIC register.
>>
>> Yes.  As well as quite a few more registers, many of those specific to
>> the target.  And there are many more things you can do terribly wrong in
>> inline assembler, of course, most of which we can never detect.
> 
> Right.  And I don't think anyone's suggesting GCC can detect everything.
> It can only police the things it knows about, which include the input,
> output and clobber clauses.
> 
> What makes the PIC register and sp worth special attention is that
> changing their values would in general invalidate other code that GCC
> generates itself.  It's not just about whether the asm has the effect
> the author wanted (whatever that was).
> 
> FWIW, I don't think we should go on a proactive hunt for other registers
> to complain about.
> 

out of curiosity I looked at the clobber statement in gdb/nat/linux-ptrace.c:

           asm volatile ("pushq %0;"
                         ".globl linux_ptrace_test_ret_to_nx_instr;"
                         "linux_ptrace_test_ret_to_nx_instr:"
                         "ret"
                         : : "r" ((uint64_t) (uintptr_t) return_address)
                         : "%rsp", "memory");

it turns out to be a far jump, instruction.  And I wanted to find out what
removing the %rsp clobber actually does.  First there is a technical problem
with that, because gcc does not print an error, it is possbile to compile the
code without the sp clobber, but not to compare the code that would have been
generated if the error would only be a warning.  So I had to undo the patch
in order to see, what the sp clobber actually does, and I think Segher
mentioned that this might have an influence on the frame pointer, that turns
out to be true:

diff  linux-ptrace-spclobber.dis  linux-ptrace-noclobber.dis

449,590c449,593
<  5c0: 55                      push   %rbp
<  5c1: 45 31 c9                xor    %r9d,%r9d
<  5c4: 41 b8 ff ff ff ff       mov    $0xffffffff,%r8d
<  5ca: b9 22 00 00 00          mov    $0x22,%ecx
<  5cf: ba 03 00 00 00          mov    $0x3,%edx
<  5d4: be 02 00 00 00          mov    $0x2,%esi
<  5d9: 31 ff                   xor    %edi,%edi
<  5db: 48 89 e5                mov    %rsp,%rbp
<  5de: 41 57                   push   %r15
<  5e0: 41 56                   push   %r14
<  5e2: 41 55                   push   %r13
<  5e4: 41 54                   push   %r12
<  5e6: 53                      push   %rbx
<  5e7: 48 81 ec f8 00 00 00    sub    $0xf8,%rsp
[snip]
---
>  5c0: 41 56                   push   %r14
>  5c2: 45 31 c9                xor    %r9d,%r9d
>  5c5: 41 b8 ff ff ff ff       mov    $0xffffffff,%r8d
>  5cb: b9 22 00 00 00          mov    $0x22,%ecx
>  5d0: 41 55                   push   %r13
>  5d2: ba 03 00 00 00          mov    $0x3,%edx
>  5d7: be 02 00 00 00          mov    $0x2,%esi
>  5dc: 31 ff                   xor    %edi,%edi
>  5de: 41 54                   push   %r12
>  5e0: 55                      push   %rbp
>  5e1: 53                      push   %rbx
>  5e2: 48 81 ec f0 00 00 00    sub    $0xf0,%rsp


So for me this looks not at all trivial to see if this
would work without the sp clobber, since the stack frame
might be completely different without that sp clobber.

I wonder what gdb developers think about the sp clobber
here, if it is easy to fix or if it gives trouble to them.


Thanks
Bernd.

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Dec 17, 2018 at 11:47:42AM +0000, Richard Sandiford wrote:
>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>> >> Hi,
>> >> 
>> >> if I understood that right, then clobbering sp is and has always been
>> >> ignored.
>> 
>> PR77904 was about the clobber not being ignored, so the behaviour
>> hasn't been consistent.
>> 
>> I'm also not sure it was always ignored in recent sources.  The clobber
>> does get added to the associated rtl insn, and it'd be surprising if
>> that never had an effect.
>
> Yes, you will usually get a frame pointer.  My point was that the epilogue
> will restore your stack pointer both with and without the asm clobber.

I'm not confident that's the only effect though.

Also, we didn't use a frame in PR77904, and using a frame would have
been the wrong thing to do.

>> I don't think there's a good reason to treat this differently from the
>> preexisting PIC register error.  If the argument for making it a warning
>> rather than an error is that the asm might happen to work by accident,
>> then the same is true for the PIC register.
>
> Yes.  As well as quite a few more registers, many of those specific to
> the target.  And there are many more things you can do terribly wrong in
> inline assembler, of course, most of which we can never detect.

Right.  And I don't think anyone's suggesting GCC can detect everything.
It can only police the things it knows about, which include the input,
output and clobber clauses.

What makes the PIC register and sp worth special attention is that
changing their values would in general invalidate other code that GCC
generates itself.  It's not just about whether the asm has the effect
the author wanted (whatever that was).

FWIW, I don't think we should go on a proactive hunt for other registers
to complain about.

Thanks,
Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool]

On Mon, Dec 17, 2018 at 11:47:42AM +0000, Richard Sandiford wrote:
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
> >> Hi,
> >> 
> >> if I understood that right, then clobbering sp is and has always been
> >> ignored.
> 
> PR77904 was about the clobber not being ignored, so the behaviour
> hasn't been consistent.
> 
> I'm also not sure it was always ignored in recent sources.  The clobber
> does get added to the associated rtl insn, and it'd be surprising if
> that never had an effect.

Yes, you will usually get a frame pointer.  My point was that the epilogue
will restore your stack pointer both with and without the asm clobber.

> I don't think there's a good reason to treat this differently from the
> preexisting PIC register error.  If the argument for making it a warning
> rather than an error is that the asm might happen to work by accident,
> then the same is true for the PIC register.

Yes.  As well as quite a few more registers, many of those specific to
the target.  And there are many more things you can do terribly wrong in
inline assembler, of course, most of which we can never detect.


Segher

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

On 12/17/18 2:35 PM, Richard Sandiford wrote:
> Christophe Lyon <christophe.lyon@linaro.org> writes:
>> On Mon, 17 Dec 2018 at 12:47, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>>
>>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>>>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>>>> Hi,
>>>>>
>>>>> if I understood that right, then clobbering sp is and has always been
>>>>> ignored.
>>>
>>> PR77904 was about the clobber not being ignored, so the behaviour
>>> hasn't been consistent.
>>>
>>> I'm also not sure it was always ignored in recent sources.  The clobber
>>> does get added to the associated rtl insn, and it'd be surprising if
>>> that never had an effect.
>>>
>>>>> If that is right, then I would much prefer a warning, that says exactly
>>>>> that, because that would also help to understand why removing that clobber
>>>>> statement is safe even for old gcc versions.
>>>
>>> If the asm does leave sp with a different value, then it's never been safe,
>>> regardless of the gcc version.  That's why an error seems more appropriate.
>>>
>>>> Thank you. Looks like general consensus is to have a warning. See attached
>>>> patch that switches the error to a warning.
>>>
>>> I don't think there's a good reason to treat this differently from the
>>> preexisting PIC register error.  If the argument for making it a warning
>>> rather than an error is that the asm might happen to work by accident,
>>> then the same is true for the PIC register.
>>>
>>
>> If we leave the error, maybe a more explanatory message would be helpful?
>> (along the lines of what I posted earlier in this thread, which may be
>> too verbose)
> 
> The message in that patch suggested removing the clobber and hoping for
> the best, which IMO is bad advice.  If the current message doesn't make
> it clear enough that changing the sp is not allowed, how about:
> 
>      inline %<asm%> statements must not change the value of the stack pointer
> 
> ?
> 

Yes, things could be easy, but, mot the closer one looks, the more complicated
they get...

Even pushing a value on the stack and popping it again in the _same_ asm statement
is dangerous with red-zone targets. Maybe that would be also good to add as an advice?


Bernd.

> Thanks,
> Richard
> 
>      
> 

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

On 12/17/18 2:42 PM, Christophe Lyon wrote:
> On Mon, 17 Dec 2018 at 14:35, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Christophe Lyon <christophe.lyon@linaro.org> writes:
>>> On Mon, 17 Dec 2018 at 12:47, Richard Sandiford
>>> <richard.sandiford@arm.com> wrote:
>>>>
>>>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>>>>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>>>>> Hi,
>>>>>>
>>>>>> if I understood that right, then clobbering sp is and has always been
>>>>>> ignored.
>>>>
>>>> PR77904 was about the clobber not being ignored, so the behaviour
>>>> hasn't been consistent.
>>>>
>>>> I'm also not sure it was always ignored in recent sources.  The clobber
>>>> does get added to the associated rtl insn, and it'd be surprising if
>>>> that never had an effect.
>>>>
>>>>>> If that is right, then I would much prefer a warning, that says exactly
>>>>>> that, because that would also help to understand why removing that clobber
>>>>>> statement is safe even for old gcc versions.
>>>>
>>>> If the asm does leave sp with a different value, then it's never been safe,
>>>> regardless of the gcc version.  That's why an error seems more appropriate.
>>>>
>>>>> Thank you. Looks like general consensus is to have a warning. See attached
>>>>> patch that switches the error to a warning.
>>>>
>>>> I don't think there's a good reason to treat this differently from the
>>>> preexisting PIC register error.  If the argument for making it a warning
>>>> rather than an error is that the asm might happen to work by accident,
>>>> then the same is true for the PIC register.
>>>>
>>>
>>> If we leave the error, maybe a more explanatory message would be helpful?
>>> (along the lines of what I posted earlier in this thread, which may be
>>> too verbose)
>>
>> The message in that patch suggested removing the clobber and hoping for
>> the best, which IMO is bad advice.  If the current message doesn't make
>> it clear enough that changing the sp is not allowed, how about:
>>
>>      inline %<asm%> statements must not change the value of the stack pointer
>>
>> ?
>>
> 
> My understanding is that in some cases, some users still want to
> deliberately change SP,
> so "must not" may be confusing in such cases?
> 

What do they want to achieve, something like a longjmp ?

BTW: on arm, you can add r15 (PC) to the clobber list, and
that seems to be ignored as well.  If you want to jump to
another function, you might want to clobber "sp", "pc".
PS: I know that is invalid, just guessing why there is an itch.


Bernd.

>> Thanks,
>> Richard
>>
>>

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

On Mon, 17 Dec 2018 at 14:35, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> writes:
> > On Mon, 17 Dec 2018 at 12:47, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> >> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
> >> >> Hi,
> >> >>
> >> >> if I understood that right, then clobbering sp is and has always been
> >> >> ignored.
> >>
> >> PR77904 was about the clobber not being ignored, so the behaviour
> >> hasn't been consistent.
> >>
> >> I'm also not sure it was always ignored in recent sources.  The clobber
> >> does get added to the associated rtl insn, and it'd be surprising if
> >> that never had an effect.
> >>
> >> >> If that is right, then I would much prefer a warning, that says exactly
> >> >> that, because that would also help to understand why removing that clobber
> >> >> statement is safe even for old gcc versions.
> >>
> >> If the asm does leave sp with a different value, then it's never been safe,
> >> regardless of the gcc version.  That's why an error seems more appropriate.
> >>
> >> > Thank you. Looks like general consensus is to have a warning. See attached
> >> > patch that switches the error to a warning.
> >>
> >> I don't think there's a good reason to treat this differently from the
> >> preexisting PIC register error.  If the argument for making it a warning
> >> rather than an error is that the asm might happen to work by accident,
> >> then the same is true for the PIC register.
> >>
> >
> > If we leave the error, maybe a more explanatory message would be helpful?
> > (along the lines of what I posted earlier in this thread, which may be
> > too verbose)
>
> The message in that patch suggested removing the clobber and hoping for
> the best, which IMO is bad advice.  If the current message doesn't make
> it clear enough that changing the sp is not allowed, how about:
>
>     inline %<asm%> statements must not change the value of the stack pointer
>
> ?
>

My understanding is that in some cases, some users still want to
deliberately change SP,
so "must not" may be confusing in such cases?

> Thanks,
> Richard
>
>

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Mon, 17 Dec 2018 at 12:47, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>> >> Hi,
>> >>
>> >> if I understood that right, then clobbering sp is and has always been
>> >> ignored.
>>
>> PR77904 was about the clobber not being ignored, so the behaviour
>> hasn't been consistent.
>>
>> I'm also not sure it was always ignored in recent sources.  The clobber
>> does get added to the associated rtl insn, and it'd be surprising if
>> that never had an effect.
>>
>> >> If that is right, then I would much prefer a warning, that says exactly
>> >> that, because that would also help to understand why removing that clobber
>> >> statement is safe even for old gcc versions.
>>
>> If the asm does leave sp with a different value, then it's never been safe,
>> regardless of the gcc version.  That's why an error seems more appropriate.
>>
>> > Thank you. Looks like general consensus is to have a warning. See attached
>> > patch that switches the error to a warning.
>>
>> I don't think there's a good reason to treat this differently from the
>> preexisting PIC register error.  If the argument for making it a warning
>> rather than an error is that the asm might happen to work by accident,
>> then the same is true for the PIC register.
>>
>
> If we leave the error, maybe a more explanatory message would be helpful?
> (along the lines of what I posted earlier in this thread, which may be
> too verbose)

The message in that patch suggested removing the clobber and hoping for
the best, which IMO is bad advice.  If the current message doesn't make
it clear enough that changing the sp is not allowed, how about:

    inline %<asm%> statements must not change the value of the stack pointer

?

Thanks,
Richard

    

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Christophe Lyon]

On Mon, 17 Dec 2018 at 12:47, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
> > On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
> >> Hi,
> >>
> >> if I understood that right, then clobbering sp is and has always been
> >> ignored.
>
> PR77904 was about the clobber not being ignored, so the behaviour
> hasn't been consistent.
>
> I'm also not sure it was always ignored in recent sources.  The clobber
> does get added to the associated rtl insn, and it'd be surprising if
> that never had an effect.
>
> >> If that is right, then I would much prefer a warning, that says exactly
> >> that, because that would also help to understand why removing that clobber
> >> statement is safe even for old gcc versions.
>
> If the asm does leave sp with a different value, then it's never been safe,
> regardless of the gcc version.  That's why an error seems more appropriate.
>
> > Thank you. Looks like general consensus is to have a warning. See attached
> > patch that switches the error to a warning.
>
> I don't think there's a good reason to treat this differently from the
> preexisting PIC register error.  If the argument for making it a warning
> rather than an error is that the asm might happen to work by accident,
> then the same is true for the PIC register.
>

If we leave the error, maybe a more explanatory message would be helpful?
(along the lines of what I posted earlier in this thread, which may be
too verbose)

Christophe

> Thanks,
> Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Richard Sandiford]

Dimitar Dimitrov <dimitar@dinux.eu> writes:
> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>> Hi,
>> 
>> if I understood that right, then clobbering sp is and has always been
>> ignored.

PR77904 was about the clobber not being ignored, so the behaviour
hasn't been consistent.

I'm also not sure it was always ignored in recent sources.  The clobber
does get added to the associated rtl insn, and it'd be surprising if
that never had an effect.

>> If that is right, then I would much prefer a warning, that says exactly
>> that, because that would also help to understand why removing that clobber
>> statement is safe even for old gcc versions.

If the asm does leave sp with a different value, then it's never been safe,
regardless of the gcc version.  That's why an error seems more appropriate.

> Thank you. Looks like general consensus is to have a warning. See attached 
> patch that switches the error to a warning.

I don't think there's a good reason to treat this differently from the
preexisting PIC register error.  If the argument for making it a warning
rather than an error is that the asm might happen to work by accident,
then the same is true for the PIC register.

Thanks,
Richard

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Dimitar Dimitrov]

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

On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
> Hi,
> 
> if I understood that right, then clobbering sp is and has always been
> ignored.
>
> If that is right, then I would much prefer a warning, that says exactly
> that, because that would also help to understand why removing that clobber
> statement is safe even for old gcc versions.
> 
> Since your patch did not actually change the handling of the PIC register,
> that one should of course stay an error.

Thank you. Looks like general consensus is to have a warning. See attached 
patch that switches the error to a warning.

Regards,
Dimitar

[-- Attachment #2: 0001-PR-target-52813.patch --]
[-- Type: text/x-patch, Size: 2298 bytes --]

From d589ebd7824b4505ab75a2404f49a7c200679545 Mon Sep 17 00:00:00 2001
From: Dimitar Dimitrov <dimitar@dinux.eu>
Date: Sun, 16 Dec 2018 10:13:18 +0200
Subject: [PATCH] PR target/52813

Turns out there are existing programs that clobber stack pointer.
To avoid disruption, switch the newly introduced error to a warning.

Tested with:
  $ make check-gcc-c RUNTESTFLAGS="i386.exp=pr52813.c "

gcc/ChangeLog:

2018-12-16  Dimitar Dimitrov  <dimitar@dinux.eu>

	* cfgexpand.c (asm_clobber_reg_is_valid): Switch error to warning.
	Add clarification why there is a warning.

gcc/testsuite/ChangeLog:

2018-12-16  Dimitar Dimitrov  <dimitar@dinux.eu>

	* gcc.target/i386/pr52813.c (test1): Update warning message.

Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu>
---
 gcc/cfgexpand.c                         | 7 +++++--
 gcc/testsuite/gcc.target/i386/pr52813.c | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 0d04bbcafce..1e44c9a7ad0 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2872,10 +2872,13 @@ asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
+  /* Clobbered STACK POINTER register is not saved/restored by GCC,
+     which is often unexpected by users.  See PR52813.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
+      warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
+	       regname);
+      warning (0, "GCC has always ignored Stack Pointer %<asm%> clobbers");
       is_valid = false;
     }
 
diff --git a/gcc/testsuite/gcc.target/i386/pr52813.c b/gcc/testsuite/gcc.target/i386/pr52813.c
index 154ebbfc423..644fef15fef 100644
--- a/gcc/testsuite/gcc.target/i386/pr52813.c
+++ b/gcc/testsuite/gcc.target/i386/pr52813.c
@@ -5,5 +5,5 @@
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%esp"); /* { dg-warning "Stack Pointer register clobbered.\+GCC has always ignored Stack Pointer 'asm' clobbers" } */
 }
-- 
2.11.0

Re: [PATCH] [RFC] PR target/52813 and target/11807

[Bernd Edlinger]

Hi,

if I understood that right, then clobbering sp is and has always been ignored.

If that is right, then I would much prefer a warning, that says exactly that,
because that would also help to understand why removing that clobber statement
is safe even for old gcc versions.

Since your patch did not actually change the handling of the PIC register, that
one should of course stay an error.


Thanks
Bernd.