kilograham
commented
4 months ago 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.
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_interruptsis :__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
counterwould 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);