Skip to content
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.

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

Update catch_unwind doc comments for c_unwind #128321

Merged
merged 1 commit into from
Sep 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 10 additions & 5 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2666,12 +2666,17 @@ extern "rust-intrinsic" {
///
/// `catch_fn` must not unwind.
///
/// The third argument is a function called if an unwind occurs (both Rust unwinds and foreign
/// unwinds). This function takes the data pointer and a pointer to the target-specific
/// exception object that was caught. For more information, see the compiler's source as well as
/// std's `catch_unwind` implementation.
/// The third argument is a function called if an unwind occurs (both Rust `panic` and foreign
/// unwinds). This function takes the data pointer and a pointer to the target- and
/// runtime-specific exception object that was caught.
///
/// The stable version of this intrinsic is `std::panic::catch_unwind`.
/// Note that in the case of a foreign unwinding operation, the exception object data may not be
/// safely usable from Rust, and should not be directly exposed via the standard library. To
/// prevent unsafe access, the library implementation may either abort the process or present an
BatmanAoD marked this conversation as resolved.
Show resolved Hide resolved
/// opaque error type to the user.
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like it is the library implementation that chooses which of the two happen, which I don't think is the case?

It would be good to explain somewhere (not as stable docs, but as information for people that want to understand the system) what decides which of these two outcomes occurs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that the library did indeed make that decision from my first look at the code, but on looking again I'm not so sure.

The files where the detection, and handling, of "foreign" exceptions occur seems to be in rust/library/panic_unwind/src/<target>.rs.

@workingjubilee or @chorman0773 , can either of you confirm that this understanding is correct, and that the standard library, rather than the compiler intrinsic, determines whether catch_unwind aborts in the face of a foreign exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't speak to how rustc in particular handles things, I can only speak to abi-level handling and guarantees.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nbdd0121 Are you the right person to ask here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the library decides

Copy link
Member

@workingjubilee workingjubilee Aug 26, 2024

Choose a reason for hiding this comment

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

Hi, sorry. Yes, that's correct, as mentioned we call __rust_foreign_exception, which shows up in these places:

panic_unwind/src/lib.rs
84:    fn __rust_foreign_exception() -> !;

panic_unwind/src/seh.rs
366:        super::__rust_foreign_exception();
372:        super::__rust_foreign_exception();

panic_unwind/src/gcc.rs
89:        super::__rust_foreign_exception();
101:        super::__rust_foreign_exception();

panic_unwind/src/emcc.rs
78:        super::__rust_foreign_exception();
83:        super::__rust_foreign_exception();

std/src/panicking.rs
64:extern "C" fn __rust_foreign_exception() -> ! {

Copy link
Member

Choose a reason for hiding this comment

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

Our unwinding code is unfortunately not very localized, and is splashed all over the library folder (workspace, now?). Some of the relevant code is inside library/std and some of it is in library/panic_unwind and some of it is in library/unwind and just... ugh.

I think half the reason this situation has not been cleaned up is because it's hard to just find it all.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like we currently always abort (seeing as __rust_foreign_exception has return type !)? But the point of the docs here is that we may change that in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we currently always abort, which is why no implementation change accompanies these doc changes. The point of the doc change here is to constrain what we may do in the future, because it is currently officially UB.

///
/// For more information, see the compiler's source, as well as the documentation for the stable
/// version of this intrinsic, `std::panic::catch_unwind`.
#[rustc_nounwind]
pub fn catch_unwind(try_fn: fn(*mut u8), data: *mut u8, catch_fn: fn(*mut u8, *mut u8)) -> i32;

Expand Down
58 changes: 34 additions & 24 deletions library/std/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,45 +285,55 @@ where

/// Invokes a closure, capturing the cause of an unwinding panic if one occurs.
///
/// This function will return `Ok` with the closure's result if the closure
/// does not panic, and will return `Err(cause)` if the closure panics. The
/// `cause` returned is the object with which panic was originally invoked.
/// This function will return `Ok` with the closure's result if the closure does
/// not panic, and will return `Err(cause)` if the closure panics. The `cause`
/// returned is the object with which panic was originally invoked.
///
/// It is currently undefined behavior to unwind from Rust code into foreign
/// code, so this function is particularly useful when Rust is called from
/// another language (normally C). This can run arbitrary Rust code, capturing a
/// panic and allowing a graceful handling of the error.
/// Rust functions that are expected to be called from foreign code that does
/// not support unwinding (such as C compiled with `-fno-exceptions`) should be
/// defined using `extern "C"`, which ensures that if the Rust code panics, it
/// is automatically caught and the process is aborted. If this is the desired
/// behavior, it is not necessary to use `catch_unwind` explicitly. This
/// function should instead be used when more graceful error-handling is needed.
///
/// It is **not** recommended to use this function for a general try/catch
/// mechanism. The [`Result`] type is more appropriate to use for functions that
/// can fail on a regular basis. Additionally, this function is not guaranteed
/// to catch all panics, see the "Notes" section below.
///
/// The closure provided is required to adhere to the [`UnwindSafe`] trait to ensure
/// that all captured variables are safe to cross this boundary. The purpose of
/// this bound is to encode the concept of [exception safety][rfc] in the type
/// system. Most usage of this function should not need to worry about this
/// bound as programs are naturally unwind safe without `unsafe` code. If it
/// becomes a problem the [`AssertUnwindSafe`] wrapper struct can be used to quickly
/// assert that the usage here is indeed unwind safe.
/// The closure provided is required to adhere to the [`UnwindSafe`] trait to
/// ensure that all captured variables are safe to cross this boundary. The
/// purpose of this bound is to encode the concept of [exception safety][rfc] in
/// the type system. Most usage of this function should not need to worry about
/// this bound as programs are naturally unwind safe without `unsafe` code. If
/// it becomes a problem the [`AssertUnwindSafe`] wrapper struct can be used to
/// quickly assert that the usage here is indeed unwind safe.
///
/// [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1236-stabilize-catch-panic.md
///
/// # Notes
///
/// Note that this function **might not catch all panics** in Rust. A panic in
/// Rust is not always implemented via unwinding, but can be implemented by
/// aborting the process as well. This function *only* catches unwinding panics,
/// not those that abort the process.
/// This function **might not catch all Rust panics**. A Rust panic is not
/// always implemented via unwinding, but can be implemented by aborting the
/// process as well. This function *only* catches unwinding panics, not those
/// that abort the process.
///
/// Note that if a custom panic hook has been set, it will be invoked before
/// the panic is caught, before unwinding.
/// If a custom panic hook has been set, it will be invoked before the panic is
/// caught, before unwinding.
///
/// Also note that unwinding into Rust code with a foreign exception (e.g.
/// an exception thrown from C++ code) is undefined behavior.
/// Although unwinding into Rust code with a foreign exception (e.g. an
/// exception thrown from C++ code, or a `panic!` in Rust code compiled or
/// linked with a different runtime) via an appropriate ABI (e.g. `"C-unwind"`)
/// is permitted, catching such an exception using this function will have one
/// of two behaviors, and it is unspecified which will occur:
///
/// Finally, be **careful in how you drop the result of this function**.
/// If it is `Err`, it contains the panic payload, and dropping that may in turn panic!
/// * The process aborts, after executing all destructors of `f` and the
/// functions it called.
/// * The function returns a `Result::Err` containing an opaque type.
BatmanAoD marked this conversation as resolved.
Show resolved Hide resolved
///
/// Finally, be **careful in how you drop the result of this function**. If it
/// is `Err`, it contains the panic payload, and dropping that may in turn
/// panic!
///
/// # Examples
///
Expand Down
27 changes: 26 additions & 1 deletion library/std/src/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,19 @@ impl Builder {
/// println!("{result}");
/// ```
///
/// # Notes
///
/// This function has the same minimal guarantee regarding "foreign" unwinding operations (e.g.
/// an exception thrown from C++ code, or a `panic!` in Rust code compiled or linked with a
/// different runtime) as [`catch_unwind`]; namely, if the thread created with `thread::spawn`
/// unwinds all the way to the root with such an exception, one of two behaviors are possible,
/// and it is unspecified which will occur:
///
/// * The process aborts.
/// * The process does not abort, and [`join`] will return a `Result::Err`
/// containing an opaque type.
///
/// [`catch_unwind`]: ../../std/panic/fn.catch_unwind.html
/// [`channels`]: crate::sync::mpsc
/// [`join`]: JoinHandle::join
/// [`Err`]: crate::result::Result::Err
Expand Down Expand Up @@ -1737,7 +1750,7 @@ impl<T> JoinHandle<T> {
/// operations that happen after `join` returns.
///
/// If the associated thread panics, [`Err`] is returned with the parameter given
/// to [`panic!`].
/// to [`panic!`] (though see the Notes below).
///
/// [`Err`]: crate::result::Result::Err
/// [atomic memory orderings]: crate::sync::atomic
Expand All @@ -1759,6 +1772,18 @@ impl<T> JoinHandle<T> {
/// }).unwrap();
/// join_handle.join().expect("Couldn't join on the associated thread");
/// ```
///
/// # Notes
///
/// If a "foreign" unwinding operation (e.g. an exception thrown from C++
/// code, or a `panic!` in Rust code compiled or linked with a different
/// runtime) unwinds all the way to the thread root, the process may be
/// aborted; see the Notes on [`thread::spawn`]. If the process is not
/// aborted, this function will return a `Result::Err` containing an opaque
/// type.
///
/// [`catch_unwind`]: ../../std/panic/fn.catch_unwind.html
/// [`thread::spawn`]: spawn
#[stable(feature = "rust1", since = "1.0.0")]
pub fn join(self) -> Result<T> {
self.0.join()
Expand Down
Loading