Faulthandler #6406
Faulthandler #6406
Conversation
WalkthroughThis pull request introduces Python faulthandler support to RustPython, including a restructured signal handling implementation with per-signal handler management, frame snapshot capabilities, and integration with the VM startup sequence via CLI options and environment variables. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50–70 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// faulthandler_handlers[] | ||
| /// Number of fatal signals | ||
| #[cfg(unix)] | ||
| const FAULTHANDLER_NSIGNALS: usize = 5; | ||
| #[cfg(windows)] | ||
| const FAULTHANDLER_NSIGNALS: usize = 3; | ||
|
|
||
| // CPython uses static arrays for signal handlers which requires mutable static access. | ||
| // This is safe because: | ||
| // 1. Signal handlers run in a single-threaded context (from the OS perspective) | ||
| // 2. FAULTHANDLER_HANDLERS is only modified during enable/disable operations | ||
| // 3. This matches CPython's faulthandler.c implementation | ||
| #[allow(static_mut_refs)] | ||
| #[cfg(unix)] | ||
| static mut FAULTHANDLER_HANDLERS: [FaultHandler; FAULTHANDLER_NSIGNALS] = unsafe { | ||
| [ | ||
| FaultHandler { | ||
| signum: libc::SIGBUS, | ||
| enabled: false, | ||
| name: "Bus error", | ||
| previous: std::mem::zeroed(), | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGILL, | ||
| enabled: false, | ||
| name: "Illegal instruction", | ||
| previous: std::mem::zeroed(), | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGFPE, | ||
| enabled: false, | ||
| name: "Floating-point exception", | ||
| previous: std::mem::zeroed(), | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGABRT, | ||
| enabled: false, | ||
| name: "Aborted", | ||
| previous: std::mem::zeroed(), | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGSEGV, | ||
| enabled: false, | ||
| name: "Segmentation fault", | ||
| previous: std::mem::zeroed(), | ||
| }, | ||
| ] | ||
| }; | ||
|
|
||
| #[allow(static_mut_refs)] | ||
| #[cfg(windows)] | ||
| static mut FAULTHANDLER_HANDLERS: [FaultHandler; FAULTHANDLER_NSIGNALS] = [ | ||
| FaultHandler { | ||
| signum: libc::SIGFPE, | ||
| enabled: false, | ||
| name: "Floating-point exception", | ||
| previous: 0, | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGABRT, | ||
| enabled: false, | ||
| name: "Aborted", | ||
| previous: 0, | ||
| }, | ||
| FaultHandler { | ||
| signum: libc::SIGSEGV, | ||
| enabled: false, | ||
| name: "Segmentation fault", | ||
| previous: 0, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
When I was workshopping this on my own, prior to #6400, I came up with something similar to this, finding out from the libc repository that SIGILL does exist on Windows. Would this work?
| /// faulthandler_handlers[] | |
| /// Number of fatal signals | |
| #[cfg(unix)] | |
| const FAULTHANDLER_NSIGNALS: usize = 5; | |
| #[cfg(windows)] | |
| const FAULTHANDLER_NSIGNALS: usize = 3; | |
| // CPython uses static arrays for signal handlers which requires mutable static access. | |
| // This is safe because: | |
| // 1. Signal handlers run in a single-threaded context (from the OS perspective) | |
| // 2. FAULTHANDLER_HANDLERS is only modified during enable/disable operations | |
| // 3. This matches CPython's faulthandler.c implementation | |
| #[allow(static_mut_refs)] | |
| #[cfg(unix)] | |
| static mut FAULTHANDLER_HANDLERS: [FaultHandler; FAULTHANDLER_NSIGNALS] = unsafe { | |
| [ | |
| FaultHandler { | |
| signum: libc::SIGBUS, | |
| enabled: false, | |
| name: "Bus error", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGILL, | |
| enabled: false, | |
| name: "Illegal instruction", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGFPE, | |
| enabled: false, | |
| name: "Floating-point exception", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGABRT, | |
| enabled: false, | |
| name: "Aborted", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGSEGV, | |
| enabled: false, | |
| name: "Segmentation fault", | |
| previous: std::mem::zeroed(), | |
| }, | |
| ] | |
| }; | |
| #[allow(static_mut_refs)] | |
| #[cfg(windows)] | |
| static mut FAULTHANDLER_HANDLERS: [FaultHandler; FAULTHANDLER_NSIGNALS] = [ | |
| FaultHandler { | |
| signum: libc::SIGFPE, | |
| enabled: false, | |
| name: "Floating-point exception", | |
| previous: 0, | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGABRT, | |
| enabled: false, | |
| name: "Aborted", | |
| previous: 0, | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGSEGV, | |
| enabled: false, | |
| name: "Segmentation fault", | |
| previous: 0, | |
| }, | |
| ]; | |
| // CPython uses static arrays for signal handlers which requires mutable static access. | |
| // This is safe because: | |
| // 1. Signal handlers run in a single-threaded context (from the OS perspective) | |
| // 2. FAULTHANDLER_HANDLERS is only modified during enable/disable operations | |
| // 3. This matches CPython's faulthandler.c implementation | |
| #[allow(static_mut_refs)] | |
| static mut FAULTHANDLER_HANDLERS: [FaultHandler; if cfg!(unix) {5} else {4}] = unsafe { | |
| [ | |
| #[cfg(unix)] | |
| FaultHandler { | |
| signum: libc::SIGBUS, | |
| enabled: false, | |
| name: "Bus error", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGILL, | |
| enabled: false, | |
| name: "Illegal instruction", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGFPE, | |
| enabled: false, | |
| name: "Floating-point exception", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGABRT, | |
| enabled: false, | |
| name: "Aborted", | |
| previous: std::mem::zeroed(), | |
| }, | |
| FaultHandler { | |
| signum: libc::SIGSEGV, | |
| enabled: false, | |
| name: "Segmentation fault", | |
| previous: std::mem::zeroed(), | |
| }, | |
| ] | |
| }; | |
| /// faulthandler_handlers[] | |
| /// Number of fatal signals | |
| const FAULTHANDLER_NSIGNALS: usize = FAULTHANDLER_HANDLERS.len(); |
You might also want a FaultHandler::default() implementation to same some repetition in the future.
32c6419 to
7442ff1
Compare
7442ff1 to
2b5340c
Compare
2b5340c to
cf3ad0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/stdlib/src/faulthandler.rs (1)
59-99: Consider future migration fromstatic mutpattern.The current implementation mirrors CPython's approach with
static mutfor signal handlers, which is acceptable for this use case. The#[allow(static_mut_refs)]at the module level (line 3) acknowledges this. However,static_mut_refsis being deprecated in Rust.For future consideration, you could explore using
std::cell::UnsafeCellwith a wrapper type orstd::sync::OnceLockfor the parts that are only written during enable/disable. This is not blocking for this PR since it matches CPython's design.src/lib.rs (1)
190-198: Consider logging faulthandler initialization failures.The initialization correctly runs after site import, matching CPython's startup sequence. However, silently ignoring failures with
let _ =may hide configuration issues. Consider logging a warning on failure:// Enable faulthandler if -X faulthandler, PYTHONFAULTHANDLER or -X dev is set // _PyFaulthandler_Init() if vm.state.settings.faulthandler { - let _ = vm.run_code_string( + if let Err(e) = vm.run_code_string( vm.new_scope_with_builtins(), "import faulthandler; faulthandler.enable()", "<faulthandler>".to_owned(), - ); + ) { + warn!("Failed to enable faulthandler: {:?}", e); + } }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_cmd_line.pyis excluded by!Lib/**Lib/test/test_faulthandler.pyis excluded by!Lib/**
📒 Files selected for processing (5)
.cspell.dict/python-more.txt(1 hunks)crates/stdlib/src/faulthandler.rs(5 hunks)crates/vm/src/vm/setting.rs(2 hunks)src/lib.rs(1 hunks)src/settings.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
src/lib.rscrates/vm/src/vm/setting.rssrc/settings.rscrates/stdlib/src/faulthandler.rs
🧠 Learnings (2)
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
src/lib.rscrates/stdlib/src/faulthandler.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
src/lib.rscrates/stdlib/src/faulthandler.rs
🧬 Code graph analysis (1)
src/settings.rs (1)
src/interpreter.rs (1)
settings(59-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (17)
.cspell.dict/python-more.txt (1)
173-173: LGTM!The dictionary entry for
PYTHONFAULTHANDLERis correctly added in alphabetical order, matching the new environment variable support in settings.crates/stdlib/src/faulthandler.rs (12)
17-57: LGTM!The platform-specific
FaultHandlerstructs are well-designed with appropriate storage for previous handlers (sigactionon Unix,sighandler_ton Windows). Theconst fn new()constructors enable compile-time initialization of the static arrays.
114-146: LGTM!The
FrameSnapshotdesign with fixed-size arrays is correct for signal-safe contexts where heap allocation is forbidden. The sizing (256 bytes for filename, 128 for funcname, 100 max frames) provides reasonable limits.
148-219: LGTM!The signal-safe output functions correctly avoid heap allocation and use raw
libc::write. Ignoring the return value is acceptable in signal handler context where there's limited recovery options. The platform-specific length type casting (u32on Windows,usizeon Unix) correctly handles thelibc::writesignature differences.
221-282: LGTM!The thread ID retrieval is platform-appropriate (
pthread_selfon Unix,GetCurrentThreadIdon Windows). The hex width calculation usingsize_of::<usize>() * 2correctly adapts to 32-bit vs 64-bit platforms.
284-319: Note:all_threadsparameter is unused.The
_all_threadsparameter is ignored. This is acceptable for an initial implementation since dumping all threads' tracebacks from a signal handler context is complex (requires thread enumeration and accessing other threads' frame stacks).
401-419: LGTM!The
enablefunction correctly stores configuration inFATAL_ERRORstate before installing signal handlers, with proper error handling if installation fails.
446-536: LGTM!The signal handler implementation correctly:
- Saves/restores errno for async-signal-safety
- Disables the handler before dumping to prevent recursion
- Re-raises the signal to allow default handling or chaining
- Special-cases
SIGSEGVon Windows to match CPython behaviorThe
iter_mut()onFAULTHANDLER_HANDLERSin signal context is covered by the module-level safety comment.
538-596: LGTM!The enable implementations correctly use platform-appropriate APIs (
sigactionwithSA_NODEFERon Unix,signalon Windows). The early return when already enabled prevents duplicate handler registration.
598-629: LGTM!The disable implementation correctly uses atomic swap to get previous state while disabling. The Python-exposed
disable()function returns the previous enabled state, matching CPython's API.
953-970: LGTM!The
check_signumvalidation correctly prevents user signal registration for fatal signals (which should useenable()instead) and validates the signal number range.
1079-1093: Verify the infinite loop for Windows SIGSEGV.The infinite loop around
libc::raise(libc::SIGSEGV)on Windows is unusual. Is this intentional to work around Windows signal delivery behavior? If so, a brief comment explaining why would be helpful.#[cfg(windows)] { // On Windows, we need to raise SIGSEGV multiple times + // because Windows signal handling may not immediately terminate loop { unsafe { libc::raise(libc::SIGSEGV); } } }
1169-1208: LGTM!The Windows-specific exception constants and
_raise_exceptionfunction provide test parity with CPython's faulthandler module.crates/vm/src/vm/setting.rs (2)
22-24: LGTM!The
faulthandlerfield is correctly added as an ungated boolean with appropriate documentation referencing both-X faulthandlerandPYTHONFAULTHANDLERenvironment variable.
162-162: LGTM!Default initialization to
falseis correct—faulthandler should be explicitly opt-in.src/settings.rs (2)
270-270: LGTM!The
-X faulthandleroption is correctly handled in the implementation options parser, settingsettings.faulthandler = true.
293-300: LGTM!The faulthandler configuration correctly:
- Preserves CLI setting with logical OR for environment variable
- Enables faulthandler in dev mode, matching CPython's
-X devbehavior
Summary by CodeRabbit
-X faulthandlercommand-line flag, setPYTHONFAULTHANDLERenvironment variable, or automatic activation in development mode✏️ Tip: You can customize this high-level summary in your review settings.