Repository navigation
Conversation
|
I reviewed the caller-stack lifecycle across attribute capture, Linux/Windows thread startup, guest entry trampoline, return/exit/cancellation cleanup, stack-bound tracking, and the new lifecycle/unwind tests. The implementation consistently keeps host startup/TLS on the native thread, switches only for the guest entry, and restores native bounds before teardown; the test matrix described covers the main return, detach/reuse, cleanup and unwind cases. I ran Current main is 262 commits ahead of the merge base and has changed the shared libkernel test CMake file and debt entry; the pthread implementation paths themselves are unchanged. Please refresh those shared files before merge and preserve current registrations/debt. The listed adjacent PRs warrant the stated merge coordination. I found no independent blocker in this pass. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
I reviewed head The live overlap scan identifies comparable shared hunks with #8041 (guard-size queries/attributes), #8477 and #9160 (thread startup), and #9684 (Linux stack placement); its comparisons against #4563 and #7149 are incomplete because those branches conflict with main. #7149 is particularly important to reconcile manually because both change GuestStacks and Windows pthread stack placement. #9573 also touches the pthread attribute/thread files, while #9482 shares the debt entry. The PR currently suggests merging in any order; please identify which behaviors are independent and preserve/retest them together, and add |
65f0cc1 to
b1f5164
Compare
|
Rebased onto Linux on the rebased tree: the full build, the regression (also in 20 repeats), 9 neighbouring tests, ASan and conventions pass. Full suite: 1,826 of 1,848 pass, 19 skip, and the same 3 None of the PRs below needs another one merged first, so
Two of them also meet this PR at run time without a conflict: #9160's new I have read the PR heads but not built the combinations. #7149 with this PR needs a Windows run, mainly to see whether its 256 KiB native stack is enough for caller-stack threads (this PR reserves 1 MiB). |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 3875f8c and FreeBSD 11.4 libthr (thr_create.c create_stack/_pthread_create, thr_attr.c):
- The semantics match libthr. A stack given in the attributes gets guard size 0 and is never freed or reprotected by the library. Both
setstackandsetstackaddrtreat the address as the lowest byte, andsetstackaddrkeeps the attr's current size. The existingEINVALchecks in bothAttr.cppfiles are unchanged and match. - The entry trampoline leaves the guest entry with rsp ≡ 8 mod 16, the same as FreeBSD's
cpu_set_upcall(top&~0xf, minus 8). TheDW_CFA_def_cfa_expressionresolves to the native frame (*(rbp+16)+16), so unwinding back across it reads correctly. - Freeing the stack after
scePthreadJoinis safe. Join returns only afterWaitForSingleObject/pthread_joinon the host thread, so the thread has left the caller stack, as with FreeBSD'sthr_exittid wakeup. - Small question: the non-Windows
StartNativeThreadbranch is also the macOS path, so macOS now runs caller-stack threads too, while the description and the test registration (if(WIN32 OR CMAKE_SYSTEM_NAME STREQUAL "Linux")) cover only Linux and Windows. Could the description mention that macOS gets the same switch untested, or could the test be registered there once macOS links? - I couldn't build or run this here, so I can't confirm the Windows TEB and
__chkstkbehaviour myself. The new debt entry about host code, handlers and probes overrunning below the range reads accurately to me.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
MarcZX10
left a comment
There was a problem hiding this comment.
The caller-stack implementation and its lifetime/unwind coverage were already reviewed in my earlier comments; the author subsequently rebased to b1f516491827f8e53af5f189d064926a9128f6e2 and reports a passing Linux build, repeated focused tests, neighboring tests, ASan and conventions. Those runs are contributor-reported; hosted product build/test jobs remain skipped.
This branch is no longer current with upstream: tools/check_main_sync.py --base upstream/main --head refs/reviews/pr-9938 reports 1 ahead /278 behind, with Thread.cpp, libkernel/CMakeLists.txt, and docs/dev/debt/libkernel.md changed on both sides since the merge base. The merge is clean, but #9960 has changed Thread.cpp since this branch was based. Please refresh against current main, preserve the caller-stack lifecycle alongside that thread-startup work, and rerun the focused Linux/Windows stack and unwind tests plus conventions on the refreshed head.
The existing overlap feedback on #8041, #7149, #4563 and #9684 also still needs an agreed integration order/dependency where the shared behavior requires it; a general “whichever is ready” statement does not identify dependencies among those specific changes. Please record the agreed order in the checklist. This request is for the repository's current-main and overlap rules, not queue volume.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b1f5164 to
dca8f5b
Compare
|
Thanks for checking it against libthr. macOS: yes, the non-Windows branch of Windows: the TEB bounds were checked in the native runs under Tested, with WinLibs and with MSYS2 UCRT64. |
|
Refreshed onto On the new head, Linux passes the full build, Order: the description now proposes #8041 and #9684 first, then this PR, then #7149 and #4563, with what each step needs, and the checklist says |
What
scePthreadCreateignored a stack address set in the thread attributes. A thread created with one now runs on that memory, with the given size, on Linux and Windows. This finishes the caller-stack follow-up from #1988. The caller keeps ownership of the memory. Host startup still runs on the native stack, and TLS stays per host thread.Caller stacks report guard size 0. Thread identity and the stack's lifetime survive return, explicit exit, cancellation and key destructors. On Windows the TEB stack bounds cover the caller's range while the thread runs and are set back to the native bounds before it ends; existing context-stack registrations and pins stay. When
longjmp,siglongjmporsetcontextlands on a caller stack, the TEB gets that stack's range, also when a larger registered context stack contains it.A caller stack has no guard page below it; the libkernel debt file records this.
Merge order with the four open PRs that share behavior with this one:
retiredWinStacks. With fix(libkernel): keep a finished Windows thread's stack until it is joined or detached #4563AwaitReapruns after the exit bridge has switched back to the native stack, because the joiner may free the caller's stack once join returns.If one of you needs another order, reply here and I will change
Depends on. #7192 (@Jev1337), #8477 (@ycarogabriel-lgtm), #9160 (@yabanana), #9482 (@valenciana-nc) and #9573 (@zaryabzip) can merge before or after this PR. This comment lists what the later one keeps; with #9482, the new test's joined-thread case has to expectESRCH.Tested
On clean upstream
cecb540a, a probe thread's stack locals lie outside the range it was given.The new regression test covers all three setter paths, small stacks with large TLS, detach and reuse, and mixed cleanup chains including libc
_Thrd_exit. Threads with and without a caller stack catch a host exception and then unwind with the standard unwinder across the entry trampoline to the host stack. On Windows it also checks the TEB range after auc_linkreturn onto a caller stack inside a larger registered context stack.Linux x86_64, GCC 15.3.0, tree
9c08e18don upstream938812df(mainon 2026-10-23):libsand conventions pass. The regression passes, also in 20 repeats, and so do 8 other pthread tests and 9 neighbouring tests (libc threads, backtrace, mutex, cancel recovery, user context, memory, fixed mmap). Under ASan, 7 pthread tests and a separate range and lifecycle probe pass.agc_driver_sample_order,agc_driver_sample_order_grid_reference,agc_driver_sample_order_recipe_depth_locationsandagc_driver_sgpr_pattern_fill_keys. A clean build of938812dffails them with the same assertions.tools/check_skips.pyrejects 2 skips that come from this host:agc_driver_sparse_dma_buf_importandagc_host_import_sparseskip because its udmabufsize_limit_mbis above 1024, also on938812df.Windows 11 x64, local native run (not CI), tree
9c08e18don upstream938812df:libs, conventions and 2 memory tests.tools/check_skips.py --job windowsrejects 2 skips that come from this laptop:long_paths(long paths are disabled in the registry) andguest_video_out_high_frame_rate(the display has only 60 Hz modes).Broken versions of the change fail the regression. On Linux, with the trampoline's CFI damaged (CFA without the final
+16, or no CFA expression), it aborts at the unwind check. On Windows (WinLibs, treeebf12e33), ifGuestStackSwitch_nid_no_patchkeeps the current TEB range whenever it already contains the new stack pointer, it fails at theuc_linkcheck.Not covered: the host-bounds check runs on the main test thread and does not see a worker's final TEB state. macOS takes the same stack switch as Linux but was not built or run. Project CI, game compatibility and performance are not tested.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code