diff mbox series

[RFC] PR target/52813 and target/11807

Message ID 20181209100856.14051-1-dimitar@dinux.eu
State New
Headers show
Series [RFC] PR target/52813 and target/11807 | expand

Commit Message

Dimitar Dimitrov Dec. 9, 2018, 10:08 a.m. UTC 
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

Comments

Richard Sandiford Dec. 10, 2018, 11:21 a.m. UTC | #1 
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
Dimitar Dimitrov Dec. 10, 2018, 7:36 p.m. UTC | #2 
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
Richard Sandiford Dec. 11, 2018, 3:52 p.m. UTC | #3 
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
Christophe Lyon Dec. 12, 2018, 9:42 a.m. UTC | #4 
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
Christophe Lyon Dec. 12, 2018, 10:03 a.m. UTC | #5 
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
Thomas Preudhomme Dec. 12, 2018, 10:30 a.m. UTC | #6 
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
Thomas Preudhomme Dec. 12, 2018, 11:21 a.m. UTC | #7 
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
Andreas Schwab Dec. 12, 2018, 11:23 a.m. UTC | #8 
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.  */
Christophe Lyon Dec. 12, 2018, 1:19 p.m. UTC | #9 
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
Christophe Lyon Dec. 12, 2018, 3:13 p.m. UTC | #10 
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
2018-12-12  Christophe Lyon  <christophe.lyon@linaro.org>

	gcc/
	* cfgexpand.c (asm_clobber_reg_is_valid): Add a more descriptive
	error message.
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;
     }
Thomas Preudhomme Dec. 12, 2018, 3:35 p.m. UTC | #11 
[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
Dimitar Dimitrov Dec. 12, 2018, 4:26 p.m. UTC | #12 
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
Dimitar Dimitrov Dec. 12, 2018, 4:39 p.m. UTC | #13 
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
Segher Boessenkool Dec. 13, 2018, 2:48 p.m. UTC | #14 
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
Dimitar Dimitrov Dec. 13, 2018, 9:26 p.m. UTC | #15 
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
Segher Boessenkool Dec. 14, 2018, 8:52 a.m. UTC | #16 
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
Richard Sandiford Dec. 14, 2018, 1:49 p.m. UTC | #17 
(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
Segher Boessenkool Dec. 15, 2018, 3:38 p.m. UTC | #18 
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
Dimitar Dimitrov Dec. 16, 2018, 8:43 a.m. UTC | #19 
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
Segher Boessenkool Dec. 17, 2018, 3:23 p.m. UTC | #20 
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
diff mbox series

Patch

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" } */
+}