Replaced the stale system stack switch pseudo-code in the ARMv7-A ports with comments that describe what the code actually does - #735
Merged
Conversation
…ts with comments that describe what the code actually does
The context save, vectored context save and system return routines in the
ARMv7-A ports carried pseudo-code comments claiming that they saved the
thread stack pointer and then switched to _tx_thread_system_stack_ptr.
Neither of those things happens, and none of these ports references that
variable outside an unused IMPORT in their example builds.
On ARMv7-A each processor mode has its own banked stack pointer. The IRQ
handler branches to _tx_thread_context_save while still in IRQ mode, so the
core's banked IRQ stack already serves as the system stack, and the thread
stack pointer is stored in the control block by _tx_thread_context_restore,
and only when the interrupt results in preemption. The scheduler runs on the
banked SVC mode stack that the startup code sets up. There is nothing for a
software stack switch to do.
The comments were therefore misleading rather than merely redundant, and had
led at least one user to try to restore the code they described. They are now
replaced by a description of the actual mechanism.
The AArch64 SMP ports keep their comments unchanged, because ARMv8-A does not
bank a stack pointer per processor mode and those ports do reload
_tx_thread_system_stack_ptr[core] explicitly.
This is a comment-only change. Every changed line is a comment, and all
twenty-five GNU variants still assemble cleanly for their target core.
The fourteen files under ports/cortex_a{5,7,8,9,12,15,17} were regenerated
from ports_arch/ARMv7-A/threadx/common/src/tx_thread_system_return.S with
ports_arch/ARMv7-A/update.sh. The ARMv7-A SMP ports have no generator, so
those files were edited directly.
Fixes eclipse-threadx#734
Assisted-by: Copilot (Opus 5) <noreply@github.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The context save, vectored context save and system return routines in the ARMv7-A ports carry pseudo-code comments claiming that they save the thread stack pointer and then switch to
_tx_thread_system_stack_ptr:Neither of those things happens. None of these ports references
_tx_thread_system_stack_ptranywhere outside an unusedIMPORT/.globalin their example builds. The comments are leftovers from an older generic port template.They are misleading rather than merely redundant: in #240 a user set out to add the "missing" stack switch code that these comments describe. This PR replaces them with a description of the mechanism the ports actually use.
Why there is no software stack switch on ARMv7-A
On ARMv7-A every processor mode has its own banked stack pointer, so the switch is done by the hardware.
IRQ_Handlerbranches straight to_tx_thread_context_savewhile still in IRQ mode (ports_smp/cortex_a7_smp/gnu/example_build/startup.S:170), so on entryspis already this core's banked IRQ stack, carved out of_stack_init_irqbystartup.S:284-289. That stack is the system stack._tx_thread_context_restore, on the__tx_thread_preempt_restorepath only (tx_thread_context_restore.S:197-233), and therefore only when the interrupt results in preemption.startup.S:298-301sets up.The AArch64 SMP ports are deliberately left alone. ARMv8-A does not bank a stack pointer per processor mode, so those ports do keep
_tx_thread_system_stack_ptr[core]as a real array and reload it explicitly; there the comments are accurate.Scope
38 files, all comment-only.
ports_arch/ARMv7-A/threadx/common/src/tx_thread_system_return.Sports/cortex_a{5,7,8,9,12,15,17}/{ac6,gnu}/src/tx_thread_system_return.Sports_arch/ARMv7-A/update.sh --port-sets txports_module/cortex_a7/{gnu,iar}/module_manager/src/tx_thread_system_return.*ports_smp/cortex_a{5,7,9}_smp/{ac5,gnu}andports_smp/cortex_r8_smp/ac5, three routines eachports_arch/ARMv7-A/threadx_smptreeUntouched, because their comments match their code: every ARMv8-A, ARC, MIPS, RISC-V, Xtensa, ARM9 and ARM11 port, and
ports/cortex_a{5,7,8,9}/{ac5,ghs,iar}.Validation
arm-none-eabi-gccfor their target core. Theac5andiarfiles have no toolchain available here and were reviewed by diff.scripts/check_ports.shpasses, including the "generated ports are reproducible fromports_arch" check, which confirms the 14 regenerated files match the template exactly.Known leftover, deliberately out of scope
Seven ARMv7-A SMP example builds still declare
_tx_thread_system_stack_ptrwith an unusedIMPORTor.global. The symbol is genuinely defined incommon_smp/src/tx_thread_initialize.c:43, so these link fine; removing them would mean touching example build files for no benefit.Fixes #734
Follows up on #240