Remove unneeded save-signal-mask FIXME#10
Open
gc00 wants to merge 1 commit into
Open
Conversation
getcontext()/setcontext() already restore a thread's blocked-signal set via uc_sigmask, even across the raw clone() used to recreate a checkpointed thread. The thread_sigmask field and abort were unnecessary; both are removed. Also drops BUG.README from version control.
📝 WalkthroughWalkthroughRemoves the manual per-thread signal mask field and associated save/restore/validation logic in dmtcp-callback.c, relying instead on ucontext_t's uc_sigmask for persisting blocked signals across getcontext()/setcontext(). Also clears all content from BUG.README. ChangesSignal Mask Handling Simplification
Estimated code review effort: 3 (Moderate) | ~20 minutes Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/dmtcp-callback.c`:
- Around line 126-130: The signal-mask handling around getcontext()/setcontext()
in dmtcp callback setup drops the required SIG_MULTITHREADED_FORK unblock on the
child resume path. Update the logic in child_setcontext_fast() and the related
context-saving path so the new thread restores the saved ucontext_t state
without leaving SIG_MULTITHREADED_FORK permanently blocked, preserving
multithreaded fork participation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51795434-8ac1-4a22-9b5c-1ff5b5c3d425
📒 Files selected for processing (2)
BUG.READMEsrc/lib/dmtcp-callback.c
💤 Files with no reviewable changes (1)
- BUG.README
Comment on lines
+126
to
130
| // No manual signal-mask save/restore needed: getcontext()/setcontext() | ||
| // already save/restore the blocked-signal set via ucontext_t's uc_sigmask, | ||
| // even when setcontext() resumes on a brand-new clone()'d OS thread (see | ||
| // child_setcontext_fast() below). | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate getcontext call site, child resume path, and any SIG_MULTITHREADED_FORK (un)masking.
fd -t f 'dmtcp-callback.c' --exec rg -nP -C4 \
'getcontext|setcontext|SIG_MULTITHREADED_FORK|sigdelset|pthread_sigmask|sigprocmask|child_setcontext_fast|thread_handle_after_dmtcp_restart' {}
# Cross-check the shared fork implementation for where the fork signal is unblocked.
fd -t f 'multithreaded_fork.c' --exec rg -nP -C3 \
'SIG_MULTITHREADED_FORK|sigdelset|sigaddset|pthread_sigmask|sigprocmask|uc_sigmask' {}Repository: mcminickpt/deep-debug
Length of output: 6625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant sections in both files with line numbers.
sed -n '225,255p' src/common/multithreaded_fork.c
printf '\n---\n'
sed -n '400,450p' src/common/multithreaded_fork.c
printf '\n---\n'
sed -n '110,145p' src/lib/dmtcp-callback.cRepository: mcminickpt/deep-debug
Length of output: 4628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit unblock/removal of SIG_MULTITHREADED_FORK on the child resume path.
rg -n "SIG_MULTITHREADED_FORK|pthread_sigmask|sigprocmask|sigdelset|sigaddset" src/lib/dmtcp-callback.c src/common/multithreaded_fork.cRepository: mcminickpt/deep-debug
Length of output: 1435
Restore the SIG_MULTITHREADED_FORK mask handling
getcontext() runs inside the SIG_MULTITHREADED_FORK handler, so the saved uc_sigmask includes that signal blocked. child_setcontext_fast() restores that mask on the new thread, and there’s no explicit unblock on the child resume path. That can leave this thread unable to participate in a later multithreaded fork and hang the process.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/dmtcp-callback.c` around lines 126 - 130, The signal-mask handling
around getcontext()/setcontext() in dmtcp callback setup drops the required
SIG_MULTITHREADED_FORK unblock on the child resume path. Update the logic in
child_setcontext_fast() and the related context-saving path so the new thread
restores the saved ucontext_t state without leaving SIG_MULTITHREADED_FORK
permanently blocked, preserving multithreaded fork participation.
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.
getcontext()/setcontext() already restore a thread's blocked-signal set via uc_sigmask, even across the raw clone() used to recreate a checkpointed thread. The thread_sigmask field and abort were unnecessary; both are removed.
Also drops BUG.README from version control.
Summary by CodeRabbit