bug: OS_Yield destroys gcc's call saved registers

[lattenzaun]

Hello,

I am using salvo lite for Atmel AVR v3.2.3 with avr-gcc3.3.1. When calling OS_Yield register r29 becomes changed. R29 is a "call saved register" and may be allocated by gcc for local data. All subroutines have to leave them unchanged.
Here is an example to get the bug:

code:

---

void TaskCount( void )
{
static unsigned int its_wrong;
  for (;;) {
    counter++;
    if ((counter % 2) == 0)      //here r29 becomes 0
    {
      OS_Yield(TaskCount99);     //this destroys r29!!!! r29=4 why???
      its_wrong = 0;             //gcc assumes r29 is 0 but is 4 and its_wrong becomes 1024!!!
    }
  }
}

---

I would be grateful for some help or bug-fix!

Regards,
Martin

[This message has been edited by aek (edited December 19, 2003).]

[aek]

What happens if you disable optimizations in the task?

I'll take a look at it ...

------------------

[Salvo Tech Support]

Hi Martin.

Formally registered as a bug, with a Service Bulletin to fix it here: SB-21.

------------------
--------
Salvo Technical Support
Please request all tech support through the Forums.

[lattenzaun]

Hello Aek,

Thanks a lot for your quick answer and for the workaround. I would not recommend to use this workaround because the bug is still there but now it is no "acute bug" but it is a "latent bug"! Your suggested change in the context-switcher just forces gcc not to use r28 and r29 but to use the zero-register r1. It is necessary to leave r28 and r29 unchanged with each function-call like OSCtxSw is. I am afraid of bugs when destroying these registers! Please see avr-libc manual about "call-saved registers".

I am looking forward having a bug-fix for all avr-gcc-salvo users in v3.2.4

Thanks in advance and Best Regards,
Martin

[This message has been edited by lattenzaun (edited December 19, 2003).]

[aek]

Hi Martin.

The solution proposed in SB-21 is AFAIK a complete solution. If you can generate another test case that shows R28 and/or R29 being used with corrupted values in the function after a Salvo context switch, I'd be very interested to see it.

The key to solving your bug did not have to do with call-saved registers, even though it might appear to be the issue. Without going too deeply into Salvo's internal workings, the R28/R29 pair (used as the Y register when a local stack frame exists) do not need to be preserved by Salvo as long as the compiler is prevented from using them in the Salvo task. That's what the revised OS_Yield() accomplishes.

There is one case (a local stack frame active in main()) which will probably break the existing scheme, even with the new OS_Yield(). I have revised portgccavr.S to fix that, and it will be included in the next release.

Regards,

------------------

[aek]

Hi Martin.

I forgot to mention -- any Salvo Pro + AVR-GCC users who want the revised portgccavr.S to use in the interim can email support and we'll send the file.

We are waiting to complete support for IAR's AVR compiler before releaseing v3.2.4.

------------------

[aek]

Hi Martin.

Here's a very quick solution: change

code:

---

#define OS_Yield(label)            		do { OSCtxSw(); } while (0)

---

in salvoincportgccavr.h to

code:

---

#define OS_Yield(label)            		do { OSCtxSw(); asm volatile(" nop"::); } while (0)

---

Instead of

code:

---

+00000029:   93CF        PUSH    R28              Push register on stack
+0000002A:   93DF        PUSH    R29              Push register on stack
35:               counter++;
+0000002B:   91800062    LDS     R24,0x0062       Load direct from data space
+0000002D:   91900063    LDS     R25,0x0063       Load direct from data space
+0000002F:   9601        ADIW    R24,0x01         Add immediate to word
+00000030:   93900063    STS     0x0063,R25       Store direct to data space
+00000032:   93800062    STS     0x0062,R24       Store direct to data space
37:               if ((counter %2) == 0)
+00000034:   2FD9        MOV     R29,R25          Copy register
+00000035:   2FC8        MOV     R28,R24          Copy register
+00000036:   70C1        ANDI    R28,0x01         Logical AND with immediate
+00000037:   70D0        ANDI    R29,0x00         Logical AND with immediate
+00000038:   FD80        SBRC    R24,0            Skip if bit in register cleared
+00000039:   CFF1        RJMP    -0x000F          Relative jump
39:       		    OS_Yield(TaskCount1);
+0000003A:   D1B6        RCALL   +0x01B6          Relative call subroutine
40:       	        its_wrong = 0;
+0000003B:   93D00061    STS     0x0061,R29       Store direct to data space
+0000003D:   93C00060    STS     0x0060,R28       Store direct to data space
34:           for (;;) {
+0000003F:   CFEB        RJMP    -0x0015          Relative jump

---

you'll get

code:

---

35:               counter++;
+00000029:   91800062    LDS     R24,0x0062       Load direct from data space
+0000002B:   91900063    LDS     R25,0x0063       Load direct from data space
+0000002D:   9601        ADIW    R24,0x01         Add immediate to word
+0000002E:   93900063    STS     0x0063,R25       Store direct to data space
+00000030:   93800062    STS     0x0062,R24       Store direct to data space
37:               if ((counter %2) == 0)
+00000032:   FD80        SBRC    R24,0            Skip if bit in register cleared
+00000033:   CFF5        RJMP    -0x000B          Relative jump
39:       		    OS_Yield(TaskCount1);
+00000034:   D1B8        RCALL   +0x01B8          Relative call subroutine
+00000035:   0000        NOP                      No operation
40:       	        its_wrong = 0;
+00000036:   92100061    STS     0x0061,R1        Store direct to data space
+00000038:   92100060    STS     0x0060,R1        Store direct to data space
34:           for (;;) {
+0000003A:   CFEE        RJMP    -0x0012          Relative jump

---

which is smaller and correct, but may be slower.

Sorry about the inconvenience -- there may be an opportunity to further optimize the context switcher, and I'm looking at it right now ...

------------------

[This message has been edited by aek (edited December 19, 2003).]

[aek]

I've placed the patch files in the Pro download directory.

------------------