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.l...@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.l...@linaro.org> wrote: > > > > Ping? > > > > On Thu, 14 May 2020 at 16:57, Christophe Lyon > > <christophe.l...@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.l...@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 > > >
