Skip to content

[BUG] Atomic_*s are not atomic! #936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sebunger opened this issue Dec 27, 2023 · 2 comments
Closed

[BUG] Atomic_*s are not atomic! #936

sebunger opened this issue Dec 27, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@sebunger
Copy link

In atomic.h, ATOMIC_ENTER_CRITICAL and ATOMIC_EXIT_CRITICAL are defined differently depending on whether portSET_INTERRUPT_MASK_FROM_ISR is defined! However, in FreeRTOS.h this macro is stubbed out if it is not already defined by the port.

This means that on all ports that do not, in fact, define their own (and functional) portSET_INTERRUPT_MASK_FROM_ISR, the atomics will end up silently not being atomic resulting in who-knows-what sort of bugs.

This is particularly egregious since the template port does not even mention that macro in its template portmacro.h!

There’s a possibly related but not quite identical post that dates back 7 years. So this may have been there a while? Unfortunately I can’t seem to link to the post since I just signed up, but the topic number is 6295.

There's a bit more discussion along with possible avenues for fixes in the forum: https://forums.freertos.org/t/atomic-s-are-not-atomic/18809

@sebunger sebunger added the bug Something isn't working label Dec 27, 2023
@jefftenney
Copy link
Contributor

I did a quick test with MSP430. That port does not define portSET_INTERRUPT_MASK_FROM_ISR(). I added some code to call Atomic_Increment_u32() and then checked the compiled code (asm file produced by compiler). The compiled code does not mask interrupts and does not provide atomic behavior. So this is a good catch.

From the forum post it seems adding a new symbol portHAS_NESTED_INTERRUPTS would allow a simple fix to atomic.h.

@chinglee-iot
Copy link
Member

@sebunger
Thank you for reporting this issue. This issue is addressed in PR #940.
Feel free to reopen this issue for further problem about this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants