[PATCH v2] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743]
Christophe Lyon
christophe.lyon@linaro.org
Mon Jun 29 19:30:45 GMT 2020
Ping?
On Tue, 9 Jun 2020 at 11:48, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>
> Ping?
>
> Maybe I could mention that LLVM emits a warning in this case
> (https://reviews.llvm.org/D28820).
>
> Thanks,
>
> Christophe
>
>
> On Wed, 3 Jun 2020 at 15:23, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> >
> > Ping?
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html
> >
> > On Wed, 27 May 2020 at 13:52, Christophe Lyon
> > <christophe.lyon@linaro.org> wrote:
> > >
> > > Ping?
> > >
> > > On Thu, 14 May 2020 at 16:57, Christophe Lyon
> > > <christophe.lyon@linaro.org> wrote:
> > > >
> > > > 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 clobber.
> > > >
> > > > 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 this seems
> > > > to be too much work for the purpose, given that in general such
> > > > handlers try to avoid this kind of penalty.
> > > > I suspect we would also want new attributes to instruct the compiler
> > > > that saving the FP context is not needed.
> > > >
> > > > 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
> > > > him aware of the latent issue.
> > > >
> > > > The patch adds several testcases:
> > > >
> > > > - pr94734-1-hard.c checks that a warning is emitted when using
> > > > -mfloat-abi=hard. Function IRQ_HDLR_Test can make implicit calls to
> > > > runtime floating-point routines (or direct use of FP instructions),
> > > > IRQ_HDLR_Test2 doesn't. We emit a warning in both cases, though.
> > > >
> > > > - pr94734-1-softfp.c: same as above wih -mfloat-abi=softfp.
> > > >
> > > > - pr94734-1-soft.c checks that no warning is emitted when using
> > > > -mfloat-abi=soft when the same code as above.
> > > >
> > > > - pr94734-2.c checks that no warning is emitted when using
> > > > -mgeneral-regs-only.
> > > >
> > > > - pr94734-3.c checks that no warning is emitted when using
> > > > -mgeneral-regs-only even using float-point data.
> > > >
> > > > 2020-05-14 Christophe Lyon <christophe.lyon@linaro.org>
> > > >
> > > > PR target/94743
> > > > gcc/
> > > > * config/arm/arm.c (arm_handle_isr_attribute): Warn if
> > > > -mgeneral-regs-only is not used.
> > > >
> > > > gcc/testsuite/
> > > > * gcc.misc-tests/arm-isr.c: Add -mgeneral-regs-only.
> > > > * gcc.target/arm/empty_fiq_handler.c: Add -mgeneral-regs-only.
> > > > * gcc.target/arm/interrupt-1.c: Add -mgeneral-regs-only.
> > > > * gcc.target/arm/interrupt-2.c: Add -mgeneral-regs-only.
> > > > * gcc.target/arm/pr70830.c: Add -mgeneral-regs-only.
> > > > * gcc.target/arm/pr94743-1-hard.c: New test.
> > > > * gcc.target/arm/pr94743-1-soft.c: New test.
> > > > * gcc.target/arm/pr94743-1-softfp.c: New test.
> > > > * gcc.target/arm/pr94743-2.c: New test.
> > > > * gcc.target/arm/pr94743-3.c: New test.
> > > > ---
> > > > gcc/config/arm/arm.c | 5 ++++
> > > > gcc/testsuite/gcc.misc-tests/arm-isr.c | 2 ++
> > > > gcc/testsuite/gcc.target/arm/empty_fiq_handler.c | 1 +
> > > > gcc/testsuite/gcc.target/arm/interrupt-1.c | 2 +-
> > > > gcc/testsuite/gcc.target/arm/interrupt-2.c | 2 +-
> > > > gcc/testsuite/gcc.target/arm/pr70830.c | 2 +-
> > > > gcc/testsuite/gcc.target/arm/pr94743-1-hard.c | 29 ++++++++++++++++++++++++
> > > > gcc/testsuite/gcc.target/arm/pr94743-1-soft.c | 27 ++++++++++++++++++++++
> > > > gcc/testsuite/gcc.target/arm/pr94743-1-softfp.c | 29 ++++++++++++++++++++++++
> > > > gcc/testsuite/gcc.target/arm/pr94743-2.c | 22 ++++++++++++++++++
> > > > gcc/testsuite/gcc.target/arm/pr94743-3.c | 23 +++++++++++++++++++
> > > > 11 files changed, 141 insertions(+), 3 deletions(-)
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1-hard.c
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1-soft.c
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1-softfp.c
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c
> > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-3.c
> > > >
> > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> > > > index dda8771..c88de3e 100644
> > > > --- a/gcc/config/arm/arm.c
> > > > +++ b/gcc/config/arm/arm.c
> > > > @@ -7232,6 +7232,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);
> > > > + }
> > > > /* FIXME: the argument if any is checked for type attributes;
> > > > should it be checked for decl ones? */
> > > > }
> > > > diff --git a/gcc/testsuite/gcc.misc-tests/arm-isr.c b/gcc/testsuite/gcc.misc-tests/arm-isr.c
> > > > index 737f9ff..9eff52c 100644
> > > > --- a/gcc/testsuite/gcc.misc-tests/arm-isr.c
> > > > +++ b/gcc/testsuite/gcc.misc-tests/arm-isr.c
> > > > @@ -1,3 +1,5 @@
> > > > +/* { dg-options "-mgeneral-regs-only" } */
> > > > +
> > > > extern void abort ();
> > > > extern void exit (int);
> > > >
> > > > diff --git a/gcc/testsuite/gcc.target/arm/empty_fiq_handler.c b/gcc/testsuite/gcc.target/arm/empty_fiq_handler.c
> > > > index 8313f21..6911ffc 100644
> > > > --- a/gcc/testsuite/gcc.target/arm/empty_fiq_handler.c
> > > > +++ b/gcc/testsuite/gcc.target/arm/empty_fiq_handler.c
> > > > @@ -1,5 +1,6 @@
> > > > /* { dg-do compile } */
> > > > /* { dg-skip-if "" { ! arm_cortex_m } { "-mthumb" } } */
> > > > +/* { dg-options "-mgeneral-regs-only" } */
> > > >
> > > > /* Below code used to trigger an ICE due to missing constraints for
> > > > sp = fp + cst pattern. */
> > > > diff --git a/gcc/testsuite/gcc.target/arm/interrupt-1.c b/gcc/testsuite/gcc.target/arm/interrupt-1.c
> > > > index 493763d..de1ecc5 100644
> > > > --- a/gcc/testsuite/gcc.target/arm/interrupt-1.c
> > > > +++ b/gcc/testsuite/gcc.target/arm/interrupt-1.c
> > > > @@ -2,7 +2,7 @@
> > > > __attribute__ ((interrupt)). */
> > > > /* { dg-do assemble } */
> > > > /* { dg-require-effective-target arm_nothumb } */
> > > > -/* { dg-options "-O0 -marm -save-temps" } */
> > > > +/* { dg-options "-mgeneral-regs-only -O0 -marm -save-temps" } */
> > > >
> > > > /* This test is not valid when -mthumb. */
> > > > extern void bar (int);
> > > > diff --git a/gcc/testsuite/gcc.target/arm/interrupt-2.c b/gcc/testsuite/gcc.target/arm/interrupt-2.c
> > > > index 5be1f16..c64b98b 100644
> > > > --- a/gcc/testsuite/gcc.target/arm/interrupt-2.c
> > > > +++ b/gcc/testsuite/gcc.target/arm/interrupt-2.c
> > > > @@ -2,7 +2,7 @@
> > > > __attribute__ ((interrupt)). */
> > > > /* { dg-do assemble } */
> > > > /* { dg-require-effective-target arm_nothumb } */
> > > > -/* { dg-options "-O1 -marm -save-temps" } */
> > > > +/* { dg-options "-mgeneral-regs-only -O1 -marm -save-temps" } */
> > > >
> > > > /* This test is not valid when -mthumb. */
> > > > extern void bar (int);
> > > > diff --git a/gcc/testsuite/gcc.target/arm/pr70830.c b/gcc/testsuite/gcc.target/arm/pr70830.c
> > > > index cd84c42..bca596c 100644
> > > > --- a/gcc/testsuite/gcc.target/arm/pr70830.c
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr70830.c
> > > > @@ -1,7 +1,7 @@
> > > > /* PR target/70830. */
> > > > /* { dg-do assemble } */
> > > > /* { dg-require-effective-target arm_arm_ok } */
> > > > -/* { dg-options "-Os -marm -save-temps" } */
> > > > +/* { dg-options "-mgeneral-regs-only -Os -marm -save-temps" } */
> > > >
> > > > /* This test is not valid when -mthumb. */
> > > >
> > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1-hard.c b/gcc/testsuite/gcc.target/arm/pr94743-1-hard.c
> > > > new file mode 100644
> > > > index 0000000..928b79d
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1-hard.c
> > > > @@ -0,0 +1,29 @@
> > > > +/* PR target/94743 */
> > > > +/* { dg-do compile } */
> > > > +/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } {"-mfloat-abi=hard" } } */
> > > > +/* { dg-require-effective-target arm_vfp_ok } */
> > > > +/* { dg-add-options arm_vfp } */
> > > > +/* Make sure with use -mfloat-abi=hard. */
> > > > +/* { dg-additional-options "-mfloat-abi=hard" } */
> > > > +
> > > > +/* Check that we emit a warning when compiling an IRQ handler without
> > > > + -mgeneral-regs-only. */
> > > > +typedef struct {
> > > > + double fpdata[32];
> > > > +} dummy_t;
> > > > +
> > > > +dummy_t global_d;
> > > > +dummy_t global_d1;
> > > > +
> > > > +/* This function may clobber VFP registers. */
> > > > +__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];
> > > > +}
> > > > +
> > > > +/* This function does not need to clobber VFP registers. */
> > > > +/* Do we want to emit a (useless?) warning? */
> > > > +__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-1-soft.c b/gcc/testsuite/gcc.target/arm/pr94743-1-soft.c
> > > > new file mode 100644
> > > > index 0000000..e06a16d
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1-soft.c
> > > > @@ -0,0 +1,27 @@
> > > > +/* PR target/94743 */
> > > > +/* { dg-do compile } */
> > > > +/* Thumb1 mode not supported for interrupt routines. */
> > > > +/* { dg-require-effective-target arm32 } */
> > > > +/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } {"-mfloat-abi=soft" } } */
> > > > +/* { dg-options "-mfloat-abi=soft" } */
> > > > +
> > > > +/* Check that we do not emit a warning when compiling an IRQ handler without
> > > > + -mgeneral-regs-only with -mfloat-abi=soft. */
> > > > +typedef struct {
> > > > + double fpdata[32];
> > > > +} dummy_t;
> > > > +
> > > > +dummy_t global_d;
> > > > +dummy_t global_d1;
> > > > +
> > > > +/* This function may clobber VFP registers. */
> > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> > > > +{
> > > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> > > > +}
> > > > +
> > > > +/* This function does not need to clobber VFP registers. */
> > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test2(void)
> > > > +{
> > > > + global_d.fpdata[3] = 1.0;
> > > > +}
> > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1-softfp.c b/gcc/testsuite/gcc.target/arm/pr94743-1-softfp.c
> > > > new file mode 100644
> > > > index 0000000..6113eb6
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1-softfp.c
> > > > @@ -0,0 +1,29 @@
> > > > +/* PR target/94743 */
> > > > +/* { dg-do compile } */
> > > > +/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } {"-mfloat-abi=softfp" } } */
> > > > +/* { dg-require-effective-target arm_vfp_ok } */
> > > > +/* { dg-add-options arm_vfp } */
> > > > +/* Make sure with use -mfloat-abi=softfp. */
> > > > +/* { dg-additional-options "-mfloat-abi=softfp" } */
> > > > +
> > > > +/* Check that we emit a warning when compiling an IRQ handler without
> > > > + -mgeneral-regs-only. */
> > > > +typedef struct {
> > > > + double fpdata[32];
> > > > +} dummy_t;
> > > > +
> > > > +dummy_t global_d;
> > > > +dummy_t global_d1;
> > > > +
> > > > +/* This function may clobber VFP registers. */
> > > > +__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];
> > > > +}
> > > > +
> > > > +/* This function does not need to clobber VFP registers. */
> > > > +/* Do we want to emit a (useless?) warning? */
> > > > +__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 0000000..50a97de
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-2.c
> > > > @@ -0,0 +1,22 @@
> > > > +/* PR target/94743 */
> > > > +/* { dg-do compile } */
> > > > +/* Thumb1 mode not supported for interrupt routines. */
> > > > +/* { dg-require-effective-target arm32 } */
> > > > +/* { dg-options "-mgeneral-regs-only" } */
> > > > +
> > > > +/* Check that we do not emit a warning when compiling an IRQ handler
> > > > + with -mgeneral-regs-only. */
> > > > +typedef struct {
> > > > + /* Do not use floating-point types, which are not compatible with
> > > > + -mgeneral-regs-only under -mfloat-abi=hard */
> > > > + int fpdata[32];
> > > > +} dummy_t;
> > > > +
> > > > +dummy_t global_d;
> > > > +dummy_t global_d1;
> > > > +
> > > > +/* This function does not clobber VFP registers. */
> > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> > > > +{
> > > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> > > > +}
> > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-3.c b/gcc/testsuite/gcc.target/arm/pr94743-3.c
> > > > new file mode 100644
> > > > index 0000000..6b8ed2b
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-3.c
> > > > @@ -0,0 +1,23 @@
> > > > +/* PR target/94743 */
> > > > +/* { dg-do compile } */
> > > > +/* Thumb1 mode not supported for interrupt routines. */
> > > > +/* { dg-require-effective-target arm32 } */
> > > > +/* { dg-skip-if "do not override -mfloat-abi" { *-*-* } { "-mfloat-abi=*" } {"-mfloat-abi=soft" } } */
> > > > +/* { dg-options "-mgeneral-regs-only -mfloat-abi=soft" } */
> > > > +
> > > > +/* Check that we do not emit a warning when compiling an IRQ handler
> > > > + with -mgeneral-regs-only even when using floating-point data. */
> > > > +typedef struct {
> > > > + /* Since we use -mfloat=abi=soft, this will generate calls to
> > > > + libgcc, but won't clobber VFP registers. */
> > > > + double fpdata[32];
> > > > +} dummy_t;
> > > > +
> > > > +dummy_t global_d;
> > > > +dummy_t global_d1;
> > > > +
> > > > +/* This function does not clobber VFP registers. */
> > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void)
> > > > +{
> > > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3];
> > > > +}
> > > > --
> > > > 2.7.4
> > > >
More information about the Gcc-patches
mailing list