From 572b253a932c9ac34156ce76a4a2c52386bb9d46 Mon Sep 17 00:00:00 2001 From: ShaharNaveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Thu, 11 Jun 2026 12:45:49 +0300 Subject: [PATCH 1/4] Initital cleanup --- crates/vm/src/signal.rs | 24 +++--- crates/vm/src/stdlib/_signal.rs | 144 +++++++++++++------------------- 2 files changed, 68 insertions(+), 100 deletions(-) diff --git a/crates/vm/src/signal.rs b/crates/vm/src/signal.rs index 6c8e9cb6d35..5356de88df6 100644 --- a/crates/vm/src/signal.rs +++ b/crates/vm/src/signal.rs @@ -1,29 +1,25 @@ -#![cfg_attr(target_os = "wasi", allow(dead_code))] use crate::{PyObjectRef, PyResult, VirtualMachine}; use alloc::fmt; use core::cell::{Cell, RefCell}; -#[cfg(windows)] -use core::sync::atomic::AtomicIsize; use core::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc; -pub(crate) const NSIG: usize = 64; +#[cfg(windows)] +use core::sync::atomic::AtomicIsize; -pub(crate) fn new_signal_handlers() -> Box; NSIG]>> { - Box::new(const { RefCell::new([const { None }; NSIG]) }) -} static ANY_TRIGGERED: AtomicBool = AtomicBool::new(false); -// hack to get around const array repeat expressions, rust issue #79270 -#[allow( - clippy::declare_interior_mutable_const, - reason = "workaround for const array repeat limitation (rust issue #79270)" -)] + +pub(crate) const NSIG: usize = 64; const ATOMIC_FALSE: AtomicBool = AtomicBool::new(false); pub(crate) static TRIGGERS: [AtomicBool; NSIG] = [ATOMIC_FALSE; NSIG]; #[cfg(windows)] static SIGINT_EVENT: AtomicIsize = AtomicIsize::new(0); +pub(crate) fn new_signal_handlers() -> Box; NSIG]>> { + Box::new(const { RefCell::new([const { None }; NSIG]) }) +} + thread_local! { /// Prevent recursive signal handler invocation. When a Python signal /// handler is running, new signals are deferred until it completes. @@ -50,6 +46,7 @@ pub fn check_signals(vm: &VirtualMachine) -> PyResult<()> { if !ANY_TRIGGERED.load(Ordering::Relaxed) { return Ok(()); } + // Atomic RMW only when a signal is actually pending. if !ANY_TRIGGERED.swap(false, Ordering::Acquire) { return Ok(()); @@ -99,8 +96,7 @@ pub(crate) fn is_triggered() -> bool { /// Reset all signal trigger state after fork in child process. /// Stale triggers from the parent must not fire in the child. -#[cfg(unix)] -#[cfg(feature = "host_env")] +#[cfg(all(unix, feature = "host_env"))] pub(crate) fn clear_after_fork() { ANY_TRIGGERED.store(false, Ordering::Release); for trigger in &TRIGGERS { diff --git a/crates/vm/src/stdlib/_signal.rs b/crates/vm/src/stdlib/_signal.rs index b9c41d6a4ce..676636ce3ab 100644 --- a/crates/vm/src/stdlib/_signal.rs +++ b/crates/vm/src/stdlib/_signal.rs @@ -11,7 +11,7 @@ pub(crate) mod _signal { use crate::{Py, PyObjectRef, PyResult, VirtualMachine, signal}; #[cfg(unix)] use crate::{ - builtins::PyTypeRef, + builtins::{PyBaseExceptionRef, PyTypeRef}, function::{ArgIntoFloat, OptionalArg}, }; use core::sync::atomic::{self, Ordering}; @@ -100,6 +100,7 @@ pub(crate) mod _signal { #[cfg(windows)] #[pyattr] const CTRL_C_EVENT: u32 = host_signal::CTRL_C_EVENT; + #[cfg(windows)] #[pyattr] const CTRL_BREAK_EVENT: u32 = host_signal::CTRL_BREAK_EVENT; @@ -130,9 +131,11 @@ pub(crate) mod _signal { #[cfg(target_os = "android")] #[pyattr] const ITIMER_REAL: libc::c_int = 0; + #[cfg(target_os = "android")] #[pyattr] const ITIMER_VIRTUAL: libc::c_int = 1; + #[cfg(target_os = "android")] #[pyattr] const ITIMER_PROF: libc::c_int = 2; @@ -147,6 +150,10 @@ pub(crate) mod _signal { ) } + fn new_itimer_error(msg: &str, vm: &VirtualMachine) -> PyBaseExceptionRef { + vm.new_exception_msg(itimer_error(vm), msg.into()) + } + #[cfg(any(unix, windows))] pub(super) fn init_signal_handlers( module: &Py, @@ -197,12 +204,12 @@ pub(crate) mod _signal { vm: &VirtualMachine, ) -> PyResult> { signal::assert_in_range(signalnum, vm)?; + #[cfg(windows)] - { - if !host_signal::is_valid_signal(signalnum) { - return Err(vm.new_value_error(format!("signal number {signalnum} out of range"))); - } + if !host_signal::is_valid_signal(signalnum) { + return Err(vm.new_value_error(format!("signal number {signalnum} out of range"))); } + if !vm.is_main_thread() { return Err(vm.new_value_error("signal only works in main thread")); } @@ -216,15 +223,11 @@ pub(crate) mod _signal { "signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable object", )), }; + signal::check_signals(vm)?; - let old = unsafe { host_signal::install_handler(signalnum, sig_handler) }; - let _old = match old { - Ok(old) => old, - Err(_) => { - return Err(vm.new_os_error("Failed to set signal".to_owned())); - } - }; + unsafe { host_signal::install_handler(signalnum, sig_handler) } + .map_err(|_| vm.new_os_error("Failed to set signal"))?; let signal_handlers = vm.signal_handlers.get_or_init(signal::new_signal_handlers); let old_handler = signal_handlers.borrow_mut()[signalnum as usize].replace(handler); @@ -269,25 +272,17 @@ pub(crate) mod _signal { it_value: double_to_timeval(seconds), it_interval: double_to_timeval(interval), }; - match host_signal::setitimer(which, &new) { - Ok(old) => Ok(itimerval_to_tuple(&old)), - Err(err) => { - let itimer_error = itimer_error(vm); - Err(vm.new_exception_msg(itimer_error, err.to_string().into())) - } - } + host_signal::setitimer(which, &new) + .map(|old| itimerval_to_tuple(&old)) + .map_err(|err| new_itimer_error(&err.to_string(), vm)) } #[cfg(unix)] #[pyfunction] fn getitimer(which: i32, vm: &VirtualMachine) -> PyResult<(f64, f64)> { - match host_signal::getitimer(which) { - Ok(old) => Ok(itimerval_to_tuple(&old)), - Err(err) => { - let itimer_error = itimer_error(vm); - Err(vm.new_exception_msg(itimer_error, err.to_string().into())) - } - } + host_signal::getitimer(which) + .map(|old| itimerval_to_tuple(&old)) + .map_err(|err| new_itimer_error(&err.to_string(), vm)) } #[pyfunction] @@ -310,10 +305,10 @@ pub(crate) mod _signal { fn set_wakeup_fd(args: SetWakeupFdArgs, vm: &VirtualMachine) -> PyResult { // TODO: implement warn_on_full_buffer let _ = args.warn_on_full_buffer; - #[cfg(windows)] - let fd = args.fd.0; - #[cfg(not(windows))] - let fd = args.fd; + let fd = cfg_select! { + windows => args.fd.0, + _ => args.fd, + }; if !vm.is_main_thread() { return Err(vm.new_value_error("set_wakeup_fd only works in main thread")); @@ -343,21 +338,16 @@ pub(crate) mod _signal { } let old_fd = WAKEUP.swap(fd, Ordering::Relaxed); + #[cfg(windows)] WAKEUP_IS_SOCKET.store(is_socket, Ordering::Relaxed); #[cfg(windows)] - { - if old_fd == INVALID_WAKEUP { - Ok(-1) - } else { - Ok(old_fd as i64) - } - } - #[cfg(not(windows))] - { - Ok(old_fd as i64) + if old_fd == INVALID_WAKEUP { + return Ok(-1); } + + Ok(old_fd as i64) } #[cfg(target_os = "linux")] @@ -387,7 +377,6 @@ pub(crate) mod _signal { host_signal::siginterrupt(signum, flag).map_err(|_| vm.new_last_errno_error()) } - /// CPython: signal_raise_signal (signalmodule.c) #[cfg(any(unix, windows))] #[pyfunction] fn raise_signal(signalnum: i32, vm: &VirtualMachine) -> PyResult<()> { @@ -395,17 +384,14 @@ pub(crate) mod _signal { // On Windows, only certain signals are supported #[cfg(windows)] - { - if !host_signal::is_valid_signal(signalnum) { - return Err(vm - .new_errno_error(libc::EINVAL, "Invalid argument") - .upcast()); - } + if !host_signal::is_valid_signal(signalnum) { + return Err(vm + .new_errno_error(libc::EINVAL, "Invalid argument") + .upcast()); } - if host_signal::raise_signal(signalnum).is_err() { - return Err(vm.new_os_error(format!("raise_signal failed for signal {signalnum}"))); - } + host_signal::raise_signal(signalnum) + .map_err(|_| vm.new_os_error(format!("raise_signal failed for signal {signalnum}")))?; // Check if a signal was triggered and handle it signal::check_signals(vm)?; @@ -413,17 +399,7 @@ pub(crate) mod _signal { Ok(()) } - /// CPython: signal_strsignal (signalmodule.c) - #[cfg(unix)] - #[pyfunction] - fn strsignal(signalnum: i32, vm: &VirtualMachine) -> PyResult> { - if signalnum < 1 || signalnum >= signal::NSIG as i32 { - return Err(vm.new_value_error(format!("signal number {signalnum} out of range"))); - } - Ok(host_signal::strsignal(signalnum)) - } - - #[cfg(windows)] + #[cfg(any(unix, windows))] #[pyfunction] fn strsignal(signalnum: i32, vm: &VirtualMachine) -> PyResult> { if signalnum < 1 || signalnum >= signal::NSIG as i32 { @@ -432,23 +408,20 @@ pub(crate) mod _signal { Ok(host_signal::strsignal(signalnum)) } - /// CPython: signal_valid_signals (signalmodule.c) #[pyfunction] fn valid_signals(vm: &VirtualMachine) -> PyResult { use crate::PyPayload; use crate::builtins::PySet; let set = PySet::default().into_ref(&vm.ctx); + + // Empty set for platforms without signal support (e.g., WASM) #[cfg(any(unix, windows))] for signum in host_signal::valid_signals(signal::NSIG) - .map_err(|_| vm.new_os_error("sigfillset failed".to_owned()))? + .map_err(|_| vm.new_os_error("sigfillset failed"))? { set.add(vm.ctx.new_int(signum).into(), vm)?; } - #[cfg(not(any(unix, windows)))] - { - // Empty set for platforms without signal support (e.g., WASM) - let _ = &set; - } + Ok(set.into()) } @@ -480,21 +453,20 @@ pub(crate) mod _signal { // Add signals to the set for sig in mask.iter(vm)? { let sig = sig?; - // Convert to i32, handling overflow by returning ValueError - let signum: i32 = sig.try_to_value(vm).map_err(|_| { - vm.new_value_error(format!( - "signal number out of range [1, {}]", - signal::NSIG - 1 - )) - })?; - // Validate signal number is in range [1, NSIG) - if signum < 1 || signum >= signal::NSIG as i32 { - return Err(vm.new_value_error(format!( - "signal number {} out of range [1, {}]", - signum, - signal::NSIG - 1 - ))); - } + // Convert to i32 + // - handling overflow by returning ValueError + // - validate signal number is in range [1, NSIG) + let signum = sig + .try_to_value::(vm) + .ok() + .filter(|v| (1..signal::NSIG as i32).contains(v)) + .ok_or_else(|| { + vm.new_value_error(format!( + "signal number out of range [1, {}]", + signal::NSIG - 1 + )) + })?; + host_signal::sigaddset(&mut sigset, signum).map_err(|e| e.into_pyexception(vm))?; } @@ -512,15 +484,15 @@ pub(crate) mod _signal { pub extern "C" fn run_signal(signum: i32) { signal::TRIGGERS[signum as usize].store(true, Ordering::Relaxed); signal::set_triggered(); - #[cfg(windows)] + host_signal::notify_signal( signum, WAKEUP.load(Ordering::Relaxed), + #[cfg(windows)] WAKEUP_IS_SOCKET.load(Ordering::Relaxed), + #[cfg(windows)] signal::get_sigint_event(), ); - #[cfg(unix)] - host_signal::notify_signal(signum, WAKEUP.load(Ordering::Relaxed)); } /// Reset wakeup fd after fork in child process. From 62f3560b1f38c7c38dbe0b5d407bd17f24b4e1ca Mon Sep 17 00:00:00 2001 From: ShaharNaveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Thu, 11 Jun 2026 13:12:10 +0300 Subject: [PATCH 2/4] More cleanup --- crates/vm/src/stdlib/_signal.rs | 59 ++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/crates/vm/src/stdlib/_signal.rs b/crates/vm/src/stdlib/_signal.rs index 676636ce3ab..75dac8fb2f2 100644 --- a/crates/vm/src/stdlib/_signal.rs +++ b/crates/vm/src/stdlib/_signal.rs @@ -6,21 +6,32 @@ pub(crate) use _signal::module_def; pub(crate) mod _signal { #![allow(unreachable_pub)] - #[cfg(any(unix, windows))] - use crate::convert::{IntoPyException, TryFromBorrowedObject}; use crate::{Py, PyObjectRef, PyResult, VirtualMachine, signal}; - #[cfg(unix)] - use crate::{ - builtins::{PyBaseExceptionRef, PyTypeRef}, - function::{ArgIntoFloat, OptionalArg}, + use core::{ + ops::Range, + sync::atomic::{self, Ordering}, }; - use core::sync::atomic::{self, Ordering}; - #[cfg(any(unix, windows))] - use rustpython_host_env::signal as host_signal; - #[cfg(unix)] - use rustpython_host_env::signal::{double_to_timeval, itimerval_to_tuple}; - #[cfg(unix)] - use std::os::fd::AsFd; + + cfg_select! { + any(unix, windows) => { + use crate::convert::{IntoPyException, TryFromBorrowedObject}; + use rustpython_host_env::signal as host_signal; + } + _ => {} + } + + cfg_select! { + unix => { + use crate::{ + builtins::{PyBaseExceptionRef, PyTypeRef}, + function::{ArgIntoFloat, OptionalArg}, + }; + use rustpython_host_env::signal::{double_to_timeval, itimerval_to_tuple}; + + use std::os::fd::AsFd; + }, + _ => {} + } #[allow(non_camel_case_types)] type sighandler_t = cfg_select! { @@ -28,6 +39,8 @@ pub(crate) mod _signal { _ => usize, }; + const SIGNUM_RANGE: Range = 1..signal::NSIG as i32; + cfg_select! { windows => { type WakeupFdRaw = libc::SOCKET; @@ -150,10 +163,14 @@ pub(crate) mod _signal { ) } + #[cfg(unix)] fn new_itimer_error(msg: &str, vm: &VirtualMachine) -> PyBaseExceptionRef { vm.new_exception_msg(itimer_error(vm), msg.into()) } + const _: () = assert!(SIGNUM_RANGE.start.is_positive()); + const _: () = assert!(SIGNUM_RANGE.end.is_positive()); + #[cfg(any(unix, windows))] pub(super) fn init_signal_handlers( module: &Py, @@ -163,8 +180,8 @@ pub(crate) mod _signal { let sig_dfl = vm.new_pyobj(SIG_DFL as u8); let sig_ign = vm.new_pyobj(SIG_IGN as u8); - for signum in 1..NSIG { - let Some(handler) = (unsafe { host_signal::probe_handler(signum as i32) }) else { + for signum in SIGNUM_RANGE { + let Some(handler) = (unsafe { host_signal::probe_handler(signum) }) else { continue; }; let py_handler = if handler == SIG_DFL { @@ -174,9 +191,10 @@ pub(crate) mod _signal { } else { None }; + vm.signal_handlers .get_or_init(signal::new_signal_handlers) - .borrow_mut()[signum] = py_handler; + .borrow_mut()[signum as usize] = py_handler; } let int_handler = module @@ -402,9 +420,10 @@ pub(crate) mod _signal { #[cfg(any(unix, windows))] #[pyfunction] fn strsignal(signalnum: i32, vm: &VirtualMachine) -> PyResult> { - if signalnum < 1 || signalnum >= signal::NSIG as i32 { + if !SIGNUM_RANGE.contains(&signalnum) { return Err(vm.new_value_error(format!("signal number {signalnum} out of range"))); } + Ok(host_signal::strsignal(signalnum)) } @@ -430,7 +449,7 @@ pub(crate) mod _signal { use crate::PyPayload; use crate::builtins::PySet; let set = PySet::default().into_ref(&vm.ctx); - for signum in 1..signal::NSIG { + for signum in SIGNUM_RANGE { if host_signal::sigset_contains(mask, signum as i32) { set.add(vm.ctx.new_int(signum as i32).into(), vm)?; } @@ -459,11 +478,11 @@ pub(crate) mod _signal { let signum = sig .try_to_value::(vm) .ok() - .filter(|v| (1..signal::NSIG as i32).contains(v)) + .filter(|v| SIGNUM_RANGE.contains(v)) .ok_or_else(|| { vm.new_value_error(format!( "signal number out of range [1, {}]", - signal::NSIG - 1 + SIGNUM_RANGE.end )) })?; From 18ba4842e9cc28daecf4422cc77d087da5d64aae Mon Sep 17 00:00:00 2001 From: ShaharNaveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Thu, 11 Jun 2026 14:08:01 +0300 Subject: [PATCH 3/4] coderabbit suggestion --- crates/vm/src/stdlib/_signal.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/vm/src/stdlib/_signal.rs b/crates/vm/src/stdlib/_signal.rs index 75dac8fb2f2..f46c6ea8cc1 100644 --- a/crates/vm/src/stdlib/_signal.rs +++ b/crates/vm/src/stdlib/_signal.rs @@ -482,7 +482,7 @@ pub(crate) mod _signal { .ok_or_else(|| { vm.new_value_error(format!( "signal number out of range [1, {}]", - SIGNUM_RANGE.end + SIGNUM_RANGE.end - 1 )) })?; From aaba3fa93c518f89267d46187b2ad0e23600f6ae Mon Sep 17 00:00:00 2001 From: ShaharNaveh <50263213+ShaharNaveh@users.noreply.github.com> Date: Thu, 11 Jun 2026 14:28:47 +0300 Subject: [PATCH 4/4] clippy --- crates/vm/src/signal.rs | 6 ++++++ crates/vm/src/stdlib/_signal.rs | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/vm/src/signal.rs b/crates/vm/src/signal.rs index 5356de88df6..50a887c1435 100644 --- a/crates/vm/src/signal.rs +++ b/crates/vm/src/signal.rs @@ -10,7 +10,13 @@ use core::sync::atomic::AtomicIsize; static ANY_TRIGGERED: AtomicBool = AtomicBool::new(false); pub(crate) const NSIG: usize = 64; + +#[expect( + clippy::declare_interior_mutable_const, + reason = "workaround for const array repeat limitation (rust issue #79270)" +)] const ATOMIC_FALSE: AtomicBool = AtomicBool::new(false); + pub(crate) static TRIGGERS: [AtomicBool; NSIG] = [ATOMIC_FALSE; NSIG]; #[cfg(windows)] diff --git a/crates/vm/src/stdlib/_signal.rs b/crates/vm/src/stdlib/_signal.rs index f46c6ea8cc1..191f67d090f 100644 --- a/crates/vm/src/stdlib/_signal.rs +++ b/crates/vm/src/stdlib/_signal.rs @@ -450,8 +450,8 @@ pub(crate) mod _signal { use crate::builtins::PySet; let set = PySet::default().into_ref(&vm.ctx); for signum in SIGNUM_RANGE { - if host_signal::sigset_contains(mask, signum as i32) { - set.add(vm.ctx.new_int(signum as i32).into(), vm)?; + if host_signal::sigset_contains(mask, signum) { + set.add(vm.ctx.new_int(signum).into(), vm)?; } } Ok(set.into())