public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ")))
@ 2020-04-24 11:59 clyon at gcc dot gnu.org
  2020-04-27 12:01 ` [Bug target/94743] IRQ handler doesn't save scratch VFP registers clyon at gcc dot gnu.org
                   ` (25 more replies)
  0 siblings, 26 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-24 11:59 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

            Bug ID: 94743
           Summary: IRQ handler implementation wrong when using
                    __attribute__ ((interrupt("IRQ")))
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: clyon at gcc dot gnu.org
  Target Milestone: ---

Hi,

As described in https://bugs.linaro.org/show_bug.cgi?id=5614:

IRQ implementation when using __attribute__ ((interrupt("IRQ"))) by GCC does
not save/restore NEON scratch registers when using -mfloat-abi=hard
-mfpu=neon-fp16.

But if the handler calls a function that uses Neon scratch registers, this will
corrupt the values seen by the program when the IRQ handler completes.

An easy example uses memcpy in the handler:
=== irq_test.c ===

typedef struct {
    int data[32];
} dummy_t ;

extern void foo(dummy_t d);


__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void) {

    dummy_t d;
    d.data[3] = 3;

    foo(d);
}

===


Compile with arm-none-eabi-gcc  -mcpu=cortex-a9 -mtune=cortex-a9 -marm
-mfloat-abi=hard -mfpu=neon-fp16 -Ofast irq_test.c

