Repository navigation
fix(thread): complete context-switch clobbers and reserve Win64 shadow space - #1720
Merged
Merged
Conversation
…skip red zone switch_context() / switch_context_defer() hide a function call inside an asm statement, across which other photon threads run and may change any register or memory. The clobber lists only named GPRs (plus xmm6-15 on Win64), so the compiler was free to keep values in vector registers, in memory it had cached, or in the red zone across a switch. This is what makes GCC 10/11/12 CI jobs crash (SEGFAULT in test-thread, test-socket, test-pool, test-executor-std, test-exportfs) on part of the nodes: with -march=native on a CPU model unknown to these versions (most likely Emerald Rapids, 0xCF), the driver falls back to -march=cooperlake -mtune=generic, under which GCC keeps the thread pointer behind `return rq.current->error_number` in %xmm0 across the switch in thread_yield_to(). - x86_64 / aarch64: declare all vector registers (and AVX-512 opmask / SVE predicate registers when enabled) as clobbered, plus "memory"; plus "cc" on aarch64. - x86_64 SysV / Apple arm64: skip the 128-byte red zone before the call, as GCC's manual describes for asm containing calls or pushes. - x86_64: call the defer function with %rsp aligned to 16 bytes instead of jumping into it on whatever the switch-point %rsp happens to be. - Win64: reserve the 32-byte shadow space for the defer function and the thread entry; the latter otherwise overlaps the head of `struct thread`. - tools/photongdb.py: account for the skipped red zone when recovering %rsp of a switched-out thread.
The asm statement is not treated as a call, so the ABI's caller-saved registers are not implied clobbered; nor does the C++ call of the inline switch_context() help, as it is inlined, or subject to -fipa-ra.
Restore the original saved-stack layout and photongdb offset. Keep vector/memory/cc clobbers, defer alignment and Win64 shadow space; no current Photon red-zone failure has been demonstrated.
lihuiba
reviewed
Oct 30, 2026
| "r15", VECTOR_CLOBBERS, "memory"); | ||
| } | ||
| #else // _WIN64 | ||
| // The Win64 thread-entry stub must reserve 32 bytes of shadow space |
Collaborator
There was a problem hiding this comment.
move to _photon_thread_stub and _photon_thread_die
Collaborator
Author
There was a problem hiding this comment.
Done in 9a3e754: the comment now sits right above DEF_ASM_FUNC(_photon_thread_stub), next to the sub $32, %rsp and the calls to the thread entry and _photon_thread_die it describes.
lihuiba
approved these changes
Nov 2, 2026
Coldwings
added a commit
that referenced
this pull request
Nov 4, 2026
…rs and reserve Win64 shadow space (#1720) (#1725) * fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) * fix(thread): complete clobbers of context switch, align defer stack, skip red zone switch_context() / switch_context_defer() hide a function call inside an asm statement, across which other photon threads run and may change any register or memory. The clobber lists only named GPRs (plus xmm6-15 on Win64), so the compiler was free to keep values in vector registers, in memory it had cached, or in the red zone across a switch. This is what makes GCC 10/11/12 CI jobs crash (SEGFAULT in test-thread, test-socket, test-pool, test-executor-std, test-exportfs) on part of the nodes: with -march=native on a CPU model unknown to these versions (most likely Emerald Rapids, 0xCF), the driver falls back to -march=cooperlake -mtune=generic, under which GCC keeps the thread pointer behind `return rq.current->error_number` in %xmm0 across the switch in thread_yield_to(). - x86_64 / aarch64: declare all vector registers (and AVX-512 opmask / SVE predicate registers when enabled) as clobbered, plus "memory"; plus "cc" on aarch64. - x86_64 SysV / Apple arm64: skip the 128-byte red zone before the call, as GCC's manual describes for asm containing calls or pushes. - x86_64: call the defer function with %rsp aligned to 16 bytes instead of jumping into it on whatever the switch-point %rsp happens to be. - Win64: reserve the 32-byte shadow space for the defer function and the thread entry; the latter otherwise overlaps the head of `struct thread`. - tools/photongdb.py: account for the skipped red zone when recovering %rsp of a switched-out thread. * fix(thread): clarify why volatile registers must be listed as clobbers The asm statement is not treated as a call, so the ABI's caller-saved registers are not implied clobbered; nor does the C++ call of the inline switch_context() help, as it is inlined, or subject to -fipa-ra. * fix(thread): remove speculative red-zone padding Restore the original saved-stack layout and photongdb offset. Keep vector/memory/cc clobbers, defer alignment and Win64 shadow space; no current Photon red-zone failure has been demonstrated. * fix(thread): preserve original defer jump trampolines * fix(thread): move Win64 shadow-space comment to _photon_thread_stub * fix(thread): resolve Win64 thread stub conflict for release/0.9 Take the 32-byte shadow-space reservation from #1720 and drop the Win64 call to _asan_start, as main did in #937. ASan builds are Linux-only (PHOTON_BUILD_WITH_ASAN fails the configure elsewhere), so on Win64 _asan_start is always an empty function. --------- Co-authored-by: Coldwings <coldwings@me.com>
lihuiba
pushed a commit
that referenced
this pull request
Nov 4, 2026
…rs and reserve Win64 shadow space (#1720) (#1725) (#1726) * fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) * fix(thread): complete clobbers of context switch, align defer stack, skip red zone switch_context() / switch_context_defer() hide a function call inside an asm statement, across which other photon threads run and may change any register or memory. The clobber lists only named GPRs (plus xmm6-15 on Win64), so the compiler was free to keep values in vector registers, in memory it had cached, or in the red zone across a switch. This is what makes GCC 10/11/12 CI jobs crash (SEGFAULT in test-thread, test-socket, test-pool, test-executor-std, test-exportfs) on part of the nodes: with -march=native on a CPU model unknown to these versions (most likely Emerald Rapids, 0xCF), the driver falls back to -march=cooperlake -mtune=generic, under which GCC keeps the thread pointer behind `return rq.current->error_number` in %xmm0 across the switch in thread_yield_to(). - x86_64 / aarch64: declare all vector registers (and AVX-512 opmask / SVE predicate registers when enabled) as clobbered, plus "memory"; plus "cc" on aarch64. - x86_64 SysV / Apple arm64: skip the 128-byte red zone before the call, as GCC's manual describes for asm containing calls or pushes. - x86_64: call the defer function with %rsp aligned to 16 bytes instead of jumping into it on whatever the switch-point %rsp happens to be. - Win64: reserve the 32-byte shadow space for the defer function and the thread entry; the latter otherwise overlaps the head of `struct thread`. - tools/photongdb.py: account for the skipped red zone when recovering %rsp of a switched-out thread. * fix(thread): clarify why volatile registers must be listed as clobbers The asm statement is not treated as a call, so the ABI's caller-saved registers are not implied clobbered; nor does the C++ call of the inline switch_context() help, as it is inlined, or subject to -fipa-ra. * fix(thread): remove speculative red-zone padding Restore the original saved-stack layout and photongdb offset. Keep vector/memory/cc clobbers, defer alignment and Win64 shadow space; no current Photon red-zone failure has been demonstrated. * fix(thread): preserve original defer jump trampolines * fix(thread): move Win64 shadow-space comment to _photon_thread_stub * fix(thread): resolve Win64 thread stub conflict for release/0.9 Take the 32-byte shadow-space reservation from #1720 and drop the Win64 call to _asan_start, as main did in #937. ASan builds are Linux-only (PHOTON_BUILD_WITH_ASAN fails the configure elsewhere), so on Win64 _asan_start is always an empty function. --------- Co-authored-by: Coldwings <coldwings@me.com>
lihuiba
added a commit
that referenced
this pull request
Nov 4, 2026
Take the 32-byte shadow-space reservation from #1720. release/0.7 never had the Win64 `call _asan_start` that the upstream change replaced, so there is nothing to drop here.
lihuiba
added a commit
that referenced
this pull request
Nov 4, 2026
…ers and reserve Win64 shadow space (#1720) (#1725) (#1726) (#1727) * [Backport][main to 0.9] | fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) (#1725) (#1726) * fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) * fix(thread): complete clobbers of context switch, align defer stack, skip red zone switch_context() / switch_context_defer() hide a function call inside an asm statement, across which other photon threads run and may change any register or memory. The clobber lists only named GPRs (plus xmm6-15 on Win64), so the compiler was free to keep values in vector registers, in memory it had cached, or in the red zone across a switch. This is what makes GCC 10/11/12 CI jobs crash (SEGFAULT in test-thread, test-socket, test-pool, test-executor-std, test-exportfs) on part of the nodes: with -march=native on a CPU model unknown to these versions (most likely Emerald Rapids, 0xCF), the driver falls back to -march=cooperlake -mtune=generic, under which GCC keeps the thread pointer behind `return rq.current->error_number` in %xmm0 across the switch in thread_yield_to(). - x86_64 / aarch64: declare all vector registers (and AVX-512 opmask / SVE predicate registers when enabled) as clobbered, plus "memory"; plus "cc" on aarch64. - x86_64 SysV / Apple arm64: skip the 128-byte red zone before the call, as GCC's manual describes for asm containing calls or pushes. - x86_64: call the defer function with %rsp aligned to 16 bytes instead of jumping into it on whatever the switch-point %rsp happens to be. - Win64: reserve the 32-byte shadow space for the defer function and the thread entry; the latter otherwise overlaps the head of `struct thread`. - tools/photongdb.py: account for the skipped red zone when recovering %rsp of a switched-out thread. * fix(thread): clarify why volatile registers must be listed as clobbers The asm statement is not treated as a call, so the ABI's caller-saved registers are not implied clobbered; nor does the C++ call of the inline switch_context() help, as it is inlined, or subject to -fipa-ra. * fix(thread): remove speculative red-zone padding Restore the original saved-stack layout and photongdb offset. Keep vector/memory/cc clobbers, defer alignment and Win64 shadow space; no current Photon red-zone failure has been demonstrated. * fix(thread): preserve original defer jump trampolines * fix(thread): move Win64 shadow-space comment to _photon_thread_stub * fix(thread): resolve Win64 thread stub conflict for release/0.9 Take the 32-byte shadow-space reservation from #1720 and drop the Win64 call to _asan_start, as main did in #937. ASan builds are Linux-only (PHOTON_BUILD_WITH_ASAN fails the configure elsewhere), so on Win64 _asan_start is always an empty function. --------- Co-authored-by: Coldwings <coldwings@me.com> * fix(thread): resolve the Win64 thread stub conflict for release/0.7 Take the 32-byte shadow-space reservation from #1720. release/0.7 never had the Win64 `call _asan_start` that the upstream change replaced, so there is nothing to drop here. --------- Co-authored-by: Coldwings <coldwings@me.com> Co-authored-by: lihuiba <huiba.lhb@alibaba-inc.com>
lihuiba
added a commit
that referenced
this pull request
Nov 4, 2026
…ers and reserve Win64 shadow space (#1720) (#1725) (#1726) (#1727) (#1728) * [Backport][main to 0.9] | fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) (#1725) (#1726) * fix(thread): complete context-switch clobbers and reserve Win64 shadow space (#1720) * fix(thread): complete clobbers of context switch, align defer stack, skip red zone switch_context() / switch_context_defer() hide a function call inside an asm statement, across which other photon threads run and may change any register or memory. The clobber lists only named GPRs (plus xmm6-15 on Win64), so the compiler was free to keep values in vector registers, in memory it had cached, or in the red zone across a switch. This is what makes GCC 10/11/12 CI jobs crash (SEGFAULT in test-thread, test-socket, test-pool, test-executor-std, test-exportfs) on part of the nodes: with -march=native on a CPU model unknown to these versions (most likely Emerald Rapids, 0xCF), the driver falls back to -march=cooperlake -mtune=generic, under which GCC keeps the thread pointer behind `return rq.current->error_number` in %xmm0 across the switch in thread_yield_to(). - x86_64 / aarch64: declare all vector registers (and AVX-512 opmask / SVE predicate registers when enabled) as clobbered, plus "memory"; plus "cc" on aarch64. - x86_64 SysV / Apple arm64: skip the 128-byte red zone before the call, as GCC's manual describes for asm containing calls or pushes. - x86_64: call the defer function with %rsp aligned to 16 bytes instead of jumping into it on whatever the switch-point %rsp happens to be. - Win64: reserve the 32-byte shadow space for the defer function and the thread entry; the latter otherwise overlaps the head of `struct thread`. - tools/photongdb.py: account for the skipped red zone when recovering %rsp of a switched-out thread. * fix(thread): clarify why volatile registers must be listed as clobbers The asm statement is not treated as a call, so the ABI's caller-saved registers are not implied clobbered; nor does the C++ call of the inline switch_context() help, as it is inlined, or subject to -fipa-ra. * fix(thread): remove speculative red-zone padding Restore the original saved-stack layout and photongdb offset. Keep vector/memory/cc clobbers, defer alignment and Win64 shadow space; no current Photon red-zone failure has been demonstrated. * fix(thread): preserve original defer jump trampolines * fix(thread): move Win64 shadow-space comment to _photon_thread_stub * fix(thread): resolve Win64 thread stub conflict for release/0.9 Take the 32-byte shadow-space reservation from #1720 and drop the Win64 call to _asan_start, as main did in #937. ASan builds are Linux-only (PHOTON_BUILD_WITH_ASAN fails the configure elsewhere), so on Win64 _asan_start is always an empty function. --------- * fix(thread): resolve the Win64 thread stub conflict for release/0.7 Take the 32-byte shadow-space reservation from #1720. release/0.7 never had the Win64 `call _asan_start` that the upstream change replaced, so there is nothing to drop here. --------- Co-authored-by: Coldwings <coldwings@me.com> Co-authored-by: lihuiba <huiba.lhb@alibaba-inc.com>
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.
Summary
switch_context()/switch_context_defer()inthread/thread.cppput acallto the switch routine inside an inlineasmstatement. Other photon threads run before that call "returns", and they can change any register and any memory. The compiler cannot see this, so it relies on the clobber list being complete — and the list is not. It names only general-purpose registers (plusxmm6–xmm15on Win64). As a result, the compiler may legitimately:"memory"clobber).Separately, the Win64
_photon_thread_stubreserves 32 bytes of shadow space for its ordinary calls to the thread entry and_photon_thread_die.Item 1 is the demonstrated cause of the recurring GCC 10/11/12 CI failures (see Evidence). The memory clobber and Win64 thread-entry shadow space are separate corrections.
Why caller-saved registers must be listed
On x86-64 SysV, all
xmm/kregisters are caller-saved (volatile). That does not make them "implicitly clobbered" here:switch_context()does not help either:inlinefunction in the same TU. In the failing build it is inlined intothread_yield_to(), so there is nocall switch_contextboundary there.-fipa-ra(enabled at-O2) lets the caller keep values in volatile registers that the callee does not use. The callee's register usage is derived from these very clobber lists.Conversely, callee-saved vector registers (
xmm6–xmm15on Win64,v8–v15on aarch64) must also be listed, because the switch does not save them the way a real callee would.Problematic code (at 6c529d5)
switch_context/switch_context_deferclobbersxmm*/k*registers, no"memory"_photon_thread_stubxmm6–xmm15;xmm0–xmm5, AVX-512 regs and"memory"missingv0–v31(incl. callee-saved low 64 bits ofv8–v15, which the switch does not save), no"cc", no"memory"References
"memory": "The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters)."from->stack._ptrandto->stack._ptr, and for thedefer_argthatdefer_migrate_current()passes by address from its own stack frame.-fipa-raxmm0–xmm15,xmm16–xmm31andk0–k7are all "temporary registers", i.e. not preserved across calls. The compiler may only keep them live across the asm if they are not clobbered.XMM0–XMM5are volatile,XMM6–XMM15are nonvolatile, and the upper halves ofYMMare volatile.Evidence: GCC 10/11/12 CI crashes
The crashes are recurring, e.g. run 40221375889 / job 121404632180 and its re-run job 121455468864.
Symptom: the same 5 tests SEGFAULT every time —
test-thread,test-socket,test-pool,test-executor-std,test-exportfs. All 5 go throughthread_yield_to().Only one class of runner is affected. The crc64 throughput printed by
test-checksumsplits the runners into two classes. I checked 56 jobs across 5 runs:Why GCC version matters. CI builds with
-march=native(CMAKE_CXX_FLAGS_MINSIZEREL). What-march=nativeexpands to depends on whether that GCC version recognises the CPU model:case 0xcf: /* Emerald Rapids. */.-marchfrom CPU features: AVX512_BF16 without VP2INTERSECT givescooperlake. It then falls back to-mtune=generic. See driver-i386.c @ gcc-11.2.0 and L689.Simulation matches CI 14/14. I compiled
thread.cppon Compiler Explorer for each GCC version × node class, using the expansion above together with the CI flags. I assumed class A = 0xCF (C8i/G8i) and class B = Ice Lake 0x6A (C7/G7/R7). Only gcc-10.3/11.2/12.2 with-march=cooperlake -mtune=genericproduce a value read from%xmm0after the switch. That matches the CI pass/fail matrix exactly. The generated code inthread_yield_to()(gcc-11.2,-O2 -march=cooperlake -mtune=generic), viewable on Compiler Explorer — before / after (source isthread.cppwith its photon headers inlined; search the output forthread_yield_to):After this PR, the same configuration produces:
Caveat: that class A is 0xCF is inferred, not directly confirmed. It is the only model in the pool that GCC ≤ 12 does not recognise and GCC 13 does, so it is the only one that explains "gcc-12 fails, gcc-13 passes". Printing
gcc -march=native -Q --help=target | grep -E 'm(arch|tune)='in CI would confirm it. Either way, the root cause is the incomplete clobber list. The CPU model only decides whether it shows up, so pinning-marchin CI would hide the bug rather than fix it.Changes
VECTOR_CLOBBERSmacro:xmm0–xmm15; when__AVX512F__is defined, alsoxmm16–xmm31andk0–k7. ClobberingxmmNalso coversymmN/zmmN.v0–v31, plusp0–p15when__ARM_FEATURE_SVEis defined.ffris left out because clang rejects it as a clobber name."memory"on all targets and"cc"on aarch64._photon_thread_stubdoessub $32,%rspbefore its ordinary calls to the thread entry and_photon_thread_die; both calls reuse the reserved area.[top-16, top+16)relative to the initial stack top. Withreserved_space == 0,topis exactly the address ofstruct thread, so a callee that homes its arguments (e.g. an-O0build) can overwrite the thread's intrusive list links. The new area is[top-48, top-16).Notes:
vzeroupperafter the switch in some functions. It is a single cheap instruction, and correct in any case, since the resumed code cannot know the upper-half state.mxcsr/ x87 CW. Their handling is unchanged and outside this PR's scope.Verification
thread_yield_to). Additional GCC 10.3 / 11.2 / 12.2 / 13.3 / 14.2 checks with-march=cooperlake -mtune=genericgave the same result.thread.cppcompiled with GCC 14.2 (x86_64-linux-gnu-g++) and Clang 20 (--target=x86_64-linux-gnu) for-march=x86-64,cooperlake -mtune=generic,x86-64-v4andsapphirerapids.test-thread,test-socket,test-executor-std,test-exportfs, andtest-poolpassed in the local RelWithDebInfo build. Clang 20 also compiledthread.cppwith-march=armv8-a+sve.+sveandarmv9-a, passed. The local RelWithDebInfo build succeeded; 66 of 70 selected tests passed.test-throttle,test-iouring, andtest-rpc-messagealso failed on the unmodified base;cache_testlacked/mnt/tmp. This was not a fully passing test suite.clang --target=x86_64-w64-windows-gnuand checked them withobjdump. No runtime test.