Page 1 of 1

Toolchain and abi

Posted: Fri Oct 10, 2014 9:43 am
by lilvinz
Hi there,

which toolchain is recommended for the AT91SAM7 port?
I am trying to migrate a legacy project and i am usually using this toolchain: https://launchpad.net/gcc-arm-embedded
However when i try that using -mcpu=arm7tdmi the issue arises that their multilib has not been built with -mabi=gnu-apcs.
So the second question is, does the port rely on -mabi=gnu-apcs? In the demo makefile this is only enabled for optimized code.

Thanks and regards,

Vinz

Re: Toolchain and abi

Posted: Fri Oct 10, 2014 10:24 am
by Giovanni
The port should not be reliant on that.

You may also try YAGARTO.

Giovanni

Re: Toolchain and abi

Posted: Fri Oct 10, 2014 3:34 pm
by lilvinz
Thanks for the quick reply.
My issue is that in ISR context, registers used in threads are being overwritten and i don't see the full picture of why this happens:
- I am using ARM mode only
- The port defines ISR as naked so gcc doesn't do anything on entry / exit.
- The cpu itself unlike e.g. cortex-m3 doesn't save anything except program counter on exception entry.
- The ChibiOS ISR prologue saves r0-r3, r12, lr
- The ChibiOS ISR epilogue restores r0-r3, r12, lr (within _port_irq_common)

But what happens if the code within the ISR between prologue and epilogue uses r5 to r11?
That is what happens for me.
r5 gets clobbered within the isr code and thus is wrong within thread context after the ISR has returned.

When i add saving of r4-r11 within ISR, the issue seems to be fixed but i am sure this is not the right way:

Code: Select all

static CH_IRQ_HANDLER(SYSIrqHandler)
{
    CH_IRQ_PROLOGUE();

    asm("push    {r4, r5, r6, r7}\n");

    if (AT91C_BASE_PITC->PITC_PISR & AT91C_PITC_PITS)
    {
        (void)AT91C_BASE_PITC->PITC_PIVR;
        chSysLockFromIsr();
        chSysTimerHandlerI();
        chSysUnlockFromIsr();
    }

    AT91C_BASE_AIC->AIC_EOICR = 0;

    asm("pop     {r4, r5, r6, r7}\n");

    CH_IRQ_EPILOGUE();
}


Any idea what i am doing wrong here?

Thanks!

Vinz

