Repository navigation
fix(libkernel): keep the signal mask per thread - #3986
Silverash-i8 wants to merge 1 commit into
Conversation
sigprocmask, _sigprocmask and pthread_sigmask shared one process-wide mask of the first 32 signals, so a thread changing its mask changed every thread's, the upper words read back as zero, SIGKILL and SIGSTOP could be blocked, and a signal raised while blocked was dropped (boykopovar#2879). Following FreeBSD's kern_sigprocmask, each guest thread now has its own 128-signal blocked and pending set. A new thread starts with its creator's mask, SIGKILL and SIGSTOP are removed from any mask that is set, and a raise of a blocked signal stays pending and is delivered when the thread unblocks it. An ignored signal is discarded rather than kept pending. Claude-Session: https://claude.ai/code/session_012SLQbhAiexxW6TtjYToW8a
|
Tested on native Linux (CachyOS x86_64, GCC) with an NVIDIA GeForce RTX 5060 Ti:
Validates the fix on native Linux and title execution as requested in the description. LGTM! |
|
One thing before merging: please drop the comment at |
Dextroid17
left a comment
There was a problem hiding this comment.
The control is what the record above does not have, and the overlap the bot lists is a text conflict rather than a same-file warning. Head 066b18da0, parent 0a8b1d32b, in a worktree:
head guest_signal + guest_signal_sets → 100% tests passed out of 2, exit 0
control parent's Pthread/src/Thread.cpp + System/src/Signals.cpp, the PR's tests kept
→ the test process exits 134 (SIGABRT, "Aborted (core dumped)"),
ctest: 691 - guest_signals (Subprocess aborted), 50% passed, 1 failed out of 2 (ctest exit 8)
The abort is deterministic and comes from the parent's own bookkeeping: it wrote previousSet->bits[1..3] = 0, while the new test asserts the previous mask above bit 31 comes back set (previous.bits[1] == 0x1, with bit 32 reaching bits[2] and bits[3]). That is the half a green head cannot show.
Two checks on the change itself:
- The mechanism fits this port.
GuestSignalSethas heldstd::uint32_t bits[4](Signals.cpp:24) since before this change; what was wrong is the mask bookkeeping — a singlestd::atomic<std::uint32_t> blockedMask, the operation applied toset->bits[0]only, andbits[1..3]returned as zero, so a mask above 32 was dropped and never read back. The change keeps the blocked and pending sets per thread and applies the operation to all four words. Per-thread storage is meaningful here because each guest thread runs on its own host thread:pthread_create(&p->hostThread, …, StartNativeThread, …)on the POSIX path (Thread.cpp:586),_beginthreadexon the Windows path (Thread.cpp:549), the entered thread adopted throughpthread_self()(Thread.cpp:692). - On the overlap, the bot's table is a same-file, same-line-range warning computed against
mainand says nothing about whether the two branches can merge; the measured state is that they cannot be merged without a decision.git merge-tree --write-tree pr-3986 pr-4138exits 1 withCONFLICT (content)incore/libs/prx/libkernel/System/src/Signals.cpp, in three regions — the blocked check inDispatch,raise_nid_postfix, and_sigprocmask_nid_postfix— where both branches add the same threehowvalues under differently capitalised names (GuestSigBlock/GuestSigUnblock/GuestSigSetmaskagainstGuestSigblock/GuestSigunblock/GuestSigsetmask, the second side also addingGuestEintr,GuestEagainandGuestSiuser) and both add pending-set handling.pr-3986withpr-4383andpr-4138withpr-4383produce trees with no textual conflict (exit 0; neither result was built or run, so that is all it shows). #4138's body already says it will keep this PR's per-thread masks and drop its own duplicate pending handling, so the conflict makes that rebase required rather than optional, and #4383 can go either side of the pair without cost. A line recording that would spare the next reader the experiment.
The comment oneandonlydean asked to be dropped sits at Signals.cpp:46-47 on this head (his range says 47-48); nothing above depends on it — and if the overlap is worth recording in the repository, docs/dev/TechnicalDebt.md in the same pull request is the place for it.
Written with AI assistance.
Hermes Agent 🤖 at your service
|
A small observation on the AI-assisted (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for this, the per-thread mask is the right model. It no longer applies to main, and the conflict is not only textual:
mainnow keeps a process-wide pending set inSignals.cpp(MarkPending/TakePending/PendingInSet, added withsigtimedwaitin #3430, used bysigwait,sigwaitinfo,pauseandkillpg). The per-threadpendingSignalshere is a second store for the same thing: a signal thatraiseleaves in it is never seen bysigwait/sigtimedwait. Please rebase and decide how the two relate (for example blocked mask per thread, pending set kept onmain's structure), then cover the interaction with a test.ExpireAlarm(setitimer/alarm, #3800) readsblockedMaskforSIGALRMandSignalBlockedonly looks at the first 32 signals. Both need to use the new per-thread mask, and the alarm thread is not a guest thread, so say which mask it checks.docs/dev/TechnicalDebt.mdwas split per library: the entry now lives indocs/dev/debt/libkernel.md, where the oldpthread_sigmaskline (line 123) and thesigtimedwaitline (149, which describes the process-wide pending set) both need updating.- @oneandonlydean's request is still open: drop the comment at
Signals.cpp:47-48. - @xXPBSXx's point about an unknown
how(write the old mask, returnEINVALinstead ofthrow, per FreeBSD 11.4kern_sigprocmask) touches the same lines you rewrite, so it fits in this PR.
After the rebase, guest_signals, guest_signal_sets, the sigtimedwait tests and guest_itimer should pass together, with the new guest_signals still failing on main's Signals.cpp/Thread.cpp.
|
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.
|
|
Conflicts with main; please rebase. Still open from 2026-10-13: @adriadam10's review above (how the per-thread mask relates to main's process-wide pending set, |
What
Fixes #2879.
sigprocmask,_sigprocmaskandpthread_sigmaskshared one process-wide mask that kept only the first 32 signals. A thread changing its mask changed every thread's, the upper words read back as zero, SIGKILL and SIGSTOP could be blocked, and a signal raised while blocked was dropped.Following FreeBSD's
kern_sigprocmask,Signals.cppnow keeps a blocked and a pending set of all 128 signals per guest thread:scePthreadCreatestarts the new thread with its creator's mask.raise(and sokillandpthread_killto self) of a blocked signal stays pending and is delivered beforesigprocmaskreturns from the unblock. A signal whose disposition is ignore is discarded instead of kept pending.guest_signalschecks a child thread inheriting its creator's mask, changing its own and leaving the creator's alone, a full-width round trip, the SIGKILL/SIGSTOP bits dropped, the deferred handler running on unblock, and an ignored blocked signal not running a handler later. The test it replaces expected the signal raised while blocked to be lost.The TechnicalDebt entry for the shared mask is replaced with the limitation left: a host signal from outside the guest (Ctrl+C, or
SIGTERMfrom another process) is checked against the mask of whichever host thread receives it and stays pending on that thread, where FreeBSD would deliver it to a thread that does not block it.Tested
On
main0a8b1d32:guest_signals,guest_signal_sets,guest_user_contextandguest_user_context_exitpass 5 of 5 (--repeat until-fail:5);sigsetjmp/siglongjmpandgetcontext/setcontextgo through_sigprocmask. All tests exceptagc_driver*: 540 of 540.agc_driver*: 533 of 535; the two failures come from the WSL environment and do not reach AnyPS5 code (pr_overlap_encodingusessubprocess._text_encoding, which Python 3.10 lacks, andlinux_executable_exportsstops compiling its own C probe,RTLD_DEFAULTundeclared). Not run on native Linux.main'sSignals.cppandThread.cpp, the newguest_signalsaborts on both systems (checked before the rebase onto0a8b1d32, ona11c8f7f).tools/check_conventions.py --base upstream/mainpasses.#3800 (
setitimer) reads the old process-wideblockedMaskinExpireAlarm, which this change removes, so whichever of the two lands second needs a small adaptation.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)https://claude.ai/code/session_012SLQbhAiexxW6TtjYToW8a