Repository navigation
Conversation
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The signal code can race with a second sender after the wait has consumed the pending bit. WaitForPendingInSet holds pending.lock while PendingInSet clears the bit, then returns and releases the lock; sigwaitinfo/sigtimedwait call FillSiginfo afterward, which loads Pending().codes[guest]. If another thread queues the same signal in that gap, MarkPending overwrites the code and the waiter reports the later sender for the signal it already consumed. The current tests use one sender at a time, so they do not cover this interleaving.
Please capture the code together with consuming the pending signal while holding pending.lock (for example, have PendingInSet return both the signal and its code), then pass that captured value to FillSiginfo. Add a focused concurrent regression if practical. I have not run this branch locally; conventions and eligibility pass, while product jobs are skipped by the current workflow.
|
Thanks, you are right: AI-assisted (Claude Code). |
2b68296 to
c46fb47
Compare
c46fb47 to
09605fb
Compare
ahmadmuj
left a comment
There was a problem hiding this comment.
Checked head 09605fb (base f26434a; also merges clean onto main 0119103).
What I ran, Linux (WSL2 Ubuntu 24.04, GCC 13.3, Release), guest_sigtimedwait 20 runs per arm, head test kept in every arm:
- head: 20/20 pass
- main's
Signals.cpp: 0/20 - first commit's
Signals.cpp(9a3c3ee): 14/20 pass, so here the race case only catches the old code some of the time
The CR fix looks right to me: codes[] is only written in MarkPending and read in PendingInSet, both under pending.lock, so the code always goes with the bit that was taken.
Three things, checked against FreeBSD release/11.4.0:
1. The new race test pins coalescing that FreeBSD doesn't do. sigqueue_add allocates one ksiginfo per send, standard signals too (kern_sig.c:403-414), and sigqueue_get keeps the bit while another one is queued (kern_sig.c:330-335). kill lands on p_sigqueue (pksignal -> tdsendsignal(p, NULL), kern_sig.c:2168), thr_kill on td_sigqueue (kern_sig.c:2170), and kern_sigtimedwait takes the thread queue first (kern_sig.c:1286-1287). So when both sends land before the wait, FreeBSD gives two waits, 0x10007 then 0x10001. The twice branch requires the opposite order, and the once branch only happens with coalescing.
Probe on the head, signal 2 blocked, both sends first, then poll until EAGAIN:
| Sends | Head | FreeBSD 11.4 (from the source) |
|---|---|---|
raise, raise |
0x10007, 1 wait | 0x10007, 0x10007 |
kill, pthread_kill(self) |
0x10007, 1 wait | 0x10007, 0x10001 |
pthread_kill(self), kill |
0x10001, 1 wait | 0x10007, 0x10001 |
The single bit is older than this PR, so a real queue can be a follow-up. But then the test shouldn't lock in the one-slot result, and last-sender-wins belongs in the debt entry. For example twice ? first.code != second.code : first.code == 0x10007 still fails on the old race (it showed up as 0x10007 twice) and accepts both FreeBSD orders.
2. sendCode leaks into sends from a handler that kill runs inline. kill -> raise_nid_postfix -> DeliverPendingSignal calls the guest handler while sendCode is still SI_USER (a host signal through std::raise -> Dispatch does the same). So a raise of a blocked signal inside that handler gets SI_USER. Probe: SIGUSR2 blocked, the SIGUSR1 handler calls raise(SIGUSR2), then kill(getpid(), SIGUSR1). sigtimedwait on SIGUSR2 reports 0x10001; FreeBSD gives SI_LWP (thr_kill, kern_thr.c:401). A siglongjmp out of that handler also skips ~SentByKill, and then every later raise on that thread reports SI_USER. Passing the code down explicitly fixes both: an internal raise that takes the code, and MarkPending with no default.
3. The debt entry is stale. The sigtimedwait entry in docs/dev/debt/libkernel.md still says only si_code (SI_USER) is written. CONTRIBUTING asks to cut it to what's still open, so: per-send queuing and last sender wins.
Not checked: Windows, macOS, a console. The codes come from the FreeBSD source only.
09605fb to
f9cc8ae
Compare
|
Thank you for the careful review and for measuring the earlier commit separately. I read the FreeBSD 11.4 sources for all three points, reproduced them with a probe on head 09605fb (Windows 10, MinGW-w64 GCC 15.2.0), and pushed a follow-up commit; the branch is rebased onto
Measured on head AI-assisted (Claude Code). |
…igtimedwait FreeBSD 11.4 stamps the queued signal with the sender: kill() uses SI_USER 0x10001 (sys/kern/kern_sig.c:1788), thr_kill, and so raise and pthread_kill, use SI_LWP 0x10007 (kern_thr.c:401, lib/libc/gen/raise.c:52), and the SIGALRM expiry of an interval timer comes from kern_psignal with SI_KERNEL 0x10006 and pid 0 (kern_time.c:899, kern_sig.c:2087; values in sys/sys/signal.h:400-409). The guest reported si_code 0 for all of them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…and sigtimedwait WaitForPendingInSet cleared the pending bit under pending.lock, but FillSiginfo read the code after the lock was released. A second sender could overwrite the code in between, so the wait reported the later sender for the signal it had already consumed. PendingInSet now returns the code together with the signal, under the lock, and FillSiginfo takes it as an argument. The test sends the same signal with kill and pthread_kill in each round and checks that the two reported codes never repeat the later sender. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
f9cc8ae to
92a7a10
Compare
|
Rebased onto Ninja exit code 0 read for every build. Windows 10: AI-assisted (Claude Code). |
|
Merged AI-assisted (Claude Code). |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Merged Two files conflicted. In
Tested on the merge head (Windows 10, MinGW-w64 GCC, Release): AI-assisted (Claude Code). |
|
A Windows run with GCC 16: Windows 10.0.22631 (Windows 11), i9-9900K, MSYS2 UCRT64 GCC 16.1.0 (Rev5), Release, this PR ( Repro for #8496: this PR's
So AI-assisted: yes (Claude Code) |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validation IDs already present on main (8 IDs in 622 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
WhatMerged The only conflict was the The change request from 2026-10-23 about the sender code racing with a second sender is answered by the commit that takes the signal and its code together in Tested
OverlapChecked against main 1e65852 with the 1738 open PRs listed at 17:41 UTC on 2026-11-03 (scan from 17:39 UTC to 18:19 UTC); the changed files of each PR come from AI-assisted (Claude Code). |
What
The guest queue of pending signals kept only a bit per signal, and
FillSiginfoalways wrotesi_code0 (and the caller's pid). FreeBSD 11.4 stamps the signal with the sender:killwithSI_USER(0x10001, sys/kern/kern_sig.c:1788),thr_kill(soraiseandpthread_kill) withSI_LWP(0x10007, kern_thr.c:401, lib/libc/gen/raise.c:52), and the interval-timer expiry withSI_KERNEL(0x10006) and pid 0 (kern_time.c:899, kern_sig.c:2087). Each pending signal now stores its code when it is marked, andsigwaitinfo/sigtimedwaitreturn it. The expectations inguest_sigtimedwaitthat pinned 0 change accordingly.Fixes #8496
The expectations of
guest_sigtimedwaitthat #4138 set to 0 change here; #4138 took thesiginfo_tlayout from FreeBSD 11 and did not measuresi_codeon a console, as far as its description says. Overlap:git merge-treeis clean for #8407, #8411, #8414 and #8467, and has conflicts with #3986 (3), #6044 (26) and #7201 (25).pr_overlapwas not run.A second commit takes the pending signal and its code together.
WaitForPendingInSetcleared the pending bit underpending.lock, butFillSiginforead the code after the lock was released, so a second sender could overwrite the code in between and the wait reported the later sender for the signal it had already consumed (review of 23 October).PendingInSetnow returns the code together with the signal under the lock, andFillSiginfotakes it as an argument. The rebase ontof26434afa3changed only type names (GuestTimespecisKernelTimespecafter #10706), one declaration in the test and the signature ofWaitForPendingInSet; the logic is unchanged.Tested
Base
mainf26434afa3, head09605fb4ad(two commits). One checkout and build directory per platform; for the "old" arm onlySystem/src/Signals.cppis taken back to the first commit (the code is read after the lock is released), the tests of the head are kept. Each build's ninja exit code 0 was read.guest_sigtimedwait_testsrun directly, 20 runs per arm: with the change 20 of 20 pass; with the oldSignals.cpp0 of 20 pass (all abort).ctest -R '^guest_sigtimedwait$', 10 runs per arm: with the change 10 of 10 pass. The old code also passes 10 of 10 in this run (0 of 10 failed), so on Windows the test does not reliably catch the race.CheckReportedCodeBelongsToConsumedSignal) sends the same signal withkilland then withpthread_killin each of 300 rounds; the wait may only report thepthread_killcode once, or thekillcode followed by thepthread_killcode. There is no deterministic hook between releasing the lock and reading the code, so it is a stress test.python tools/check_conventions.py --base upstream/mainpasses;git merge-treeagainstmainf26434afa3is clean.guest_signals,guest_itimer, ...) were run on the earlier basecabd80f472and are not repeated here.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repositoryAI-assisted (Claude Code).
🤖 Generated with Claude Code