Skip to content

add #[cold] paths for unlikely error-fetching branches #5657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
davidhewitt merged 2 commits into from
Nov 29, 2025
Merged

add #[cold] paths for unlikely error-fetching branches #5657

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc18098
add `#[cold]` paths for unlikely error-fetching branches
davidhewitt Nov 28, 2025
6a4ea61
use `if cfg!`
davidhewitt Nov 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 32 additions & 21 deletions src/err/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use crate::{BoundObject, Py, PyAny, Python};
use err_state::{PyErrState, PyErrStateLazyFnOutput, PyErrStateNormalized};
use std::convert::Infallible;
use std::ffi::CStr;
use std::ptr;

mod cast_error;
mod downcast_error;
Expand Down Expand Up @@ -280,27 +279,29 @@ impl PyErr {
/// from a C FFI function, use [`PyErr::fetch`].
pub fn take(py: Python<'_>) -> Option<PyErr> {
let state = PyErrStateNormalized::take(py)?;
let pvalue = state.pvalue.bind(py);
if ptr::eq(
pvalue.get_type().as_ptr(),
PanicException::type_object_raw(py).cast(),
) {
let msg: String = pvalue
.str()
.map(|py_str| py_str.to_string_lossy().into())
.unwrap_or_else(|_| String::from("Unwrapped panic from Python code"));
Self::print_panic_and_unwind(py, PyErrState::normalized(state), msg)

if PanicException::is_exact_type_of(state.pvalue.bind(py)) {
Self::print_panic_and_unwind(py, state)
}

Some(PyErr::from_state(PyErrState::normalized(state)))
}

fn print_panic_and_unwind(py: Python<'_>, state: PyErrState, msg: String) -> ! {
#[cold]
fn print_panic_and_unwind(py: Python<'_>, state: PyErrStateNormalized) -> ! {
let msg: String = state
.pvalue
.bind(py)
.str()
.map(|py_str| py_str.to_string_lossy().into())
.unwrap_or_else(|_| String::from("Unwrapped panic from Python code"));

eprintln!("--- PyO3 is resuming a panic after fetching a PanicException from Python. ---");
eprintln!("Python stack trace below:");

state.restore(py);
PyErrState::normalized(state).restore(py);

// SAFETY: thread is attached and error was just set in the interpreter
unsafe {
ffi::PyErr_PrintEx(0);
}
Expand All @@ -322,14 +323,7 @@ impl PyErr {
#[cfg_attr(debug_assertions, track_caller)]
#[inline]
pub fn fetch(py: Python<'_>) -> PyErr {
const FAILED_TO_FETCH: &str = "attempted to fetch exception but none was set";
match PyErr::take(py) {
Some(err) => err,
#[cfg(debug_assertions)]
None => panic!("{}", FAILED_TO_FETCH),
#[cfg(not(debug_assertions))]
None => crate::exceptions::PySystemError::new_err(FAILED_TO_FETCH),
}
PyErr::take(py).unwrap_or_else(failed_to_fetch)
}

/// Creates a new exception type with the given name and docstring.
Expand Down Expand Up @@ -628,6 +622,23 @@ impl PyErr {
}
}

/// Called when `PyErr::fetch` is called but no exception is set.
#[cold]
#[cfg_attr(debug_assertions, track_caller)]
fn failed_to_fetch() -> PyErr {
const FAILED_TO_FETCH: &str = "attempted to fetch exception but none was set";

#[cfg(debug_assertions)]
Copy link
Contributor

@Tpt Tpt Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hyper-nit: might be slightly more readable with a if cfg!:

if cfg!(debug_assertions) {
    panic!("{}", FAILED_TO_FETCH)
} else {
    crate::exceptions::PySystemError::new_err(FAILED_TO_FETCH)
}

Copy link
Member Author

@davidhewitt davidhewitt Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, good idea 👍

{
panic!("{}", FAILED_TO_FETCH)
}

#[cfg(not(debug_assertions))]
{
crate::exceptions::PySystemError::new_err(FAILED_TO_FETCH)
}
}

impl std::fmt::Debug for PyErr {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
Python::attach(|py| {
Expand Down
Loading