Re: [avr-gcc-list] avr-libc: interrupt.h, ISR and ISR_NOBLOCK

[Georg-Johann Lay]

Weddington, Eric schrieb:
> Johann Lay
> > AVR-LIbC's ISR-macros lead to confusing results for code like, e.g.
> >
> > #include <avr/io.h>
> > #include <avr/interrupt.h>
> >
> > void *p;
> >
> > ISR (ADC_vect, ISR_NOBLOCK)
> > {
> >     char c[10];
> >     p = c;
> > }
> >
> > which resolves a.a. to
> >
> > void __vector_21 (void)
> > __attribute__ ((signal,used,externally_visible)) __attribute__((interrupt));
> >
> >
> > Which states that IRQs are turned off (signal) and IRQs are turned on
> > (interrupt) at the same time.
>
>
> I haven't looked at the avr-libc code in a long time; I didn't know
> it actually generated that. :-/
>
> I agree that it shouldn't do that, and it should generate clean code.
>
> > This leads to unexpected code generated by the compiler.

BTW, the code for ATmega is like

        sei
        push r1
        push r0
        in r0,__SREG__
        push r0
        clr __zero_reg__
        push r24
        push r25
        push r28
        push r29
        in r28,__SP_L__
        in r29,__SP_H__
        sbiw r28,10
        out __SP_H__,r29
        out __SP_L__,r28
/* prologue: Interrupt */
/* frame size = 10 */
/* stack size = 17 */
.L__stack_usage = 17
        movw r24,r28
        adiw r24,1
        sts p+1,r25
        sts p,r24
/* epilogue start */
        adiw r28,10
        in __tmp_reg__,__SREG__
        cli
        out __SP_H__,r29
        out __SREG__,__tmp_reg__
        out __SP_L__,r28
        pop r29
        pop r28
        pop r25
        pop r24
        pop r0
        out __SREG__,r0
        pop r0
        pop r1
        reti

The assignment at the end of prologue is not IRQ-safe because the
function is "signal".

"interrupt" just says that "interrupts will be enabled inside the
function" but IMO it's a reasonable assumption that the compiler
generates according code.

The second set of SP is IRQ-save because I changed the code to
be more conservative because there is no knowledge if the
used did SEI by hand.

This leads to 3 more instructions, but to definitely know a new
function attribute would be needed like "no_sei" or "no_irq"
where the user ensures that he never sets I-flag, at least not
an a way that affects writes to SP. E.g. code like yield() as in

   SEI
   NOP
   CLI

would not require "no_sei" in a "signal" as it cannot affect SP atomicy.

"no_sei" would be incompatible with "OS_task" and "interrupt".

Or maybe it's better to name it "no_interrupt[s]" that is similar to
-mno-interrupts command option.

> > Thus, I'd like to introduce an error in avr-gcc saying
> >
> > "function can only be one of 'interrupt', 'signal', 'OS_task', 'OS_main'
> > at the same time"
>
> What should OS_task and OS_main be? OS_main as uninterruptable?
> OS_task as interruptable?

It's basically as in
   http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
which I found as mentioned in
   http://gcc.gnu.org/PR49824
But as I read it now, there is a wrong "call-saved".

> I have no problem with throwing an error in avr-gcc, as long as
> we get avr-libc fixed at the same time.
>
> Eric

I just don't see a trick how to add "signal" only if NO_INTERRUPT
is not specified.

The compiler could be changed to handle it, of course.
But I am no fan of trying to support mutually exclusive/contradicting
things...

Johann