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

[Christophe Lyon <christophe.lyon@linaro.org>]

[-- 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;
     }