Re: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743]
On Tue, 30 Jun 2020 at 15:34, Kyrylo Tkachov wrote:
>
>
>
> > -Original Message-
> > From: Christophe Lyon
> > Sent: 30 June 2020 14:32
> > To: Kyrylo Tkachov
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: Re: [PATCH] arm: Warn if IRQ handler is not compiled with -
> > mgeneral-regs-only [PR target/94743]
> >
> > On Tue, 30 Jun 2020 at 15:16, Kyrylo Tkachov
> > wrote:
> > >
> > > Hi Christophe,
> > >
> > > Sorry for the delay.
> > >
> > > > -Original Message-
> > > > From: Gcc-patches On Behalf Of
> > > > Christophe Lyon via Gcc-patches
> > > > Sent: 29 April 2020 16:19
> > > > To: gcc-patches@gcc.gnu.org
> > > > Subject: [PATCH] arm: Warn if IRQ handler is not compiled with -
> > mgeneral-
> > > > regs-only [PR target/94743]
> > > >
> > > > The interrupt attribute does not guarantee that the FP registers are
> > > > saved, which can result in problems difficult to debug.
> > > >
> > > > Saving the FP registers and status registers can be a large penalty,
> > > > so it's probably not desirable to do that all the time.
> > > >
> > > > If the handler calls other functions, we'd likely need to save all of
> > > > them, for lack of knowledge of which registers they actually use.
> > > >
> > > > This is even more obscure for the end-user when the compiler inserts
> > > > calls to helper functions such as memcpy (some multilibs do use FP
> > > > registers to speed it up).
> > > >
> > > > In the PR, we discussed adding routines in libgcc to save the FP
> > > > context and saving only locally-clobbered FP registers, but I think
> > > > this is too intrusive for stage 4.
> > > >
> > > > In the mean time, emit a warning to suggest re-compiling with
> > > > -mgeneral-regs-only. Note that this can lead to errors if the code
> > > > uses floating-point and -mfloat-abi=hard, eg:
> > > > argument of type 'double' not permitted with -mgeneral-regs-only
> > > >
> > > > This can be troublesome for the user, but at least this would make
> > > > them aware of the latent issue.
> > > >
> > > > The patch adds two testcases:
> > > > - pr94734-1.c checks that a warning is emitted. One function can make
> > > > implicit calls to runtime floating-point routines, the other one
> > > > doesn't. We can improve the diagnostic later not to warn in the
> > > > second case.
> > > >
> > > > - pr94734-2.c checks that no warning is emitted when using
> > > > -mgeneral-regs-only.
> > > >
> > > > 2020-04-29 Christophe Lyon
> > > >
> > > > PR target/94743
> > > > gcc/
> > > > * config/arm/arm.c (arm_handle_isr_attribute): Warn if
> > > > -mgeneral-regs-only is not used.
> > > >
> > > > gcc/testsuite/
> > > > * gcc.target/arm/pr94743-1.c: New test.
> > > > * gcc.target/arm/pr94743-2.c: New test.
> > > > ---
> > > > gcc/config/arm/arm.c | 5 +
> > > > gcc/testsuite/gcc.target/arm/pr94743-1.c | 20
> > > > gcc/testsuite/gcc.target/arm/pr94743-2.c | 17 +
> > > > 3 files changed, 42 insertions(+)
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c
> > > >
> > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> > > > index 6a6e804..34aad1d 100644
> > > > --- a/gcc/config/arm/arm.c
> > > > +++ b/gcc/config/arm/arm.c
> > > > @@ -7176,6 +7176,11 @@ arm_handle_isr_attribute (tree *node, tree
> > name,
> > > > tree args, int flags,
> > > > name);
> > > > *no_add_attrs = true;
> > > > }
> > > > + else if (TARGET_VFP_BASE)
> > > > + {
> > > > + warning (OPT_Wattributes, "FP registers might be clobbered
> > > > despite %qE attribute: compile with -mgeneral-regs-only",
> > > > +name);
> > > > + }
> > >
> > > Let's use %< and %> to quote -mgeneral-regs-only properly.
> > > Ok with that change.
> >
> > Hi Kyrill,
> >
> > Thanks for the review, however I have posted v2 here:
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html
> > (you approved v1)
> >
> > It includes a few more testcases and updates to existing ones.
> >
> > Is v2 OK with the quotation marks fixed?
> >
>
> Oops, sorry. Yes that looks ok too (with the quotation fixed).
> Kyrill
>
Thanks, pushed as r11-1732.
There were two follow-ups: r11-1752 because I forgot to update the
expected warning message in the testcases after I changed the
quotation)
and r11-1759 because I missed that gcc.target/arm/handler-align.c has
to be compiled with -mgeneral-regs-only like other testcases.
Sorry for the noise,
Christophe
> > Thanks
> >
> > Christophe
> >
> > > Thanks,
> > > Kyrill
> > >
> > > >/* FIXME: the argument if any is checked for type attributes;
> > > >should it be checked for decl ones? */
> > > > }
> > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > > b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > > new file mode 100644
> > > > index
RE: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743]
> -Original Message-
> From: Christophe Lyon
> Sent: 30 June 2020 14:32
> To: Kyrylo Tkachov
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] arm: Warn if IRQ handler is not compiled with -
> mgeneral-regs-only [PR target/94743]
>
> On Tue, 30 Jun 2020 at 15:16, Kyrylo Tkachov
> wrote:
> >
> > Hi Christophe,
> >
> > Sorry for the delay.
> >
> > > -Original Message-
> > > From: Gcc-patches On Behalf Of
> > > Christophe Lyon via Gcc-patches
> > > Sent: 29 April 2020 16:19
> > > To: gcc-patches@gcc.gnu.org
> > > Subject: [PATCH] arm: Warn if IRQ handler is not compiled with -
> mgeneral-
> > > regs-only [PR target/94743]
> > >
> > > The interrupt attribute does not guarantee that the FP registers are
> > > saved, which can result in problems difficult to debug.
> > >
> > > Saving the FP registers and status registers can be a large penalty,
> > > so it's probably not desirable to do that all the time.
> > >
> > > If the handler calls other functions, we'd likely need to save all of
> > > them, for lack of knowledge of which registers they actually use.
> > >
> > > This is even more obscure for the end-user when the compiler inserts
> > > calls to helper functions such as memcpy (some multilibs do use FP
> > > registers to speed it up).
> > >
> > > In the PR, we discussed adding routines in libgcc to save the FP
> > > context and saving only locally-clobbered FP registers, but I think
> > > this is too intrusive for stage 4.
> > >
> > > In the mean time, emit a warning to suggest re-compiling with
> > > -mgeneral-regs-only. Note that this can lead to errors if the code
> > > uses floating-point and -mfloat-abi=hard, eg:
> > > argument of type 'double' not permitted with -mgeneral-regs-only
> > >
> > > This can be troublesome for the user, but at least this would make
> > > them aware of the latent issue.
> > >
> > > The patch adds two testcases:
> > > - pr94734-1.c checks that a warning is emitted. One function can make
> > > implicit calls to runtime floating-point routines, the other one
> > > doesn't. We can improve the diagnostic later not to warn in the
> > > second case.
> > >
> > > - pr94734-2.c checks that no warning is emitted when using
> > > -mgeneral-regs-only.
> > >
> > > 2020-04-29 Christophe Lyon
> > >
> > > PR target/94743
> > > gcc/
> > > * config/arm/arm.c (arm_handle_isr_attribute): Warn if
> > > -mgeneral-regs-only is not used.
> > >
> > > gcc/testsuite/
> > > * gcc.target/arm/pr94743-1.c: New test.
> > > * gcc.target/arm/pr94743-2.c: New test.
> > > ---
> > > gcc/config/arm/arm.c | 5 +
> > > gcc/testsuite/gcc.target/arm/pr94743-1.c | 20
> > > gcc/testsuite/gcc.target/arm/pr94743-2.c | 17 +
> > > 3 files changed, 42 insertions(+)
> > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c
> > >
> > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> > > index 6a6e804..34aad1d 100644
> > > --- a/gcc/config/arm/arm.c
> > > +++ b/gcc/config/arm/arm.c
> > > @@ -7176,6 +7176,11 @@ arm_handle_isr_attribute (tree *node, tree
> name,
> > > tree args, int flags,
> > > name);
> > > *no_add_attrs = true;
> > > }
> > > + else if (TARGET_VFP_BASE)
> > > + {
> > > + warning (OPT_Wattributes, "FP registers might be clobbered
> > > despite %qE attribute: compile with -mgeneral-regs-only",
> > > +name);
> > > + }
> >
> > Let's use %< and %> to quote -mgeneral-regs-only properly.
> > Ok with that change.
>
> Hi Kyrill,
>
> Thanks for the review, however I have posted v2 here:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html
> (you approved v1)
>
> It includes a few more testcases and updates to existing ones.
>
> Is v2 OK with the quotation marks fixed?
>
Oops, sorry. Yes that looks ok too (with the quotation fixed).
Kyrill
> Thanks
>
> Christophe
>
> > Thanks,
> > Kyrill
> >
> > >/* FIXME: the argument if any is checked for type attributes;
> > >should it be checked for decl ones? */
> > > }
> > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > new file mode 100644
> > > index 000..67700c6
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > > @@ -0,0 +1,20 @@
> > > +/* PR target/94743 */
> > > +/* { dg-do compile } */
> > > +
> > > +typedef struct {
> > > + double fpdata[32];
> > > +} dummy_t;
> > > +
> > > +dummy_t global_d;
> > > +dummy_t global_d1;
> > > +
> > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> > > +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt'
> > > attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */
> > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> > > +}
> > > +
> > > +
> > >
Re: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743]
On Tue, 30 Jun 2020 at 15:16, Kyrylo Tkachov wrote:
>
> Hi Christophe,
>
> Sorry for the delay.
>
> > -Original Message-
> > From: Gcc-patches On Behalf Of
> > Christophe Lyon via Gcc-patches
> > Sent: 29 April 2020 16:19
> > To: gcc-patches@gcc.gnu.org
> > Subject: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-
> > regs-only [PR target/94743]
> >
> > The interrupt attribute does not guarantee that the FP registers are
> > saved, which can result in problems difficult to debug.
> >
> > Saving the FP registers and status registers can be a large penalty,
> > so it's probably not desirable to do that all the time.
> >
> > If the handler calls other functions, we'd likely need to save all of
> > them, for lack of knowledge of which registers they actually use.
> >
> > This is even more obscure for the end-user when the compiler inserts
> > calls to helper functions such as memcpy (some multilibs do use FP
> > registers to speed it up).
> >
> > In the PR, we discussed adding routines in libgcc to save the FP
> > context and saving only locally-clobbered FP registers, but I think
> > this is too intrusive for stage 4.
> >
> > In the mean time, emit a warning to suggest re-compiling with
> > -mgeneral-regs-only. Note that this can lead to errors if the code
> > uses floating-point and -mfloat-abi=hard, eg:
> > argument of type 'double' not permitted with -mgeneral-regs-only
> >
> > This can be troublesome for the user, but at least this would make
> > them aware of the latent issue.
> >
> > The patch adds two testcases:
> > - pr94734-1.c checks that a warning is emitted. One function can make
> > implicit calls to runtime floating-point routines, the other one
> > doesn't. We can improve the diagnostic later not to warn in the
> > second case.
> >
> > - pr94734-2.c checks that no warning is emitted when using
> > -mgeneral-regs-only.
> >
> > 2020-04-29 Christophe Lyon
> >
> > PR target/94743
> > gcc/
> > * config/arm/arm.c (arm_handle_isr_attribute): Warn if
> > -mgeneral-regs-only is not used.
> >
> > gcc/testsuite/
> > * gcc.target/arm/pr94743-1.c: New test.
> > * gcc.target/arm/pr94743-2.c: New test.
> > ---
> > gcc/config/arm/arm.c | 5 +
> > gcc/testsuite/gcc.target/arm/pr94743-1.c | 20
> > gcc/testsuite/gcc.target/arm/pr94743-2.c | 17 +
> > 3 files changed, 42 insertions(+)
> > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1.c
> > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c
> >
> > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> > index 6a6e804..34aad1d 100644
> > --- a/gcc/config/arm/arm.c
> > +++ b/gcc/config/arm/arm.c
> > @@ -7176,6 +7176,11 @@ arm_handle_isr_attribute (tree *node, tree name,
> > tree args, int flags,
> > name);
> > *no_add_attrs = true;
> > }
> > + else if (TARGET_VFP_BASE)
> > + {
> > + warning (OPT_Wattributes, "FP registers might be clobbered
> > despite %qE attribute: compile with -mgeneral-regs-only",
> > +name);
> > + }
>
> Let's use %< and %> to quote -mgeneral-regs-only properly.
> Ok with that change.
Hi Kyrill,
Thanks for the review, however I have posted v2 here:
https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html
(you approved v1)
It includes a few more testcases and updates to existing ones.
Is v2 OK with the quotation marks fixed?
Thanks
Christophe
> Thanks,
> Kyrill
>
> >/* FIXME: the argument if any is checked for type attributes;
> >should it be checked for decl ones? */
> > }
> > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > new file mode 100644
> > index 000..67700c6
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> > @@ -0,0 +1,20 @@
> > +/* PR target/94743 */
> > +/* { dg-do compile } */
> > +
> > +typedef struct {
> > + double fpdata[32];
> > +} dummy_t;
> > +
> > +dummy_t global_d;
> > +dummy_t global_d1;
> > +
> > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> > +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt'
> > attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */
> > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> > +}
> > +
> > +
> > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test2(void)
> > +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt'
> > attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */
> > + global_d.fpdata[3] = 1.0;
> > +}
> > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-2.c
> > b/gcc/testsuite/gcc.target/arm/pr94743-2.c
> > new file mode 100644
> > index 000..745fd36
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/pr94743-2.c
> > @@ -0,0 +1,17 @@
> > +/* PR target/94743 */
> > +/* { dg-do compile } */
> > +/* { dg-options
RE: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743]
Hi Christophe,
Sorry for the delay.
> -Original Message-
> From: Gcc-patches On Behalf Of
> Christophe Lyon via Gcc-patches
> Sent: 29 April 2020 16:19
> To: gcc-patches@gcc.gnu.org
> Subject: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-
> regs-only [PR target/94743]
>
> The interrupt attribute does not guarantee that the FP registers are
> saved, which can result in problems difficult to debug.
>
> Saving the FP registers and status registers can be a large penalty,
> so it's probably not desirable to do that all the time.
>
> If the handler calls other functions, we'd likely need to save all of
> them, for lack of knowledge of which registers they actually use.
>
> This is even more obscure for the end-user when the compiler inserts
> calls to helper functions such as memcpy (some multilibs do use FP
> registers to speed it up).
>
> In the PR, we discussed adding routines in libgcc to save the FP
> context and saving only locally-clobbered FP registers, but I think
> this is too intrusive for stage 4.
>
> In the mean time, emit a warning to suggest re-compiling with
> -mgeneral-regs-only. Note that this can lead to errors if the code
> uses floating-point and -mfloat-abi=hard, eg:
> argument of type 'double' not permitted with -mgeneral-regs-only
>
> This can be troublesome for the user, but at least this would make
> them aware of the latent issue.
>
> The patch adds two testcases:
> - pr94734-1.c checks that a warning is emitted. One function can make
> implicit calls to runtime floating-point routines, the other one
> doesn't. We can improve the diagnostic later not to warn in the
> second case.
>
> - pr94734-2.c checks that no warning is emitted when using
> -mgeneral-regs-only.
>
> 2020-04-29 Christophe Lyon
>
> PR target/94743
> gcc/
> * config/arm/arm.c (arm_handle_isr_attribute): Warn if
> -mgeneral-regs-only is not used.
>
> gcc/testsuite/
> * gcc.target/arm/pr94743-1.c: New test.
> * gcc.target/arm/pr94743-2.c: New test.
> ---
> gcc/config/arm/arm.c | 5 +
> gcc/testsuite/gcc.target/arm/pr94743-1.c | 20
> gcc/testsuite/gcc.target/arm/pr94743-2.c | 17 +
> 3 files changed, 42 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1.c
> create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 6a6e804..34aad1d 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -7176,6 +7176,11 @@ arm_handle_isr_attribute (tree *node, tree name,
> tree args, int flags,
> name);
> *no_add_attrs = true;
> }
> + else if (TARGET_VFP_BASE)
> + {
> + warning (OPT_Wattributes, "FP registers might be clobbered
> despite %qE attribute: compile with -mgeneral-regs-only",
> +name);
> + }
Let's use %< and %> to quote -mgeneral-regs-only properly.
Ok with that change.
Thanks,
Kyrill
>/* FIXME: the argument if any is checked for type attributes;
>should it be checked for decl ones? */
> }
> diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1.c
> b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> new file mode 100644
> index 000..67700c6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr94743-1.c
> @@ -0,0 +1,20 @@
> +/* PR target/94743 */
> +/* { dg-do compile } */
> +
> +typedef struct {
> + double fpdata[32];
> +} dummy_t;
> +
> +dummy_t global_d;
> +dummy_t global_d1;
> +
> +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt'
> attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */
> + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> +}
> +
> +
> +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test2(void)
> +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt'
> attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */
> + global_d.fpdata[3] = 1.0;
> +}
> diff --git a/gcc/testsuite/gcc.target/arm/pr94743-2.c
> b/gcc/testsuite/gcc.target/arm/pr94743-2.c
> new file mode 100644
> index 000..745fd36
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr94743-2.c
> @@ -0,0 +1,17 @@
> +/* PR target/94743 */
> +/* { dg-do compile } */
> +/* { dg-options "-mgeneral-regs-only" } */
> +
> +typedef struct {
> + /* Do not use floating-point types, which are not be compatible with
> + -mgeneral-regs-only under -mfloat-abi=hard */
> + int fpdata[32];
> +} dummy_t;
> +
> +dummy_t global_d;
> +dummy_t global_d1;
> +
> +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> +{
> + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> +}
> --
> 2.7.4
