Update faulthandler to 3.14.2#6949
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughPropagates call-expression source ranges into CALL bytecode emission; introduces lock-free, signal-safe per-thread frame pointers and per-thread frame stacks; replaces snapshot-based faulthandler traces with live-frame traversal; adds signal reentrancy guard and updates signal/exception registration and related VM APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Thread
participant VM as VirtualMachine.with_frame()
participant FrameStack as Per-thread Frame Stack
participant CurrentFrame as CURRENT_FRAME (AtomicPtr)
participant Signal as OS Signal/Exception
participant Faulthandler as Faulthandler
Thread->>VM: enter frame
VM->>FrameStack: push_thread_frame(new_frame)
VM->>CurrentFrame: set_current_frame(new_frame)
Note over VM: new_frame.previous = old_frame (atomic pointer)
Thread->>Thread: execute bytecode (calls emitted with call_range)
Signal-->>Faulthandler: signal/exception delivered
Faulthandler->>CurrentFrame: get_current_frame() (atomic, no lock)
loop walk frames via previous pointers
Faulthandler->>Faulthandler: read frame.previous
Faulthandler->>Faulthandler: render frame entry
end
Faulthandler->>Signal: restore/return handler
Thread->>VM: exit frame
VM->>CurrentFrame: set_current_frame(restored_frame)
VM->>FrameStack: pop_thread_frame()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
3633628 to
ccf40fd
Compare
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin faulthandler |
e3e072b to
7ee743e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/vm/src/vm/thread.rs (1)
1-106:⚠️ Potential issue | 🟠 MajorUse
AtomicPtrforCURRENT_FRAMEto ensure signal-safe access.
Cell<*const Frame>is not safe when accessed from a signal handler. Thereplace()andget()operations are not atomic and can be torn if a signal interruptsset_current_frame(). Replace withAtomicPtr<Frame>usingOrdering::Relaxedfor atomic, signal-safe pointer updates.🔧 Proposed fix
-use core::{ - cell::{Cell, RefCell}, - ptr::NonNull, -}; +use core::{ + cell::{Cell, RefCell}, + ptr::NonNull, + sync::atomic::{AtomicPtr, Ordering}, +}; @@ - pub(crate) static CURRENT_FRAME: Cell<*const Frame> = const { Cell::new(core::ptr::null()) }; + pub(crate) static CURRENT_FRAME: AtomicPtr<Frame> = + const { AtomicPtr::new(core::ptr::null_mut()) }; @@ pub fn set_current_frame(frame: *const Frame) -> *const Frame { - CURRENT_FRAME.with(|c| c.replace(frame)) + CURRENT_FRAME.with(|c| c.swap(frame as *mut Frame, Ordering::Relaxed) as *const Frame) } @@ pub fn get_current_frame() -> *const Frame { - CURRENT_FRAME.with(|c| c.get()) + CURRENT_FRAME.with(|c| c.load(Ordering::Relaxed) as *const Frame) }crates/stdlib/src/faulthandler.rs (2)
1052-1085:⚠️ Potential issue | 🟡 MinorThe
all_threadssetting in user signal handler only affects the header, not the actual dump.When
all_threads=true, the header says "Current thread..." but only the current thread's frames are dumped viadump_live_frames(). If the intent is to dump all threads whenall_threads=true, this needs to iterate over thread frame slots likedump_all_threads()does.If dumping only the current thread is intentional for signal safety reasons (which is valid - accessing other threads' frames from a signal handler is unsafe), consider:
- Changing the header to always say "Stack (most recent call first):" regardless of
all_threads, or- Documenting that
all_threadsis ignored in signal handlers
1323-1331:⚠️ Potential issue | 🔴 CriticalRemove undefined function call
snapshot_current_frames(vm)at line 1327—this will cause a compilation error.The function
snapshot_current_framesdoes not exist anywhere in the codebase. It is called only once (line 1327) and is never defined, imported, or conditionally compiled elsewhere. This is dead code from an earlier refactoring that removed snapshot-based frame access.Fix
#[cfg(windows)] #[pyfunction] fn _raise_exception(args: RaiseExceptionArgs, vm: &VirtualMachine) { use windows_sys::Win32::System::Diagnostics::Debug::RaiseException; suppress_crash_report(); - snapshot_current_frames(vm); + let _ = vm; unsafe { RaiseException(args.code, args.flags, 0, core::ptr::null()); } }
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 4626-4628: In compile_class_def, capture the class statement
source range at the start (e.g., let class_stmt_range =
self.current_source_range.clone() or equivalent) before compiling
decorators/body, and then use that saved class_stmt_range when emitting the
__build_class__ call (replace uses of self.current_source_range in the
codegen_call_helper(2, arguments, self.current_source_range)? invocation with
the saved class_stmt_range) so the call site points to the class statement
instead of a range overwritten by decorator/body compilation.
In `@crates/vm/src/vm/mod.rs`:
- Around line 941-951: The current sequence publishes the new frame via
set_current_frame(frame_ptr) before setting frame.previous, causing a race where
a signal handler can see the new frame with a null previous pointer; fix by
reading the current thread frame first (use
crate::vm::thread::get_current_frame() or equivalent) and store that pointer
into frame.previous with frame.previous.store(..., Ordering::Relaxed) before
calling crate::vm::thread::set_current_frame(frame_ptr); keep the existing push
to self.frames and the #[cfg(feature = "threading")]
crate::vm::thread::push_thread_frame(frame.clone()) but ensure the previous-link
is populated prior to publishing the new frame pointer.
🧹 Nitpick comments (1)
crates/stdlib/src/faulthandler.rs (1)
122-147: Consider consolidatingputsandputs_bytesto reduce duplication.Both functions have nearly identical implementations differing only in the input type. Since
&strcan be converted to&[u8]viaas_bytes(), you could haveputsdelegate toputs_bytes.♻️ Proposed refactor
#[cfg(any(unix, windows))] fn puts(fd: i32, s: &str) { - let _ = unsafe { - #[cfg(windows)] - { - libc::write(fd, s.as_ptr() as *const libc::c_void, s.len() as u32) - } - #[cfg(not(windows))] - { - libc::write(fd, s.as_ptr() as *const libc::c_void, s.len()) - } - }; + puts_bytes(fd, s.as_bytes()); }
| if let Some(arguments) = arguments { | ||
| self.codegen_call_helper(2, arguments)?; | ||
| self.codegen_call_helper(2, arguments, self.current_source_range)?; | ||
| } else { |
There was a problem hiding this comment.
Preserve class statement range for build_class call.
current_source_range is likely overwritten while compiling decorators and the class body, so the call location can incorrectly point inside the class body instead of the class statement. Capture the class statement range at the start of compile_class_def and reuse it here.
🛠️ Suggested fix
fn compile_class_def(
&mut self,
name: &str,
body: &[ast::Stmt],
decorator_list: &[ast::Decorator],
type_params: Option<&ast::TypeParams>,
arguments: Option<&ast::Arguments>,
) -> CompileResult<()> {
+ // Preserve the class statement range for __build_class__ call location.
+ let class_call_range = self.current_source_range;
self.prepare_decorators(decorator_list)?;
let is_generic = type_params.is_some();
let firstlineno = self.get_source_line_number().get().to_u32();
@@
- if let Some(arguments) = arguments {
- self.codegen_call_helper(2, arguments, self.current_source_range)?;
+ if let Some(arguments) = arguments {
+ self.codegen_call_helper(2, arguments, class_call_range)?;
} else {
emit!(self, Instruction::Call { nargs: 2 });
}🤖 Prompt for AI Agents
In `@crates/codegen/src/compile.rs` around lines 4626 - 4628, In
compile_class_def, capture the class statement source range at the start (e.g.,
let class_stmt_range = self.current_source_range.clone() or equivalent) before
compiling decorators/body, and then use that saved class_stmt_range when
emitting the __build_class__ call (replace uses of self.current_source_range in
the codegen_call_helper(2, arguments, self.current_source_range)? invocation
with the saved class_stmt_range) so the call site points to the class statement
instead of a range overwritten by decorator/body compilation.
ed5be25 to
e537baf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/vm/thread.rs`:
- Around line 29-33: The CURRENT_FRAME static must be an atomic pointer so
signal handlers can read it safely; replace the Cell<*const Frame> static with
an AtomicPtr<Frame> initialized to null (use
AtomicPtr::new(core::ptr::null_mut() or equivalent) ) and update all uses
(including set_current_frame and any direct reads) to use atomic store/load with
Ordering::Relaxed; ensure conversions between *const Frame and *mut Frame are
handled where needed and update any helper functions that previously assumed
Cell to call AtomicPtr::store/load instead.
🧹 Nitpick comments (3)
crates/stdlib/src/faulthandler.rs (3)
334-357: Inconsistent thread header output compared towrite_thread_id.
dump_traceback_thread_framesmanually writes the thread header inline (lines 342-348) instead of reusingwrite_thread_id. While functionally similar, this duplicates the format logic. Consider extracting a common helper or reusingwrite_thread_idfor consistency.♻️ Suggested refactor to reuse write_thread_id
The
write_thread_idfunction could be modified to accept a thread_id parameter instead of always usingcurrent_thread_id(), then both call sites could use the same function.#[cfg(any(unix, windows))] -fn write_thread_id(fd: i32, is_current: bool) { +fn write_thread_id(fd: i32, thread_id: u64, is_current: bool) { if is_current { puts(fd, "Current thread "); } else { puts(fd, "Thread "); } - let thread_id = current_thread_id(); // dump_hexadecimal already writes "0x" prefix dump_hexadecimal(fd, thread_id, core::mem::size_of::<usize>() * 2); puts(fd, " (most recent call first):\n"); }
866-879: Watchdog thread marks all threads as non-current.When the watchdog dumps tracebacks, all threads are passed
is_current: false(line 872), so none are labeled "Current thread". This may be intentional since the watchdog runs in its own thread, but it differs from CPython's behavior where the thread that triggered the timeout is marked as current.For the non-threading path (lines 876-879),
write_thread_idis called withis_current: falsewhich also seems inconsistent—in a single-threaded build, the main thread should arguably be marked as current.♻️ Suggested fix for non-threading case
#[cfg(not(feature = "threading"))] { - write_thread_id(fd, false); + write_thread_id(fd, true); dump_live_frames(fd); }
1051-1085:all_threadsflag doesn't dump all threads in signal context.The
faulthandler_user_signalhandler checksuser.all_threadsbut only affects the header text (lines 1058-1063). The actual frame dumping viadump_live_frames(line 1066) only dumps the current thread regardless of this flag.This is likely a deliberate limitation for signal safety (can't safely access other threads' data from a signal handler), but the behavior is inconsistent with what
all_threads=Truewould suggest. Consider adding a comment documenting this limitation, or adjusting the header whenall_threadsis true to avoid implying all threads will be dumped.📝 Suggested documentation improvement
#[cfg(unix)] extern "C" fn faulthandler_user_signal(signum: libc::c_int) { let user = match user_signals::get_user_signal(signum as usize) { Some(u) if u.enabled => u, _ => return, }; - // Write traceback header based on all_threads setting + // Note: In signal context, we can only safely dump the current thread. + // The all_threads flag only affects the header format for compatibility. if user.all_threads { write_thread_id(user.fd, true); } else { puts(user.fd, "Stack (most recent call first):\n"); } - // Dump live frame chain (walks Frame.previous pointers) + // Dump current thread's live frame chain only (signal-safe limitation) dump_live_frames(user.fd);
e537baf to
dd38c0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/stdlib/src/faulthandler.rs`:
- Around line 252-281: The truncation of function names in dump_frame_from_raw
and dump_frame_from_ref currently slices &funcname[..MAX_FUNCTION_NAME_LEN]
which can cut a UTF-8 codepoint and panic; create a small helper (e.g.
safe_truncate_funcname) that takes &str and MAX_FUNCTION_NAME_LEN, checks
is_char_boundary at that byte index and if not valid decrements the index until
it is a valid boundary, then returns the &str slice to use with puts; update
both dump_frame_from_raw and dump_frame_from_ref to call this helper before
calling puts and preserve the "..." behavior when truncation occurred.
- Around line 283-302: faulthandler_dump_traceback currently ignores the
all_threads flag (it only changes the header) and only dumps the current thread
via dump_live_frames/get_current_frame; either implement a signal-safe
multi-thread capture or explicitly document/emit that limitation—do the quick
fix by making faulthandler_dump_traceback detect when all_threads==true and emit
a clear signal-safe message (using puts/write_thread_id) that other threads
cannot be dumped from a fatal signal handler, and keep calling dump_live_frames
for the current thread; reference faulthandler_dump_traceback, dump_live_frames,
get_current_frame, and the non-signal functions dump_traceback/dump_all_threads
so the change is easy to locate.
In `@crates/vm/src/vm/thread.rs`:
- Around line 30-36: The CURRENT_FRAME atomic is currently stored inside
thread_local! and accessed via get_current_frame(), which uses TLS lookups that
are not async-signal-safe; move CURRENT_FRAME out of the thread_local! wrapper
to a plain static per-thread storage accessible without invoking
thread_local!.with() (e.g., a dedicated static AtomicPtr<Frame> per OS thread or
a non-TLS global map updated manually on thread enter/exit) and update
get_current_frame(), thread enter/exit logic, and any callers that read/write
CURRENT_FRAME so they use the new signal-safe static access pattern instead of
thread_local! lookups.
| /// Current top frame for signal-safe traceback walking. | ||
| /// Mirrors `PyThreadState.current_frame`. Read by faulthandler's signal | ||
| /// handler to dump tracebacks without accessing RefCell or locks. | ||
| /// Uses AtomicPtr for async-signal-safety (signal handlers may read this | ||
| /// while the owning thread is writing). | ||
| pub(crate) static CURRENT_FRAME: AtomicP 8000 tr<Frame> = | ||
| const { AtomicPtr::new(core::ptr::null_mut()) }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code in the target file
cd crates/vm/src/vm && head -50 thread.rs | cat -nRepository: RustPython/RustPython
Length of output: 2286
🏁 Script executed:
# Get more context around lines 30-36
sed -n '1,50p' crates/vm/src/vm/thread.rs | cat -nRepository: RustPython/RustPython
Length of output: 2286
🏁 Script executed:
# Search for CURRENT_FRAME definition and usage patterns
rg 'CURRENT_FRAME' crates/vm/src/vm/thread.rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 2546
🏁 Script executed:
# Search for get_current_frame function
rg 'get_current_frame' crates/vm/src/ -A 10Repository: RustPython/RustPython
Length of output: 819
🏁 Script executed:
# Check if there are any thread_local macros in the file
rg 'thread_local!' crates/vm/src/vm/thread.rsRepository: RustPython/RustPython
Length of output: 149
🌐 Web query:
Rust thread_local async-signal-safety POSIX signal handlers
💡 Result:
- Don’t use
thread_local!()(TLS) from a POSIX signal handler in Rust. TLS access is not async-signal-safe; even “just reading” a TLS slot can involve runtime/TLS machinery that POSIX does not guarantee is safe in an async handler. (users.rust-lang.org) - In POSIX, a signal handler must restrict itself to async-signal-safe operations/functions (the standard provides a specific allowlist; e.g.,
write,_exit,sigaction, etc.). Anything not on that list is unsafe to call from the handler (may deadlock, corrupt global state, etc.). (man7.org) errnonote: reading/writingerrnois considered async-signal-safe if you save it on entry and restore it before returning. (manpages.debian.org)
Practical Rust pattern: do the absolute minimum in the handler (typically set an AtomicBool/AtomicUsize and/or write() 1 byte to a pipe/socket), then handle the real logic on a normal thread/task. This is the common “self-pipe” style design used by Rust signal crates. (man7.org)
Citations:
- 1: https://users.rust-lang.org/t/async-signal-safety-of-core-and-std/127821?utm_source=openai
- 2: https://man7.org/linux/man-pages/man7/signal-safety.7.html?utm_source=openai
- 3: https://manpages.debian.org/testing/manpages/signal-safety.7.en.html?utm_source=openai
- 4: https://man7.org/linux/man-pages/man7/signal-safety.7.html?utm_source=openai
🏁 Script executed:
# Search for faulthandler or signal handler usage with get_current_frame
rg 'faulthandler|signal' crates/vm/src/ -A 5 -B 2 | head -100Repository: RustPython/RustPython
Length of output: 5811
🏁 Script executed:
# Check if get_current_frame is called from any signal-related code
rg 'get_current_frame' crates/vm/src/ -B 5 -A 5Repository: RustPython/RustPython
Length of output: 795
🏁 Script executed:
# Look for signal handler registration or unsafe signal code
rg 'signal|SignalHandler|SIGABRT|SIGSEGV' crates/vm/src/ -lRepository: RustPython/RustPython
Length of output: 507
CURRENT_FRAME cannot be safely accessed from signal handlers due to thread-local! machinery.
While AtomicPtr itself is async-signal-safe, thread_local! access via .with() is not. POSIX signal handlers must restrict themselves to the async-signal-safe function list, which does not include TLS lookups. Accessing CURRENT_FRAME from get_current_frame() in a signal handler can deadlock or corrupt state.
To achieve true signal safety, store CURRENT_FRAME as a plain static (not within thread_local!), accepting that it requires manual per-thread management for sys._current_frames() or implement a signal-safe variant of frame access (e.g., using a separate, non-TLS global data structure for faulthandler).
🤖 Prompt for AI Agents
In `@crates/vm/src/vm/thread.rs` around lines 30 - 36, The CURRENT_FRAME atomic is
currently stored inside thread_local! and accessed via get_current_frame(),
which uses TLS lookups that are not async-signal-safe; move CURRENT_FRAME out of
the thread_local! wrapper to a plain static per-thread storage accessible
without invoking thread_local!.with() (e.g., a dedicated static AtomicPtr<Frame>
per OS thread or a non-TLS global map updated manually on thread enter/exit) and
update get_current_frame(), thread enter/exit logic, and any callers that
read/write CURRENT_FRAME so they use the new signal-safe static access pattern
instead of thread_local! lookups.
- Rewrite faulthandler with live frame walking via Frame.previous AtomicPtr chain and thread-local CURRENT_FRAME (AtomicPtr) instead of frame snapshots - Add signal-safe traceback dumping (dump_live_frames, dump_frame_from_raw) walking the Frame.previous chain - Add safe_truncate/dump_ascii for UTF-8 safe string truncation in signal handlers - Refactor write_thread_id to accept thread_id parameter - Add SA_RESTART for user signal registration, SA_NODEFER only when chaining - Save/restore errno in faulthandler_user_signal - Add signal re-entrancy guard in trigger_signals to prevent recursive handler invocation - Add thread frame tracking (push/pop/cleanup/reinit) with fo 4D24 rce_unlock fallback for post-fork recovery - Remove expectedFailure markers for now-passing tests
141cb11 to
cdadde5
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_faulthandler.py (TODO: 5) dependencies: dependent tests: (1 tests)
[x] lib: cpython/Lib/inspect.py dependencies:
dependent tests: (44 tests)
[ ] test: cpython/Lib/test/test_list.py (TODO: 5) dependencies: dependent tests: (no tests depend on list) [x] test: cpython/Lib/test/test_setcomps.py dependencies: dependent tests: (no tests depend on setcomps) [ ] lib: cpython/Lib/traceback.py dependencies:
dependent tests: (31 tests)
Legend:
|
Summary by CodeRabbit
Bug Fixes
New Features