edit:
I understand now, that saving the registers r4+ is up to the callee. (https://www.cl.cam.ac.uk/~fms27/teachin ... t/apcs.txt)
I can see in the disassembly that when using -flto void chSysTimerHandlerI(void) does not save its registers.
Disabling -flto fixes the issue so it must be another -flto related bug. Time to stop using that feature. :(

Re: Toolchain and abi

Posted: Fri Oct 10, 2014 5:14 pm
by Giovanni
The ARM port ISRs should just call another function, if you want to use global inlining mark the function as no-inline using a GCC attribute.

The ARM port in 3.0 will not suffer of this limitation, it is work in progress.

Giovanni

Re: Toolchain and abi

Posted: Fri Oct 10, 2014 8:14 pm
by JetForMe
This has some ramifications for using clang/LLVM, too, which doesn't support "naked" functions with anything other than asm code in them.

Re: Toolchain and abi

Posted: Sat Oct 11, 2014 10:58 pm
by lilvinz
Giovanni wrote:The ARM port ISRs should just call another function, if you want to use global inlining mark the function as no-inline using a GCC attribute.

The ARM port in 3.0 will not suffer of this limitation, it is work in progress.

Giovanni


How can i guarantee that the compiler doesn't use registers > r3 within an interrupt handler?

e.g. when i compile the ARM7-AT91SAM7X-GCC demo using -Os instead of -O2 the SYSIrqHandler clobbers r4:

Code: Select all

00107a50 <SYSIrqHandler>:
/*
 * SYS IRQ handling here.
 */
static CH_IRQ_HANDLER(SYSIrqHandler) {

  CH_IRQ_PROLOGUE();
  107a50:       e92d500f        push    {r0, r1, r2, r3, ip, lr}

  if (AT91C_BASE_PITC->PITC_PISR & AT91C_PITC_PITS) {
  107a54:       e3e04000        mvn     r4, #0
  107a58:       e51432cb        ldr     r3, [r4, #-715] ; 0x2cb
  107a5c:       e3130001        tst     r3, #1
    (void) AT91C_BASE_PITC->PITC_PIVR;
  107a60:       151432c7        ldrne   r3, [r4, #-711] ; 0x2c7
    chSysLockFromIsr();
    chSysTimerHandlerI();
  107a64:       1bffe245        blne    100380 <chSysTimerHandlerI>
    chSysUnlockFromIsr();
  }
 
#if USE_SAM7_DBGU_UART
  if (AT91C_BASE_DBGU->DBGU_CSR &
  107a68:       e5143deb        ldr     r3, [r4, #-3563]        ; 0xdeb
  107a6c:       e31300e7        tst     r3, #231        ; 0xe7
      (AT91C_US_RXRDY | AT91C_US_TXRDY | AT91C_US_PARE | AT91C_US_FRAME |
       AT91C_US_OVRE | AT91C_US_RXBRK)) {
    sd_lld_serve_interrupt(&SDDBG);
  107a70:       159f0010        ldrne   r0, [pc, #16]   ; 107a88 <SYSIrqHandler+0x38>
  107a74:       1bffff39        blne    107760 <sd_lld_serve_interrupt>
  }
#endif
  AT91C_BASE_AIC->AIC_EOICR = 0;
  107a78:       e3a02000        mov     r2, #0
  107a7c:       e3e03000        mvn     r3, #0
  107a80:       e5032ecf        str     r2, [r3, #-3791]        ; 0xecf
  CH_IRQ_EPILOGUE();
  107a84:       eaffe1a5        b       100120 <_port_irq_common>
  107a88:       00200a3c        .word   0x00200a3c
  107a8c:       00000000        .word   0x00000000


I can work around that by adding a trampoline function with __attribute__((noinline)) but what about all the other interrupt handlers in the hal?
But working around in this way is no guarantee that registers are not being clobbered if the compiler sees fit, it just makes it unlikely.

Cheers,

Vinz

Re: Toolchain and abi

Posted: Sat Oct 11, 2014 11:00 pm
by Giovanni
It is guaranteed if you just call a function from the ISR and you don't have automatic variables in the ISR that live across the function call.

In 3.0 it will work differently.

Giovanni

Re: Toolchain and abi

Posted: Sat Oct 11, 2014 11:06 pm
by lilvinz
Giovanni wrote:It is guaranteed if you just call a function from the ISR and you don't have automatic variables in the ISR that live across the function call.


I am not convined on that one. Currently all the hal lld_serve_interrupt functions are not __attribute__((noinline)) and thus the compiler can inline them as it sees fit and then clobber registers as it sees fit.

e.g. using this code when building with -Os gets inlined and thus r4 gets clobbered:

Code: Select all

static void serve_system_interrupt(void)
{
    if (AT91C_BASE_PITC->PITC_PISR & AT91C_PITC_PITS)
    {
        (void)AT91C_BASE_PITC->PITC_PIVR;
        chSysLockFromIsr();
        chSysTimerHandlerI();
        chSysUnlockFromIsr();
    }

#if USE_SAM7_DBGU_UART
    if (AT91C_BASE_DBGU->DBGU_CSR &
            (AT91C_US_RXRDY | AT91C_US_TXRDY | AT91C_US_PARE |
                    AT91C_US_FRAME | AT91C_US_OVRE | AT91C_US_RXBRK))
    {
        sd_lld_serve_interrupt(&SDDBG);
    }
#endif
}

static CH_IRQ_HANDLER(SYSIrqHandler)
{
    CH_IRQ_PROLOGUE();

    serve_system_interrupt();

    AT91C_BASE_AIC->AIC_EOICR = 0;

    CH_IRQ_EPILOGUE();
}


I can only fix that by adding __attribute__((noinline)) to serve_system_interrupt().
I assume the compilers inlines it because there is only one caller. This can very well also be the case for hal irq handlers.

Re: Toolchain and abi

Posted: Sat Oct 11, 2014 11:17 pm
by lilvinz
Ok, i see now that some serve_interrupt functions in the hal have __attribute__((noinline)).

The following do:
- mac_lld.c serve_interrupt()
- pwm_lld.c pwm_lld_serve_interrupt()
- serial_lld.c sd_lld_serve_interrupt()
- spi_lld.c spi_lld_serve_interrupt()

The following don't:
- adc_lld.c handleint()
- ext_lld.c ext_lld_serveInterrupt()
- gpt_lld.c gpt_lld_serve_interrupt()
- i2c_lld.c i2c_lld_serve_interrupt()

Is this intentional?

Cheers,

Vinz

Re: Toolchain and abi

Posted: Sun Oct 12, 2014 8:10 am
by Giovanni
It is required, probably the requirement was not clear to the contributors.

Giovanni