Fixed race condition and message loss in Cortex-M GNU ports - #523
Conversation
…readx#516) - Added compiler memory barriers to BASEPRI management functions in tx_port.h. - Added architectural barriers (DSB/ISB) to scheduler return paths in tx_port.h and tx_thread_system_return.S to prevent fall-through before context switch. - These changes address spurious thread resumption and lost messages, especially when TX_NOT_INTERRUPTABLE is enabled. Assisted-by: Gemini (Gemini 2.0 Flash)
…readx#516) - Added compiler memory barriers to BASEPRI management functions in tx_port.h. - Added architectural barriers (DSB/ISB) to scheduler return paths in tx_port.h and tx_thread_system_return.S to prevent fall-through before context switch. - These changes address spurious thread resumption and lost messages, especially when TX_NOT_INTERRUPTABLE is enabled. Assisted-by: Gemini (Gemini 2.0 Flash)
…readx#516) - Added compiler memory barriers to BASEPRI management functions in tx_port.h. - Added architectural barriers (DSB/ISB) to scheduler return paths in tx_port.h and tx_thread_system_return.s to prevent fall-through before context switch. - These changes address spurious thread resumption and lost messages, especially when TX_NOT_INTERRUPTABLE is enabled. Assisted-by: Gemini (Gemini 2.0 Flash)
…clipse-threadx#516) - Added compiler memory barriers to BASEPRI management functions in tx_port.h (M7). - Added architectural barriers (DSB/ISB) to scheduler return paths in tx_port.h and tx_thread_system_return.S (M7, M23) to prevent fall-through before context switch. - These changes address spurious thread resumption and lost messages, especially when TX_NOT_INTERRUPTABLE is enabled. Assisted-by: Gemini (Gemini 2.0 Flash)
…ts (eclipse-threadx#516) - Added compiler memory barriers to BASEPRI management functions in tx_port.h (M3, M4, M7, M55, M85). - Added architectural barriers (DSB/ISB) to scheduler return paths in tx_port.h and tx_thread_system_return.s (all architectures) to prevent fall-through before context switch. - Added Gemini attribution and updated headers to follow project mandates. - These changes address spurious thread resumption and lost messages, especially when TX_NOT_INTERRUPTABLE is enabled. Assisted-by: Gemini (Gemini 2.0 Flash)
| #else | ||
| __enable_interrupts(); | ||
| #endif | ||
| __restore_interrupt(interrupt_save); |
There was a problem hiding this comment.
I would advise adding ISB immediately after restoring interrupts, so in the line following
| __restore_interrupt(interrupt_save); | |
| __asm__ volatile ("isb 0xF " : : : "memory"); |
as per Arm DAI 0321A PDF
“If it is necessary to ensure a pended interrupt is recognized before subsequent operations, the ISB instruction should be used after CPSIE I.”
(section 4.7, p.28; lines 1077-1080)
__enable_irq(); // CPSIE I : Enable interrupt __ISB(); // Allow pended interrupts to be recognized
(p.28; lines 1083-1087)
“you might want to insert an ISB instruction if the priority level change can result in the interrupt being accepted, and you want this interrupt to be executed immediately.”
(p.34; lines 1321-1324)
| MSR BASEPRI, r1 // Restore original interrupt posture | ||
| #else | ||
| MRS r1, PRIMASK // Thread context returning, pickup PRIMASK | ||
| CPSIE i // Enable interrupts | ||
| MSR PRIMASK, r1 // Restore original interrupt posture | ||
| #endif |
There was a problem hiding this comment.
Same thing here
| MSR BASEPRI, r1 // Restore original interrupt posture | |
| ISB #0xF // Flush pipeline | |
| #else | |
| MRS r1, PRIMASK // Thread context returning, pickup PRIMASK | |
| CPSIE i // Enable interrupts | |
| MSR PRIMASK, r1 // Restore original interrupt posture | |
| ISB #0xF // Flush pipeline | |
| #endif |
Pulling in same ARM citations:
as per Arm DAI 0321A PDF
“If it is necessary to ensure a pended interrupt is recognized before subsequent operations, the ISB instruction should be used after CPSIE I.”
(section 4.7, p.28; lines 1077-1080)
__enable_irq(); // CPSIE I : Enable interrupt __ISB(); // Allow pended interrupts to be recognized
(p.28; lines 1083-1087)
“you might want to insert an ISB instruction if the priority level change can result in the interrupt being accepted, and you want this interrupt to be executed immediately.”
(p.34; lines 1321-1324)
…clipse-threadx#516) - Added ISB (Instruction Synchronization Barrier) to _tx_thread_system_return_inline in tx_port.h for all Cortex-M GNU ports. - Added ISB instruction to _tx_thread_system_return in tx_thread_system_return.S for all Cortex-M GNU ports. - These changes ensure that pending interrupts (specifically PendSV) are recognised before subsequent instructions are executed, following Kairalite's feedback and ARM architectural guidelines. Assisted-by: Gemini (Gemini 2.0 Flash)
|
Thank you for the feedback, @Kairalite. I updated the PR accordingly. Do you feel other changes are needed before I merge? |
|
@fdesbiens Thank you! One more thing I want to bring to your attention: This might be out of scope, but another section Codex flagged when I was using it to help me pin down this bug was in tx_queue_cleanup.c Specifically in the non_interruptable path, it checks if the cleanup is still required and that the thread objects are still valid. This may be noise, as I am not familiar with the rest of the repo, but wanted to flag it for you guys just in case. I've attached the diff below |
In TX_NOT_INTERRUPTABLE mode the caller keeps interrupts disabled across the entire cleanup call, so the race window that makes the guards necessary in the interruptable path cannot occur. Add a comment explaining this, and noting that all paths that resume a suspended thread clear tx_thread_suspend_cleanup before calling _tx_thread_system_ni_resume, making double-cleanup impossible. This prevents future false-positive suggestions (e.g. from AI tools) to add redundant checks to the NI path. Relates to: eclipse-threadx#516 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@Kairalite After careful analysis, I won't be applying the change from the diff file you shared. The The Additionally:
So the preconditions the checks guard against simply cannot arise in a correct NI execution. A small comment has been added to document the above. |
|
Understood, thank you for taking a look at this! I have no further comments, and the fixes work on my end. Thank you again! |
The Cortex-M ports under ports/ are generated. scripts/copy_armv7_m.sh copies one tx_port.h and the per tool sources to fifteen M3, M4 and M7 targets, and scripts/copy_armv8_m.sh does the same for nine M33, M55 and M85 targets. The ports_arch_check workflow runs both scripts and fails if the tree is not reproducible, so those copies are meant never to be edited directly. They were. Every Cortex-M fix since eclipse-threadx#523 was applied to the generated copies and not to the source, so the source fell behind and the check went red: running the three scripts on dev changes 35 files. The check triggers only on pull requests targeting master, which is why nothing caught it while the fixes were merged into dev. Left alone, the next run of these scripts would have reverted three separate pieces of work: the memory barriers and clobbers from eclipse-threadx#523, the correction of the IAR assembly header to use the assembler's own comment syntax, and the move of tx_initialize_low_level.S into example_build for the M33, M55 and M85 GNU ports from eclipse-threadx#514. Bring the sources up to what the ports carry today, and regenerate. Two behavioural changes come with that, both deliberate. The barriers from eclipse-threadx#523 reach the ac5 and keil variants of M3, M4 and M7, which were outside the scope of that fix and never received it. The barrier that follows restoring the interrupt posture, which eclipse-threadx#523 gave only to the GNU ports because GNU was the only toolchain that could be tested, now applies to every tool; the identical asm statement already shipped in the AC6 and IAR ports, so this adds a pipeline flush rather than any new compiler exposure. Regenerating also drops a stray #endif at the end of the Cortex-M85 IAR tx_port.h, added by eclipse-threadx#523, which left that header with one more #endif than #if and unable to compile. Every other ARMv8-M port was balanced. Verified that the scripts are idempotent afterwards, that ports_arch_check would pass, that no port loses a barrier or a clobber, that every regenerated header is preprocessor balanced, and that every Cortex-M port covered by the two scripts now carries the entry barrier. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
The Cortex-M ports under ports/ are generated. scripts/copy_armv7_m.sh copies one tx_port.h and the per tool sources to fifteen M3, M4 and M7 targets, and scripts/copy_armv8_m.sh does the same for nine M33, M55 and M85 targets. The ports_arch_check workflow runs both scripts and fails if the tree is not reproducible, so those copies are meant never to be edited directly. They were. Every Cortex-M fix since #523 was applied to the generated copies and not to the source, so the source fell behind and the check went red: running the three scripts on dev changes 35 files. The check triggers only on pull requests targeting master, which is why nothing caught it while the fixes were merged into dev. Left alone, the next run of these scripts would have reverted three separate pieces of work: the memory barriers and clobbers from #523, the correction of the IAR assembly header to use the assembler's own comment syntax, and the move of tx_initialize_low_level.S into example_build for the M33, M55 and M85 GNU ports from #514. Bring the sources up to what the ports carry today, and regenerate. Two behavioural changes come with that, both deliberate. The barriers from #523 reach the ac5 and keil variants of M3, M4 and M7, which were outside the scope of that fix and never received it. The barrier that follows restoring the interrupt posture, which #523 gave only to the GNU ports because GNU was the only toolchain that could be tested, now applies to every tool; the identical asm statement already shipped in the AC6 and IAR ports, so this adds a pipeline flush rather than any new compiler exposure. Regenerating also drops a stray #endif at the end of the Cortex-M85 IAR tx_port.h, added by #523, which left that header with one more #endif than #if and unable to compile. Every other ARMv8-M port was balanced. Verified that the scripts are idempotent afterwards, that ports_arch_check would pass, that no port loses a barrier or a clobber, that every regenerated header is preprocessor balanced, and that every Cortex-M port covered by the two scripts now carries the entry barrier. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
… toolchains (#595) Two unrelated Cortex-M0 gaps, both left over from earlier work. The memory barriers from #523 reached the Cortex-M0 gnu port but not its ac6 or iar siblings, in either the inline system return in tx_port.h or the assembly routine. Both take the same GNU or IAR code path, and iar already carried the entry barrier, so the missing pieces were the entry pair for ac6 and the barrier after restoring the interrupt posture for both. All three tools now match. The Cortex-M0 example could not link with any toolchain. cortexm0_crt0.S references 23 linker script symbols and the script defined only 12 of them, so __text_start__, __text_end__, __text_load_start__, the rodata and fast section symbols, and the ctors and dtors load addresses were all unresolved. The Cortex-M4 script defines all 23, including a .fast section with no content whose symbols exist so that the startup copy is a no-op, and its comment says as much. The Cortex-M0 script is brought to that same shape. That left the example failing under LLVM only, on instructions that ARMv6-M can encode just one way. The file declared .code 16 but no syntax mode, so GNU as used the legacy divided syntax in which a plain add or sub sets the flags implicitly, while LLVM implements unified syntax only and rejected the non-flag-setting spelling. Declaring .syntax unified and writing movs, adds and subs makes both assemblers agree, and the encodings GNU produces are byte identical before and after, verified by disassembling both objects. The Cortex-M0 example now links with GNU at 22,520 bytes of text and with Arm Toolchain for Embedded at 22,866, so it comes off the list of examples not expected to link and scripts/check_clang.sh now links eight of eight. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
…lipse-threadx#590) The Cortex-M ports under ports/ are generated. scripts/copy_armv7_m.sh copies one tx_port.h and the per tool sources to fifteen M3, M4 and M7 targets, and scripts/copy_armv8_m.sh does the same for nine M33, M55 and M85 targets. The ports_arch_check workflow runs both scripts and fails if the tree is not reproducible, so those copies are meant never to be edited directly. They were. Every Cortex-M fix since eclipse-threadx#523 was applied to the generated copies and not to the source, so the source fell behind and the check went red: running the three scripts on dev changes 35 files. The check triggers only on pull requests targeting master, which is why nothing caught it while the fixes were merged into dev. Left alone, the next run of these scripts would have reverted three separate pieces of work: the memory barriers and clobbers from eclipse-threadx#523, the correction of the IAR assembly header to use the assembler's own comment syntax, and the move of tx_initialize_low_level.S into example_build for the M33, M55 and M85 GNU ports from eclipse-threadx#514. Bring the sources up to what the ports carry today, and regenerate. Two behavioural changes come with that, both deliberate. The barriers from eclipse-threadx#523 reach the ac5 and keil variants of M3, M4 and M7, which were outside the scope of that fix and never received it. The barrier that follows restoring the interrupt posture, which eclipse-threadx#523 gave only to the GNU ports because GNU was the only toolchain that could be tested, now applies to every tool; the identical asm statement already shipped in the AC6 and IAR ports, so this adds a pipeline flush rather than any new compiler exposure. Regenerating also drops a stray #endif at the end of the Cortex-M85 IAR tx_port.h, added by eclipse-threadx#523, which left that header with one more #endif than #if and unable to compile. Every other ARMv8-M port was balanced. Verified that the scripts are idempotent afterwards, that ports_arch_check would pass, that no port loses a barrier or a clobber, that every regenerated header is preprocessor balanced, and that every Cortex-M port covered by the two scripts now carries the entry barrier. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
… toolchains (eclipse-threadx#595) Two unrelated Cortex-M0 gaps, both left over from earlier work. The memory barriers from eclipse-threadx#523 reached the Cortex-M0 gnu port but not its ac6 or iar siblings, in either the inline system return in tx_port.h or the assembly routine. Both take the same GNU or IAR code path, and iar already carried the entry barrier, so the missing pieces were the entry pair for ac6 and the barrier after restoring the interrupt posture for both. All three tools now match. The Cortex-M0 example could not link with any toolchain. cortexm0_crt0.S references 23 linker script symbols and the script defined only 12 of them, so __text_start__, __text_end__, __text_load_start__, the rodata and fast section symbols, and the ctors and dtors load addresses were all unresolved. The Cortex-M4 script defines all 23, including a .fast section with no content whose symbols exist so that the startup copy is a no-op, and its comment says as much. The Cortex-M0 script is brought to that same shape. That left the example failing under LLVM only, on instructions that ARMv6-M can encode just one way. The file declared .code 16 but no syntax mode, so GNU as used the legacy divided syntax in which a plain add or sub sets the flags implicitly, while LLVM implements unified syntax only and rejected the non-flag-setting spelling. Declaring .syntax unified and writing movs, adds and subs makes both assemblers agree, and the encodings GNU produces are byte identical before and after, verified by disassembling both objects. The Cortex-M0 example now links with GNU at 22,520 bytes of text and with Arm Toolchain for Embedded at 22,866, so it comes off the list of examples not expected to link and scripts/check_clang.sh now links eight of eight. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
Aims to solve the problems raised in issue #516.