This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

MSP430-GCC-OPENSOURCE: __get_interrupt_state() / __set_interrupt_state() may clobber condition and LPM flags

Part Number: MSP430-GCC-OPENSOURCE

According to manuals, the TI sanctioned way of disabling and reenabling interrupts for critical sections is:

void CriticalFn()
{
    __istate_t s = __get_interrupt_state();
    __disable_interrupt();
    /* Do something here. */
    __set_interrupt_state(s);
}

In in430.h, these intrinsics are implemented as inline ASM:

#define _get_interrupt_state() \
({ \
	unsigned int __x; \
	__asm__ __volatile__( \
	"mov SR, %0" \
	: "=r" ((unsigned int) __x) \
	:); \
	__x; \
})

#define _set_interrupt_state(x) \
({ \
    __asm__ __volatile__ ("nop { mov %0, SR { nop" \
        : : "ri"((unsigned int) x) \
    );\
})

As one can see, "s" receives the value of the whole status register, not the GIE flag exclusively. Thus, a subsequent _set_interrupt_state() will destroy condition flags, that have changed inside the critical section. This is bad, because assumptions the optimiser makes about condition flags are no longer valid. And even worse, LPM flags (SCG0, SCG1) that were changed inside the critical section are overwritten.

Proposed fix:

Add a

"and #GIE, %0"

instruction to _get_interrupt_state() and change _set_interrupt_state(x) to

"nop { bis.w %0, SR { nop"

or just be honest about what _set_interrupt_state() does, and add "cc" to the clobber list.

  • Hi Thilo,

    Thank you for your request.

    This issue has been discussed in some previous threads:

    https://e2e.ti.com/support/microcontrollers/msp430/f/166/t/50102?Intrinsic-functions

    https://e2e.ti.com/support/microcontrollers/msp430/f/166/t/137607?precise-behavior-of-disable-interrupts-set-interrupt-state-intrinsics

    You are correct that the naming might be confusing, but the threads above discuss the validity of the function.

    I'm tending to agree with Jens-Michael comment saying that most flags are irrelevant when using C. Modifying OSCOFF or LPM bits, is not very common in a critical routine (or at least, I'm not aware of previous customer issues), and it can also be handled manually.

    Furthermore, please note that this is the expected behavior from the function. The CCS compiler guide has the following information:

    Modifying it would disrupt existing implementations.I've seen cases where this intrinsic is used to get/manipulate other SR bits.

    But feel free to modify it or adapt it to your needs if you have cases where the existing intrinsic would cause problems.

    Regards,

    Luis R

  • In that case, __get_interrupt_state() is redundant with __get_SR_register().

    Furthermore, please note that this is the expected behavior from the function. The CCS compiler guide has the following information

    It is nice that the CCS compiler guide says this. I guess the average programmer will not be aware of side-effects when copy&pasting how-to-do-it™ code with names like these, though. I certainly was not, before I saw the implementation.

    From Jens-Michael's answer:

    And since the content of the status register is unimportant across C instructions, it is safe to do so.

    It is possible that I misunderstoof the meaning of the "cc" clobber flag in gcc extended ASM. But my understanding is that "cc" would not be required if this were true. Even if the optimiser does not optimise condition flags across C instructions currently on MSP430 for -O3, what guarantee is there that won't change in the future?

    I consider these intrinsics broken and will implement them myself.

  • MSP430-GCC has no real concept of a condition code register, "cc", like other GCC targets might have. The only time the GCC optimizer considers the condition code register is when it splits a 32-bit add into two 16-bit adds, since the carry from the first add must be preserved, GCC ensures that the carry register will not be clobbered by instructions which might get placed in between the two adds (conditional branch instructions).

    I think it is very unlikely (but I cannot assert it is impossible without further investigation), that GCC would split a 32-bit add so that first part of the add is in the critical section, and the second part is out of the critical section, or vice-versa.

    If you want to clobber the register which tracks the carry status, clobber register number 2, or you might be able to use the name "CARRY".

    Regards,

  • I think it is very unlikely (but I cannot assert it is impossible without further investigation), that GCC would split a 32-bit add so that first part of the add is in the critical section, and the second part is out of the critical section, or vice-versa.

    I realize what we're talking about here is pretty obscure. But it's the obscure stuff that produces those bugs most difficult to hunt down, so I'd like to get this right.

    If you want to clobber the register which tracks the carry status, clobber register number 2, or you might be able to use the name "CARRY".

    It doesn't accept "CARRY", but "r2".

    BTW, my proposal for _set_interrupt_state() in the opening post is broken, too. It can only re-enable an interrupt (on-off-on), but not re-disable it (off-on-off). Interestingly, one cannot enable interrupts by XORing GIE into the status register either. I guess it's because XOR updates SR for status bits, so I'm down to branch instructions for my _set_interrupt_state() implementation.

  • Thilo Schulz said:
    BTW, my proposal for _set_interrupt_state() in the opening post is broken, too. It can only re-enable an interrupt (on-off-on), but not re-disable it (off-on-off). Interestingly, one cannot enable interrupts by XORing GIE into the status register either. I guess it's because XOR updates SR for status bits, so I'm down to branch instructions for my _set_interrupt_state() implementation

    The section on the status register in the reference manuals for MSP430 devices says that bit manipulations of the SR should be done only with MOV, BIS and BIC instructions (e.g. section 4.3.3 of slau367)

  • Indeed. Thank you for going to the trouble to find this for me. I've read this before, alas the entirety of the documentation is not present in my head all of the time. I have implemented my own solution now, so I'm happy.

**Attention** This is a public forum