stack update is not interrupt-safe?

Go To Last Post
9 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm trying to chase down a bug in my C code for the AVR Atmega1284. Every so often, it looks like I may be getting a corrupted stack.

Looking at the compiler-generated code, I see lots of blocks like this:

     95a:	cd b7       	in	r28, 0x3d	; 61
     95c:	de b7       	in	r29, 0x3e	; 62
     95e:	ac 97       	sbiw	r28, 0x2c	; 44
     960:	0f b6       	in	r0, 0x3f	; 63
     962:	f8 94       	cli
     964:	de bf       	out	0x3e, r29	; 62
     966:	0f be       	out	0x3f, r0	; 63
     968:	cd bf       	out	0x3d, r28	; 61

I think this is loading the 16-bit stack pointer from port 0x3d-3e, subtracting 0x2C (reserving space on the stack), then writing the result back. Along the way, it also reads the current status register from port 0x3f, then disables interrupts, and later restores the saved status.

But isn't the timing of the status register update wrong in this code? It restores interrupts (second to last line) before it writes the second byte of the stack pointer (last line). Why doesn't it write both stack pointer bytes, and *then* restore the saved status?

If an interrupt is already pending when the saved status is restored, the interrupt handler could be invoked before the stack pointer is completely updated, causing it to write to unexpected locations in RAM when the handler uses the stack. Which is very much like the behavior I'm seeing.

This is with AVR studio 5, which I believe is using avr-gcc 3.3.1.27.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

But isn't the timing of the status register update wrong in this code? It restores interrupts (second to last line) before it writes the second byte of the stack pointer (last line). Why doesn't it write both stack pointer bytes, and *then* restore the saved status?

If an interrupt is already pending when the saved status is restored, the interrupt handler could be invoked before the stack pointer is completely updated, causing it to write to unexpected locations in RAM when the handler uses the stack. Which is very much like the behavior I'm seeing.


The compiler is making use of an AVR "feature". (That feature might just have been put there for this purpose.) Find a datasheet and this in the "Reset and Interrupt Handling" section:
Quote:
When using the SEI instruction to enable interrupts, the instruction following SEI will be executed before any pending interrupts, as shown in this example.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There is always one instruction executed after sei before the interrupts are enabled, so that is OK.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Got it, thanks. The datasheet says that about the SEI instruction, but doesn't say anything (that I found) about a programmatic update of SREG from a register, as this code is doing here. Should I assume this 1-slot delay rule applies any time interrupts are enabled, regardless of the method used to enable them?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

but doesn't say anything (that I found) about a programmatic update of SREG from a register, as this code is doing here.

Quote:

..., as shown in this example.

Didja see any explicit SEI in the code examples? But agreed that it is a good question and might not be explicit in the datasheet.

Remember that SEI isn't an "instruction", but rather a name for a particular case of "set bit in status register". So then your question becomes more of "Is there a difference between using that mechanism vs. OUT?"

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, I'll have to look for the source of my stack corruption elsewhere. :-) But just to clarify:

theusch wrote:

Didja see any explicit SEI in the code examples? But agreed that it is a good question and might not be explicit in the datasheet.

Yes, the datasheet example explicitly says SEI, not something more general like "enabling interrupts":

When using the SEI instruction to enable interrupts, the instruction following SEI will be executed before any pending interrupts, as shown in this example.

sei ; set Global Interrupt Enable
sleep; enter sleep, waiting for interrupt
; note: will enter sleep before any pending 
; interrupt(s)
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The datasheet example uses an OUT.

SEI (SEC, SEV, ...) are variations of BSET instruction.

So, again, your question boils down to "Is there a difference between setting an status register flag with BSET vs. OUT?" I wouldn't think there would be. Atmel's own examples use both.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Avr studio 5 is known to be defective. Upgrade now!
You're not doing nested interrupts? That is reenabling interrupts whilst in an isr? Whilst perfectly legal, it can have some side effects that will blow your stack if you're not aware of the implications.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
The datasheet example uses an OUT.
You and I have different versions of the datasheets ;)

This is the full text of the relevant section from the 48/88/168/328 datasheet:

Quote:
When using the CLI instruction to disable interrupts, the interrupts will be immediately disabled. No interrupt will be executed after the CLI instruction, even if it occurs simultaneously with the CLI instruction. The following example shows how this can be used to avoid interrupts during the timed EEPROM write sequence.
 Assembly Code Example
---------------------
     in r16, SREG    ; store SREG value
     cli   ; disable interrupts during timed sequence
     sbi EECR, EEMPE ; start EEPROM write
     sbi EECR, EEPE
     out SREG, r16   ; restore SREG value (I-bit)
     
C Code Example
--------------
     char cSREG;
     cSREG = SREG; /* store SREG value */
     /* disable interrupts during timed sequence */
     _CLI();
     EECR |= (1<<EEMPE); /* start EEPROM write */
     EECR |= (1<<EEPE);
     SREG = cSREG; /* restore SREG value (I-bit) */

When using the SEI instruction to enable interrupts, the instruction following SEI will be executed before any pending interrupts, as shown in this example.

Assembly Code Example
---------------------
   sei  ; set Global Interrupt Enable
   sleep; enter sleep, waiting for interrupt
   ; note: will enter sleep before any pending interrupt(s)
   
C Code Example
--------------
   __enable_interrupt(); /* set Global Interrupt Enable */
   __sleep(); /* enter sleep, waiting for interrupt */
   /* note: will enter sleep before any pending interrupt(s) */

To be fair, the text referring to SEI and the example code using it straddle a page break, and it's easy to concluded that the text regarding SEI is referring to the code example for CLI above it.
Quote:
So, again, your question boils down to "Is there a difference between setting an status register flag with BSET vs. OUT?"
STS, STD, and the various forms of ST can also affect the status register and its bits (although why you would ever use them, I can't imagine). Even PUSH has to potential to change SREG, provided the stack pointer points to 0x5F.
Quote:
I wouldn't think there would be. Atmel's own examples use both.
The 'one instruction' rule indeed applies to all methods for setting 'I' in SREG.

To the OP, if you're curious, you can easily contrive a test program.

JJ

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]