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