The generated code looks like:

  21                    IRQ_HDLR_Test:
  22                            @ Interrupt Service Routine.
  23                            @ args = 0, pretend = 0, frame = 128
  24                            @ frame_needed = 0, uses_anonymous_args = 0
  25 0000 04E04EE2              sub     lr, lr, #4
  26 0004 0F502DE9              push    {r0, r1, r2, r3, ip, lr}
  27 0008 F0D04DE2              sub     sp, sp, #240
  28 000c 0330A0E3              mov     r3, #3
  29 0010 80108DE2              add     r1, sp, #128
  30 0014 7020A0E3              mov     r2, #112
  31 0018 0D00A0E1              mov     r0, sp
  32 001c 7C308DE5              str     r3, [sp, #124]
  33 0020 FEFFFFEB              bl      memcpy
  34 0024 70308DE2              add     r3, sp, #112
  35 0028 0F0093E8              ldm     r3, {r0, r1, r2, r3}
  36 002c FEFFFFEB              bl      foo
  37 0030 F0D08DE2              add     sp, sp, #240
  38                            @ sp needed
  39 0034 0F90FDE8              ldmfd   sp!, {r0, r1, r2, r3, ip, pc}^


In this case, the newlib version of memcpy uses NEON code and will clobber NEON
registers.

To be safe, the IRQ handler should
push {d0-d7, d16-d31}

This can be costly in terms of CPU cycles for an IRQ handler, so maybe we could
think of adding another attribute but it would be hard for the end-user to
guess that he might use Neon registers in an implicit way (like here where the
compiler calls memcpy)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
@ 2020-04-27 12:01 ` clyon at gcc dot gnu.org
  2020-04-27 17:04 ` clyon at gcc dot gnu.org
                   ` (24 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-27 12:01 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #1 from Christophe Lyon <clyon at gcc dot gnu.org> ---
clang has implemented a warning for this case:
https://reviews.llvm.org/D28820

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
  2020-04-27 12:01 ` [Bug target/94743] IRQ handler doesn't save scratch VFP registers clyon at gcc dot gnu.org
@ 2020-04-27 17:04 ` clyon at gcc dot gnu.org
  2020-04-28 11:40 ` clyon at gcc dot gnu.org
                   ` (23 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-27 17:04 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #2 from Christophe Lyon <clyon at gcc dot gnu.org> ---
I have a preliminary patch which generates:
        vpush.64        {d0, d1, d2, d3, d4, d5, d6, d7}
        vpush.64        {d16, d17, d18, d19, d20, d21, d22, d23, d24, d25, d26,
d27, d28, d29, d30, d31}

I'm not sure users would be happy with such long push sequences....

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
  2020-04-27 12:01 ` [Bug target/94743] IRQ handler doesn't save scratch VFP registers clyon at gcc dot gnu.org
  2020-04-27 17:04 ` clyon at gcc dot gnu.org
@ 2020-04-28 11:40 ` clyon at gcc dot gnu.org
  2020-04-28 11:42 ` clyon at gcc dot gnu.org
                   ` (22 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-28 11:40 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #3 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Maybe we could
- save the VFP registers as needed by default
- emit a warning "IRQ handler 'foo' saves VFP registers because it is not a
leaf function. If you know none of the callees will clobber the VFP registers
you can use the 'IRQ-dont-push-VFP-regs' attribute"
- implement this new attribute such that users can disable such long vpush
sequences

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (2 preceding siblings ...)
  2020-04-28 11:40 ` clyon at gcc dot gnu.org
@ 2020-04-28 11:42 ` clyon at gcc dot gnu.org
  2020-04-28 13:21 ` rearnsha at gcc dot gnu.org
                   ` (21 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-28 11:42 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

Christophe Lyon <clyon at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ktkachov at gcc dot gnu.org

--- Comment #4 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Adding Kyrylo so that he can share Arm's thoughts.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (3 preceding siblings ...)
  2020-04-28 11:42 ` clyon at gcc dot gnu.org
@ 2020-04-28 13:21 ` rearnsha at gcc dot gnu.org
  2020-04-28 14:52 ` clyon at gcc dot gnu.org
                   ` (20 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rearnsha at gcc dot gnu.org @ 2020-04-28 13:21 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #5 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
This is made more complex due to the fact that the existence of the top 16 D
registers depends on the hardware you have, so saving them might require a d32
variant of the ISA, but we can't (quickly) tell in an interrupt context whether
or not we have that.  It only matters in reality if the interrupt routine calls
a function in another translation unit where we can't see what FP registers
might be needed.

Also, it's not just the registers that need to be saved.  The floating point
status registers also need to be saved and restored.

My initial thoughts are along the lines of...
Only try to save FP registers that this function directly clobbers.
Provide libgcc routines to save/restore the FP context.

Or we could say simply:
interrupt routines should be compiled as if with -mgeneral-regs-only and if
they want to call some routine that uses FP then they must take it upon
themselves to save and restore the FP context.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (4 preceding siblings ...)
  2020-04-28 13:21 ` rearnsha at gcc dot gnu.org
@ 2020-04-28 14:52 ` clyon at gcc dot gnu.org
  2020-04-28 15:04 ` rearnsha at gcc dot gnu.org
                   ` (19 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-28 14:52 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #6 from Christophe Lyon <clyon at gcc dot gnu.org> ---
If we consider the initial testcase, it doesn't clobber any FP register
directly, but the compiler inserts a call to memcpy which does.

So IIUC your 1st suggestion, it would mean:
- save no FP register in the IRQ handler
- call a libgcc routine to save all FP registers+status registers (this routine
would have to decide about d16 vs d32 at runtime, unless we can rely on
multilibs -- it would mean defining mandatory d16 and d32 libgcc multilibs...)

Hence I like your simpler suggestion :-)
But I think we should help the user diagnose potential problems:

- maybe issue a warning when compiling an IRQ handler without
-mgeneral-regs-only. That might break some packages, but would force them to
check their code
- also emit a warning when calling a function defined in another unit (but we
should recurse)

However this does not help if the compiler inserts a call to memcpy which
happens to be using FPU code: the user would get a warning, but how could he
solve it? Avoid implicit use of memcpy?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (5 preceding siblings ...)
  2020-04-28 14:52 ` clyon at gcc dot gnu.org
@ 2020-04-28 15:04 ` rearnsha at gcc dot gnu.org
  2020-04-29 15:22 ` clyon at gcc dot gnu.org
                   ` (18 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rearnsha at gcc dot gnu.org @ 2020-04-28 15:04 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #7 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
well, __aeabi_memcpy is required not to clobber the FP state.  Sadly, GCC does
not know about it...

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (6 preceding siblings ...)
  2020-04-28 15:04 ` rearnsha at gcc dot gnu.org
@ 2020-04-29 15:22 ` clyon at gcc dot gnu.org
  2020-05-04 11:44 ` clyon at gcc dot gnu.org
                   ` (17 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-04-29 15:22 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

Christophe Lyon <clyon at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-04-29
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |clyon at gcc dot gnu.org

--- Comment #8 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Patch sent: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544872.html

This is a simple improvement, hopefully simple enough for stage 4, yet useful
for the end-users.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (7 preceding siblings ...)
  2020-04-29 15:22 ` clyon at gcc dot gnu.org
@ 2020-05-04 11:44 ` clyon at gcc dot gnu.org
  2020-05-04 14:52 ` rearnsha at gcc dot gnu.org
                   ` (16 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-04 11:44 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #9 from Christophe Lyon <clyon at gcc dot gnu.org> ---

> My initial thoughts are along the lines of...
> Only try to save FP registers that this function directly clobbers.
What's the point of saving these if a callee clobbers other registers?

Shouldn't that be something like save-nothing vs save-all-FP-regs if there is a
callee?

Do you mean save direct clobbers only when the handler is a leaf function?

> Provide libgcc routines to save/restore the FP context.
Do you mean such routines should push all FP regs+status regs?

> Or we could say simply:
> interrupt routines should be compiled as if with -mgeneral-regs-only and if
> they want to call some routine that uses FP then they must take it upon
> themselves to save and restore the FP context.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (8 preceding siblings ...)
  2020-05-04 11:44 ` clyon at gcc dot gnu.org
@ 2020-05-04 14:52 ` rearnsha at gcc dot gnu.org
  2020-05-04 16:19 ` clyon at gcc dot gnu.org
                   ` (15 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rearnsha at gcc dot gnu.org @ 2020-05-04 14:52 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #10 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #9)
> > My initial thoughts are along the lines of...
> > Only try to save FP registers that this function directly clobbers.
> What's the point of saving these if a callee clobbers other registers?
> 

They need to be done early enough to ensure that any code in *this* function
does not clobber them.  Any additional registers would have to be saved by a
library call that does that.

> Shouldn't that be something like save-nothing vs save-all-FP-regs if there
> is a callee?
> 
> Do you mean save direct clobbers only when the handler is a leaf function?

Well, obviously if it's a leaf function, saving only the registers that are
clobbered is enough, and the compiler can do the analysis to ensure that.

> 
> > Provide libgcc routines to save/restore the FP context.
> Do you mean such routines should push all FP regs+status regs?

All registers that are are call clobbered.  There's no need to do the
call-saved registers as the compiler can do that on an as-needed case already.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (9 preceding siblings ...)
  2020-05-04 14:52 ` rearnsha at gcc dot gnu.org
@ 2020-05-04 16:19 ` clyon at gcc dot gnu.org
  2020-05-04 16:23 ` rearnsha at gcc dot gnu.org
                   ` (14 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-04 16:19 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #11 from Christophe Lyon <clyon at gcc dot gnu.org> ---
(In reply to Richard Earnshaw from comment #10)
> (In reply to Christophe Lyon from comment #9)
> > > My initial thoughts are along the lines of...
> > > Only try to save FP registers that this function directly clobbers.
> > What's the point of saving these if a callee clobbers other registers?
> > 
> 
> They need to be done early enough to ensure that any code in *this* function
> does not clobber them.  Any additional registers would have to be saved by a
> library call that does that.
> 
Why do we need a library function for that? It would have to be special with
the stack: push FP registers, but do not restore SP, so that the dual restore
function can pop them and restore SP.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (10 preceding siblings ...)
  2020-05-04 16:19 ` clyon at gcc dot gnu.org
@ 2020-05-04 16:23 ` rearnsha at gcc dot gnu.org
  2020-05-04 18:26 ` clyon at gcc dot gnu.org
                   ` (13 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rearnsha at gcc dot gnu.org @ 2020-05-04 16:23 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #12 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #11)
> (In reply to Richard Earnshaw from comment #10)
> > (In reply to Christophe Lyon from comment #9)
> > > > My initial thoughts are along the lines of...
> > > > Only try to save FP registers that this function directly clobbers.
> > > What's the point of saving these if a callee clobbers other registers?
> > > 
> > 
> > They need to be done early enough to ensure that any code in *this* function
> > does not clobber them.  Any additional registers would have to be saved by a
> > library call that does that.
> > 
> Why do we need a library function for that? It would have to be special with
> the stack: push FP registers, but do not restore SP, so that the dual
> restore function can pop them and restore SP.

Because it's a lot of code to work out how many FP registers there are.  You
can't assume that the FPU used to compile the interrupt handler is the same as
that being used at run time.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (11 preceding siblings ...)
  2020-05-04 16:23 ` rearnsha at gcc dot gnu.org
@ 2020-05-04 18:26 ` clyon at gcc dot gnu.org
  2020-05-04 19:10 ` rearnsha at gcc dot gnu.org
                   ` (12 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-04 18:26 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #13 from Christophe Lyon <clyon at gcc dot gnu.org> ---

> > Why do we need a library function for that? It would have to be special with
> > the stack: push FP registers, but do not restore SP, so that the dual
> > restore function can pop them and restore SP.
> 
> Because it's a lot of code to work out how many FP registers there are.  You
> can't assume that the FPU used to compile the interrupt handler is the same
> as that being used at run time.

Ha, I had missed that point in your comment #5. I'll re-iterate on my WIP
patch.

But, in general (non-interrupt) code, what is supposed to happen if you compile
for a d32 VFP and run on d16 one ? (and the code uses the extra registers)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (12 preceding siblings ...)
  2020-05-04 18:26 ` clyon at gcc dot gnu.org
@ 2020-05-04 19:10 ` rearnsha at gcc dot gnu.org
  2020-05-05 10:06 ` clyon at gcc dot gnu.org
                   ` (11 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: rearnsha at gcc dot gnu.org @ 2020-05-04 19:10 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #14 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #13)

> But, in general (non-interrupt) code, what is supposed to happen if you
> compile for a d32 VFP and run on d16 one ? (and the code uses the extra
> registers)

Well obviously that won't work.  But if you build the interrupt routine with a
d16 system and then call a function from it that requires d32 then that should
still work if running on a d32 CPU.

I think we can probably make that work, but it's probably a bit of a dance to
get it all right.  Hence the suggestion that this be done in a library
function.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (13 preceding siblings ...)
  2020-05-04 19:10 ` rearnsha at gcc dot gnu.org
@ 2020-05-05 10:06 ` clyon at gcc dot gnu.org
  2020-05-05 13:34 ` clyon at gcc dot gnu.org
                   ` (10 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-05 10:06 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #15 from Christophe Lyon <clyon at gcc dot gnu.org> ---

> Well obviously that won't work.  But if you build the interrupt routine with
> a d16 system and then call a function from it that requires d32 then that
> should still work if running on a d32 CPU.

Thanks, I hadnt' thought of this combination.

> 
> I think we can probably make that work, but it's probably a bit of a dance
> to get it all right.  Hence the suggestion that this be done in a library
> function.

I suspect I'll have to iterate a few times to get that function right: I
haven't yet checked the interactions with the secure/non-secure modes,
LSPEN/ASPEN (I've noticed CMSE code in GCC that takes care of FP registers).

So what about adding a simple warning along the lines of comment #5 and comment
#6, like the one I posted (comment #8, but maybe it should also make sure to
call __aeabi_memcpy instead of memcpy?)

Then a second step would allow not to use -mgeneral-regs-only and save whatever
is needed. I am wondering whether we could introduce other attributes such as:
- "irq-nosave-fp-regs" basically saying the user does not want to save FP
registers; this would clear the warning
- "irq-save-fp-regs", asking the compiler to save all the needed regs despite
the penalty; this would also avoid the warning

At least that would make users think about their code, but we'd needed to
document that properly :-)

I've noticed that several existing tests fail because of my new warning if the
target defaults to float-abi=hard, depending on the default cpu/mode:

    gcc.misc-tests/arm-isr.c (test for excess errors)
    gcc.target/arm/empty_fiq_handler.c (test for excess errors)
    gcc.target/arm/interrupt-1.c (test for excess errors)
    gcc.target/arm/interrupt-2.c (test for excess errors)
    gcc.target/arm/pr70830.c (test for excess errors)

so I'd need to change their attribute or compile them with -mgeneral-regs-only

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (14 preceding siblings ...)
  2020-05-05 10:06 ` clyon at gcc dot gnu.org
@ 2020-05-05 13:34 ` clyon at gcc dot gnu.org
  2020-05-13 15:24 ` clyon at gcc dot gnu.org
                   ` (9 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-05 13:34 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #16 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Another potential issue just came to my mind: what if the IRQ handler is
compiled with -mfloat-abi=soft but calls a function compiled with
-mfloat-abi=softfp? We have no way to guess that the FP registers can be
clobbered when compiling the handler.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (15 preceding siblings ...)
  2020-05-05 13:34 ` clyon at gcc dot gnu.org
@ 2020-05-13 15:24 ` clyon at gcc dot gnu.org
  2020-05-14  8:16 ` clyon at gcc dot gnu.org
                   ` (8 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-13 15:24 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #17 from Christophe Lyon <clyon at gcc dot gnu.org> ---
(In reply to Richard Earnshaw from comment #10)
> (In reply to Christophe Lyon from comment #9)
> > > My initial thoughts are along the lines of...
> > > Only try to save FP registers that this function directly clobbers.
> > What's the point of saving these if a callee clobbers other registers?
> > 
> 
> They need to be done early enough to ensure that any code in *this* function
> does not clobber them.  Any additional registers would have to be saved by a
> library call that does that.

So if this function clobbers, say d16-d17, but calls another function, do you
mean we should
vpush d16-d17
then call the new lib function which saves all the FP context (thus saving
d16-d17 twice)?

> 
> > Shouldn't that be something like save-nothing vs save-all-FP-regs if there
> > is a callee?
> > 
> > Do you mean save direct clobbers only when the handler is a leaf function?
> 
> Well, obviously if it's a leaf function, saving only the registers that are
> clobbered is enough, and the compiler can do the analysis to ensure that.

I'm working on this, and just realized that this also means saving FPSR. It
seems there's no support for that yet in arm.md (unlike aarch64.md), am I
missing something?

> 
> > 
> > > Provide libgcc routines to save/restore the FP context.
> > Do you mean such routines should push all FP regs+status regs?
> 
> All registers that are are call clobbered.  There's no need to do the
> call-saved registers as the compiler can do that on an as-needed case
> already.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (16 preceding siblings ...)
  2020-05-13 15:24 ` clyon at gcc dot gnu.org
@ 2020-05-14  8:16 ` clyon at gcc dot gnu.org
  2020-05-14 15:20 ` clyon at gcc dot gnu.org
                   ` (7 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-14  8:16 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #18 from Christophe Lyon <clyon at gcc dot gnu.org> ---

> I'm working on this, and just realized that this also means saving FPSR. It
> seems there's no support for that yet in arm.md (unlike aarch64.md), am I
> missing something?
> 

Sorry, I see it's called FPSCR for arm.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (17 preceding siblings ...)
  2020-05-14  8:16 ` clyon at gcc dot gnu.org
@ 2020-05-14 15:20 ` clyon at gcc dot gnu.org
  2020-06-08  8:19 ` cvs-commit at gcc dot gnu.org
                   ` (6 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-05-14 15:20 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #19 from Christophe Lyon <clyon at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #8)
> Patch sent: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544872.html
> 
> This is a simple improvement, hopefully simple enough for stage 4, yet
> useful for the end-users.

I have just sent an updated version of this patch:
https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html

Maybe that would be sufficient to consider this PR fixed?

Indeed I fear we open a can of worms, as I've also realized that at least some
Cortex-M cores save part of the FP registers when taking an interruption, the
number depends on several parameters (eg secure/non-secure, ... see "Exception
entry in Cortex-M33 GUG for instance
https://static.docs.arm.com/100235/0002/arm_cortex_m33_dgug_100235_0002_00_en.pdf)

I haven't found such documentation for Cortex-A, so I'm not sure if they have
the same behaviour.

I have attached a WIP patch that demonstrates local saving of FP registers as
an attachment to https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545754.html

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (18 preceding siblings ...)
  2020-05-14 15:20 ` clyon at gcc dot gnu.org
@ 2020-06-08  8:19 ` cvs-commit at gcc dot gnu.org
  2020-06-30 14:23 ` cvs-commit at gcc dot gnu.org
                   ` (5 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-06-08  8:19 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Christophe Lyon <clyon@gcc.gnu.org>:

https://gcc.gnu.org/g:635408da1eb1d441ef4d59fe00a038c920e51085

commit r11-1062-g635408da1eb1d441ef4d59fe00a038c920e51085
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Mon Jun 8 08:17:20 2020 +0000

    [arm] Fix vfp_operand_register for VFP HI regs

    While looking at PR target/94743 I noticed an ICE when I tried to save
    all the FP registers: this was because all HI registers wouldn't match
    vfp_register_operand.

    gcc/ChangeLog:

            * config/arm/predicates.md (vfp_register_operand): Use VFP_HI_REGS
            instead of VFP_REGS.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (19 preceding siblings ...)
  2020-06-08  8:19 ` cvs-commit at gcc dot gnu.org
@ 2020-06-30 14:23 ` cvs-commit at gcc dot gnu.org
  2020-06-30 14:35 ` clyon at gcc dot gnu.org
                   ` (4 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-06-30 14:23 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #21 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Christophe Lyon <clyon@gcc.gnu.org>:

https://gcc.gnu.org/g:3c3b4224875d7b8bfd4126b9dd1d9cb028997285

commit r11-1732-g3c3b4224875d7b8bfd4126b9dd1d9cb028997285
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Tue Jun 30 13:51:20 2020 +0000

    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 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-06-30  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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (20 preceding siblings ...)
  2020-06-30 14:23 ` cvs-commit at gcc dot gnu.org
@ 2020-06-30 14:35 ` clyon at gcc dot gnu.org
  2020-07-01  6:55 ` cvs-commit at gcc dot gnu.org
                   ` (3 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-06-30 14:35 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #22 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Not sure if we can close this PR: I have only implemented a part of what we
discussed here. GCC now emits a warning so the user can take action to make
sure his code is correct/correctly generated, but GCC does not handle
saving/restoring all of the FP registers automatically.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (21 preceding siblings ...)
  2020-06-30 14:35 ` clyon at gcc dot gnu.org
@ 2020-07-01  6:55 ` cvs-commit at gcc dot gnu.org
  2020-07-01 12:29 ` cvs-commit at gcc dot gnu.org
                   ` (2 subsequent siblings)
  25 siblings, 0 replies; 27+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-07-01  6:55 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #23 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Christophe Lyon <clyon@gcc.gnu.org>:

https://gcc.gnu.org/g:2f3fd53220b74d834d300e0b7aa99eca039ffbea

commit r11-1752-g2f3fd53220b74d834d300e0b7aa99eca039ffbea
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Wed Jul 1 06:48:17 2020 +0000

    arm: Fix typos in testcases [PR target/94743]

    In my commit r11-1732, I updated the warning message to include
    quotes, but I forgot to update the testcases.

    2020-01-07  Christophe Lyon  <christophe.lyon@linaro.org>

            PR target/94743

            gcc/testsuite/
            * gcc.target/arm/pr94743-1-hard.c: Add missing quotes in expected
            warning.
            * gcc.target/arm/pr94743-1-softfp.c: Likewise.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (22 preceding siblings ...)
  2020-07-01  6:55 ` cvs-commit at gcc dot gnu.org
@ 2020-07-01 12:29 ` cvs-commit at gcc dot gnu.org
  2020-12-04 17:02 ` clyon at gcc dot gnu.org
  2021-05-04 12:26 ` rguenth at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-07-01 12:29 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

--- Comment #24 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Christophe Lyon <clyon@gcc.gnu.org>:

https://gcc.gnu.org/g:aa8b5ca0b540fde5890f3114f2d7076d5238fc2e

commit r11-1759-gaa8b5ca0b540fde5890f3114f2d7076d5238fc2e
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Wed Jul 1 12:23:51 2020 +0000

    arm: Fix handler-align.c testcase [PR target/94743]

    This testcase triggers the new warning, so compile it with
    -mgeneral-regs-only.

    2020-07-01  Christophe Lyon  <christophe.lyon@linaro.org>

            PR target/94743

            gcc/testsuite/
            * gcc.target/arm/handler-align.c: Add -mgeneral-regs-only.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (23 preceding siblings ...)
  2020-07-01 12:29 ` cvs-commit at gcc dot gnu.org
@ 2020-12-04 17:02 ` clyon at gcc dot gnu.org
  2021-05-04 12:26 ` rguenth at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: clyon at gcc dot gnu.org @ 2020-12-04 17:02 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

Christophe Lyon <clyon at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|clyon at gcc dot gnu.org           |unassigned at gcc dot gnu.org

--- Comment #25 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Un-assigning myself: I do not plan to work more on this for the moment.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Bug target/94743] IRQ handler doesn't save scratch VFP registers
  2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
                   ` (24 preceding siblings ...)
  2020-12-04 17:02 ` clyon at gcc dot gnu.org
@ 2021-05-04 12:26 ` rguenth at gcc dot gnu.org
  25 siblings, 0 replies; 27+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-05-04 12:26 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94743

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2021-05-04 12:26 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-24 11:59 [Bug target/94743] New: IRQ handler implementation wrong when using __attribute__ ((interrupt("IRQ"))) clyon at gcc dot gnu.org
2020-04-27 12:01 ` [Bug target/94743] IRQ handler doesn't save scratch VFP registers clyon at gcc dot gnu.org
2020-04-27 17:04 ` clyon at gcc dot gnu.org
2020-04-28 11:40 ` clyon at gcc dot gnu.org
2020-04-28 11:42 ` clyon at gcc dot gnu.org
2020-04-28 13:21 ` rearnsha at gcc dot gnu.org
2020-04-28 14:52 ` clyon at gcc dot gnu.org
2020-04-28 15:04 ` rearnsha at gcc dot gnu.org
2020-04-29 15:22 ` clyon at gcc dot gnu.org
2020-05-04 11:44 ` clyon at gcc dot gnu.org
2020-05-04 14:52 ` rearnsha at gcc dot gnu.org
2020-05-04 16:19 ` clyon at gcc dot gnu.org
2020-05-04 16:23 ` rearnsha at gcc dot gnu.org
2020-05-04 18:26 ` clyon at gcc dot gnu.org
2020-05-04 19:10 ` rearnsha at gcc dot gnu.org
2020-05-05 10:06 ` clyon at gcc dot gnu.org
2020-05-05 13:34 ` clyon at gcc dot gnu.org
2020-05-13 15:24 ` clyon at gcc dot gnu.org
2020-05-14  8:16 ` clyon at gcc dot gnu.org
2020-05-14 15:20 ` clyon at gcc dot gnu.org
2020-06-08  8:19 ` cvs-commit at gcc dot gnu.org
2020-06-30 14:23 ` cvs-commit at gcc dot gnu.org
2020-06-30 14:35 ` clyon at gcc dot gnu.org
2020-07-01  6:55 ` cvs-commit at gcc dot gnu.org
2020-07-01 12:29 ` cvs-commit at gcc dot gnu.org
2020-12-04 17:02 ` clyon at gcc dot gnu.org
2021-05-04 12:26 ` rguenth at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).