On Wed, 12 Dec 2018 at 14:19, Christophe Lyon
<christophe.l...@linaro.org> wrote:
>
> On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme
> <thomas.preudho...@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.preudho...@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.l...@linaro.org> wrote:
> > > >
> > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford
> > > > <richard.sandif...@arm.com> wrote:
> > > > >
> > > > > Dimitar Dimitrov <dimi...@dinux.eu> writes:
> > > > > > On понеделник, 10 декември 2018 г. 11:21:53 EET Richard Sandiford
> > > > > > wrote:
> > > > > >> Dimitar Dimitrov <dimi...@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 <dimi...@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 <dimi...@dinux.eu>
> > > > > >> >
> > > > > >> > * gcc.target/i386/pr52813.c: New test.
> > > > > >> >
> > > > > >> > Signed-off-by: Dimitar Dimitrov <dimi...@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.l...@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;
}
