Faulthandler by youknowone · Pull Request #6406 · RustPython/RustPython

17-57: LGTM!

The platform-specific FaultHandler structs are well-designed with appropriate storage for previous handlers (sigaction on Unix, sighandler_t on Windows). The const fn new() constructors enable compile-time initialization of the static arrays.

114-146: LGTM!

The FrameSnapshot design 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 (u32 on Windows, usize on Unix) correctly handles the libc::write signature differences.

221-282: LGTM!

The thread ID retrieval is platform-appropriate (pthread_self on Unix, GetCurrentThreadId on Windows). The hex width calculation using size_of::<usize>() * 2 correctly adapts to 32-bit vs 64-bit platforms.

284-319: Note: all_threads parameter is unused.

The _all_threads parameter 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 enable function correctly stores configuration in FATAL_ERROR state before installing signal handlers, with proper error handling if installation fails.

446-536: LGTM!

The signal handler implementation correctly:

1. Saves/restores errno for async-signal-safety
2. Disables the handler before dumping to prevent recursion
3. Re-raises the signal to allow default handling or chaining
4. Special-cases SIGSEGV on Windows to match CPython behavior

The iter_mut() on FAULTHANDLER_HANDLERS in signal context is covered by the module-level safety comment.

538-596: LGTM!

The enable implementations correctly use platform-appropriate APIs (sigaction with SA_NODEFER on Unix, signal on 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_signum validation correctly prevents user signal registration for fatal signals (which should use enable() 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_exception function provide test parity with CPython's faulthandler module.