Bug in save_and_disable_interrupts() #1644

New issue

Closed

opened 2024-02-20 14:29:25 -06:00 by simonbeaudoin0935 · 3 comments

simonbeaudoin0935 commented

2024-02-20 14:29:25 -06:00

(Migrated from github.com)

The save_and_disable_interrupts function is buggy for two reasons. Either one is not a bug in itself, but combined they produce the following problem.

The first problem :

The function save_and_disable_interrupts is :
__force_inline static uint32_t save_and_disable_interrupts(void) { uint32_t status; pico_default_asm_volatile( "mrs %0, PRIMASK\n" "cpsid i" : "=r" (status) ::); return status; }
The inline assembly statement does not include the : "memory" statement at the end. Without this, the compiler is not prevented from reordering and optimizing stuff.

The second problem is that the function is declared inline. Coupled with problem number one, what it does is that when the body of the function is "copied" in place by the compiler, the compiler can then start optimizing things. Because there is no ~implicit instruction barrier with the "memory", then the compiler can reorder things around.

I came accross a situation there the following code would not work with optimization :

`
uint32_t counter = 0;

...

uint32_t irq_save = save_and_disable_interrupts();
{
counter--;
}
restore_interrupts(irq_save);
`

The problem was that an interrupt modifying counter would pop and execute in the middle of the critical zone above like so :

uint32_t irq_save = save_and_disable_interrupts(); { counter--; <-------------- Interrupt happening in the middle of the read-modify-write } restore_interrupts(irq_save);

The save_and_disable_interrupts function is buggy for two reasons. Either one is not a bug in itself, but combined they produce the following problem. The first problem : The function `save_and_disable_interrupts` is : ` __force_inline static uint32_t save_and_disable_interrupts(void) { uint32_t status; pico_default_asm_volatile( "mrs %0, PRIMASK\n" "cpsid i" : "=r" (status) ::); return status; } ` The inline assembly statement does not include the `: "memory" ` statement at the end. Without this, the compiler is not prevented from reordering and optimizing stuff. The second problem is that the function is declared inline. Coupled with problem number one, what it does is that when the body of the function is "copied" in place by the compiler, the compiler can then start optimizing things. Because there is no ~implicit instruction barrier with the "memory", then the compiler can reorder things around. I came accross a situation there the following code would not work with optimization : ` uint32_t counter = 0; ... uint32_t irq_save = save_and_disable_interrupts(); { counter--; } restore_interrupts(irq_save); ` The problem was that an interrupt modifying `counter` would pop and execute in the middle of the critical zone above like so : ` uint32_t irq_save = save_and_disable_interrupts(); { counter--; <-------------- Interrupt happening in the middle of the read-modify-write } restore_interrupts(irq_save); `

kilograham commented

2024-02-20 17:12:37 -06:00

(Migrated from github.com)

Still, i do tend to agree that this is surprising, but I would want to check if this affects code generation on existing code negatively first.

There is a bit of semantic wiggle room here (or lack of documentation)... in that you could argue that it is the caller's job to prevent compiler re-ordering (is this just an intrinsic?). The synchronization primitives in the SDK already do that, so don't have an issue. Still, i do tend to agree that this is surprising, but I would want to check if this affects code generation on existing code negatively first.

kilograham commented

2024-07-19 14:02:31 -05:00

(Migrated from github.com)