From ffd3e4cf9570777acc66706b69599b952fd76940 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Fri, 24 Feb 2023 23:06:12 +0100 Subject: [PATCH 01/11] Add `CxxException` to wrap a C++ exception The exception object can be passed as a pointer via `CxxException` error through Rust frames. A method to create the default message-based `rust::Error` and to drop and clone the exception are provided. --- src/cxx.cc | 47 ++++++++++++++++++++++++++++++++++++++++++ src/result.rs | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/src/cxx.cc b/src/cxx.cc index 4958eb08b..a44605d82 100644 --- a/src/cxx.cc +++ b/src/cxx.cc @@ -454,10 +454,57 @@ static const char *errorCopy(const char *ptr, std::size_t len) { return copy; } +namespace { +template <> +class impl final { +public: + static Error error_copy(const char *ptr, std::size_t len) noexcept { + Error error; + error.msg = errorCopy(ptr, len); + error.len = len; + return error; + } +}; +} // namespace + +// Ensure statically that `std::exception_ptr` is really a pointer. +static_assert(sizeof(std::exception_ptr) == sizeof(void *), + "Unsupported std::exception_ptr size"); +static_assert(alignof(std::exception_ptr) == alignof(void *), + "Unsupported std::exception_ptr alignment"); + extern "C" { const char *cxxbridge1$error(const char *ptr, std::size_t len) noexcept { return errorCopy(ptr, len); } + +void *cxxbridge1$default_exception(const char *ptr, std::size_t len) noexcept { + // Construct an `std::exception_ptr` for the default `rust::Error` in the + // space provided by the pointer itself (placement new). + // + // The `std::exception_ptr` itself is just a pointer, so this effectively + // converts it to the pointer. + void *eptr; + new (&eptr) std::exception_ptr( + std::make_exception_ptr(impl::error_copy(ptr, len))); + return eptr; +} + +void cxxbridge1$drop_exception(char *ptr) noexcept { + // Implement the `drop` for `CxxException` on the Rust side, which is just a + // pointer to the exception stored in `std::exception_ptr`. + auto eptr = std::move(*reinterpret_cast(&ptr)); + // eptr goes out of scope and deallocates `std::exception_ptr`. +} + +void *cxxbridge1$clone_exception(const char *ptr) noexcept { + // Implement the `clone` for `CxxException` on the Rust side, which is just a + // pointer to the exception stored in `std::exception_ptr`. + void *eptr; + new (&eptr) std::exception_ptr( + *reinterpret_cast(&ptr)); + return eptr; +} } // extern "C" Error::Error(const Error &other) diff --git a/src/result.rs b/src/result.rs index ba77858e3..4ea14d6c7 100644 --- a/src/result.rs +++ b/src/result.rs @@ -17,6 +17,63 @@ pub struct PtrLen { pub len: usize, } +extern "C" { + /// Helper to construct the default exception from the error message. + #[link_name = "cxxbridge1$default_exception"] + fn default_exception(ptr: *const u8, len: usize) -> *mut u8; + /// Helper to clone the instance of `std::exception_ptr` on the C++ side. + #[link_name = "cxxbridge1$clone_exception"] + fn clone_exception(ptr: *const u8) -> *mut u8; + /// Helper to drop the instance of `std::exception_ptr` on the C++ side. + #[link_name = "cxxbridge1$drop_exception"] + fn drop_exception(ptr: *mut u8); +} + +/// C++ exception containing `std::exception_ptr`. +/// +/// This object is the Rust wrapper over `std::exception_ptr`, so it owns the exception pointer. +/// I.e., the exception is either referenced by a `std::exception_ptr` on the C++ side or the +/// reference is moved to this object on the Rust side. +#[repr(C)] +#[must_use] +pub struct CxxException(NonNull); + +impl CxxException { + /// Construct the default `rust::Error` exception from the specified `exc_text`. + pub fn new_default(exc_text: &str) -> Self { + let exception_ptr = unsafe { + default_exception(exc_text.as_ptr(), exc_text.len()) + }; + CxxException( + NonNull::new(exception_ptr) + .expect("Exception conversion returned a null pointer") + ) + } +} + +impl Clone for CxxException { + fn clone(&self) -> Self { + let clone_ptr = unsafe { clone_exception(self.0.as_ptr()) }; + Self( + NonNull::new(clone_ptr) + .expect("Exception cloning returned a null pointer") + ) + } +} + +impl Drop for CxxException { + fn drop(&mut self) { + unsafe { drop_exception(self.0.as_ptr()) }; + } +} + +// SAFETY: This is safe, since the C++ exception referenced by `std::exception_ptr` +// is not thread-local. +unsafe impl Send for CxxException {} +// SAFETY: This is safe, since the C++ exception referenced by `std::exception_ptr` +// can be shared across threads read-only. +unsafe impl Sync for CxxException {} + #[repr(C)] pub union Result { err: PtrLen, From 73eeef3168e057ab9d5d8bf59e4c83f47442016b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Fri, 24 Feb 2023 23:10:10 +0100 Subject: [PATCH 02/11] Add `CxxExceptionPtr` atom This represents C++ `std::exception_ptr` on the Rust side via `CxxException` object, which can be mentioned as a return type for functions creating custom exceptions or evaluating exceptions caught from C++ (added in further commits). --- gen/src/write.rs | 3 ++- macro/src/expand.rs | 4 ++++ syntax/atom.rs | 3 +++ syntax/check.rs | 8 ++++---- syntax/pod.rs | 2 +- syntax/tokens.rs | 2 +- 6 files changed, 15 insertions(+), 7 deletions(-) diff --git a/gen/src/write.rs b/gen/src/write.rs index 6f535ccb9..a18f416b9 100644 --- a/gen/src/write.rs +++ b/gen/src/write.rs @@ -211,7 +211,7 @@ fn pick_includes_and_builtins(out: &mut OutFile, apis: &[Api]) { Some(Isize) => out.builtin.rust_isize = true, Some(CxxString) => out.include.string = true, Some(RustString) => out.builtin.rust_string = true, - Some(Bool) | Some(Char) | Some(F32) | Some(F64) | None => {} + Some(Bool) | Some(Char) | Some(F32) | Some(F64) | Some(CxxExceptionPtr) | None => {} }, Type::RustBox(_) => out.builtin.rust_box = true, Type::RustVec(_) => out.builtin.rust_vec = true, @@ -1323,6 +1323,7 @@ fn write_atom(out: &mut OutFile, atom: Atom) { F64 => write!(out, "double"), CxxString => write!(out, "::std::string"), RustString => write!(out, "::rust::String"), + CxxExceptionPtr => write!(out, "::std::exception_ptr"), } } diff --git a/macro/src/expand.rs b/macro/src/expand.rs index ea5af66a4..f354d0d24 100644 --- a/macro/src/expand.rs +++ b/macro/src/expand.rs @@ -1751,6 +1751,10 @@ fn expand_extern_type(ty: &Type, types: &Types, proper: bool) -> TokenStream { let span = ident.rust.span(); quote_spanned!(span=> ::cxx::private::RustString) } + Type::Ident(ident) if ident.rust == CxxExceptionPtr => { + let span = ident.rust.span(); + quote_spanned!(span=> ::cxx::CxxException) + } Type::RustBox(ty) | Type::UniquePtr(ty) => { let span = ty.name.span(); if proper && types.is_considered_improper_ctype(&ty.inner) { diff --git a/syntax/atom.rs b/syntax/atom.rs index d4ad78f17..4d6b3778c 100644 --- a/syntax/atom.rs +++ b/syntax/atom.rs @@ -20,6 +20,7 @@ pub enum Atom { F64, CxxString, RustString, + CxxExceptionPtr, } impl Atom { @@ -46,6 +47,7 @@ impl Atom { "f64" => Some(F64), "CxxString" => Some(CxxString), "String" => Some(RustString), + "CxxException" => Some(CxxExceptionPtr), _ => None, } } @@ -77,6 +79,7 @@ impl AsRef for Atom { F64 => "f64", CxxString => "CxxString", RustString => "String", + CxxExceptionPtr => "CxxException", } } } diff --git a/syntax/check.rs b/syntax/check.rs index 66883be03..7d92571d2 100644 --- a/syntax/check.rs +++ b/syntax/check.rs @@ -126,7 +126,7 @@ fn check_type_rust_vec(cx: &mut Check, ty: &Ty1) { None | Some(Bool) | Some(Char) | Some(U8) | Some(U16) | Some(U32) | Some(U64) | Some(Usize) | Some(I8) | Some(I16) | Some(I32) | Some(I64) | Some(Isize) | Some(F32) | Some(F64) | Some(RustString) => return, - Some(CxxString) => {} + Some(CxxString) | Some(CxxExceptionPtr) => {} } } Type::Str(_) => return, @@ -165,7 +165,7 @@ fn check_type_shared_ptr(cx: &mut Check, ptr: &Ty1) { None | Some(Bool) | Some(U8) | Some(U16) | Some(U32) | Some(U64) | Some(Usize) | Some(I8) | Some(I16) | Some(I32) | Some(I64) | Some(Isize) | Some(F32) | Some(F64) | Some(CxxString) => return, - Some(Char) | Some(RustString) => {} + Some(Char) | Some(RustString) | Some(CxxExceptionPtr) => {} } } else if let Type::CxxVector(_) = &ptr.inner { cx.error(ptr, "std::shared_ptr is not supported yet"); @@ -186,7 +186,7 @@ fn check_type_weak_ptr(cx: &mut Check, ptr: &Ty1) { None | Some(Bool) | Some(U8) | Some(U16) | Some(U32) | Some(U64) | Some(Usize) | Some(I8) | Some(I16) | Some(I32) | Some(I64) | Some(Isize) | Some(F32) | Some(F64) | Some(CxxString) => return, - Some(Char) | Some(RustString) => {} + Some(Char) | Some(RustString) | Some(CxxExceptionPtr) => {} } } else if let Type::CxxVector(_) = &ptr.inner { cx.error(ptr, "std::weak_ptr is not supported yet"); @@ -211,7 +211,7 @@ fn check_type_cxx_vector(cx: &mut Check, ptr: &Ty1) { | Some(I16) | Some(I32) | Some(I64) | Some(Isize) | Some(F32) | Some(F64) | Some(CxxString) => return, Some(Char) => { /* todo */ } - Some(Bool) | Some(RustString) => {} + Some(Bool) | Some(RustString) | Some(CxxExceptionPtr) => {} } } diff --git a/syntax/pod.rs b/syntax/pod.rs index 0bf152eea..f857ce6a1 100644 --- a/syntax/pod.rs +++ b/syntax/pod.rs @@ -10,7 +10,7 @@ impl<'a> Types<'a> { match atom { Bool | Char | U8 | U16 | U32 | U64 | Usize | I8 | I16 | I32 | I64 | Isize | F32 | F64 => true, - CxxString | RustString => false, + CxxString | RustString | CxxExceptionPtr => false, } } else if let Some(strct) = self.structs.get(ident) { derive::contains(&strct.derives, Trait::Copy) diff --git a/syntax/tokens.rs b/syntax/tokens.rs index a9f42bd43..69fce2062 100644 --- a/syntax/tokens.rs +++ b/syntax/tokens.rs @@ -14,7 +14,7 @@ impl ToTokens for Type { if ident.rust == Char { let span = ident.rust.span(); tokens.extend(quote_spanned!(span=> ::cxx::private::)); - } else if ident.rust == CxxString { + } else if ident.rust == CxxString || ident.rust == CxxExceptionPtr { let span = ident.rust.span(); tokens.extend(quote_spanned!(span=> ::cxx::)); } else if ident.rust == RustString { From b11a62217f79fb250a79a314e146261a9dcfd494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Fri, 24 Feb 2023 23:12:40 +0100 Subject: [PATCH 03/11] Update exception handling to use `std::exception_ptr` This makes the interface between C++ exception handling and Rust `Result` cleaner and allows passing C++ exception from the inner C++ call to the outer C++ call via Rust's `CxxException` unmodified, i.e., without losing information. To allow custom exception types, trait `ToCxxException` was introduced. With this trait, it's possible to convert a Rust error to the wrapper `CxxExeception` via a C++ function, thus allowing throwing other exceptions as well. This required changing `r#try` function into macros, so we can properly default to `rust::Error` for errors not having `ToCxxException` defined. Background: The `throw` statement in C++ (__cxa_throw) effectively first allocates space on the heap and creates the exception within, then starts unwinding. This can be also done via standard C++11 API in two steps. First, `std::make_exception_ptr()` creates a new `std::exception_ptr`, which points into this allocated space and internally is just a pointer (it's a smart pointer much like `std::shared_ptr`). Then, `std::rethrow_exception()` can be used to actually throw and/or rethrow this exception. Basically, the new implementation now uses `std::make_exception_ptr()` called from Rust to construct an exception for the `Result<_, E>` and then after returning it back to C++ via `CxxResult` (which is now BTW smaller, just 8B) the C++ part throws it using `std::rethrow_exception()`. --- book/src/binding/result.md | 29 ++++- gen/src/builtin.rs | 34 +++--- gen/src/write.rs | 22 ++-- macro/src/expand.rs | 31 ++++-- src/cxx.cc | 30 ++++-- src/exception.rs | 12 ++- src/lib.rs | 4 +- src/result.rs | 210 +++++++++++++++++++++++++++++-------- 8 files changed, 275 insertions(+), 97 deletions(-) diff --git a/book/src/binding/result.md b/book/src/binding/result.md index e49dcf4de..f055cb96b 100644 --- a/book/src/binding/result.md +++ b/book/src/binding/result.md @@ -25,7 +25,19 @@ the Rust side produces an error. Note that the return type written inside of cxx::bridge must be written without a second type parameter. Only the Ok type is specified for the purpose of the FFI. The Rust *implementation* (outside of the bridge module) may pick any error -type as long as it has a std::fmt::Display impl. +type as long as it has a `std::fmt::Display` or `cxx:ToCxxException` +implementation. + +Exception is built from the actual error type via `cxx::ToCxxException` trait +which converts the error type into a custom exception by the user code, if such +an implementation exists, else using `cxx::ToCxxExceptionDefault`, which only +requires the type to implement `std::fmt::Display` trait. The sole trait method +of both traits returns a `cxx::CxxException`, which wraps a `std::exception_ptr` +on the C++ side. An implementation of `cxx::ToCxxException` will call the +appropriate C++ function (again, via the bridge) to construct the +`std::exception_ptr`, likely using standard C++ function +`std::make_exception_ptr()` to wrap an exception. The signature on the C++ side +expects `std::exception_ptr` for `cxx::CxxException` on the Rust side. ```rust,noplayground # use std::io; @@ -51,9 +63,10 @@ fn fallible2() -> Result<(), io::Error> { } ``` -The exception that gets thrown by CXX on the C++ side is always of type -`rust::Error` and has the following C++ public API. The `what()` member function -gives the error message according to the Rust error's std::fmt::Display impl. +The exception that gets thrown by CXX on the C++ side is of type `rust::Error` +(unless otherwise specified by `cxx::ToCxxException` trait for a custom error +type) and has the following C++ public API. The `what()` member function gives +the error message according to the Rust error's `std::fmt::Display` implementation. ```cpp,hidelines // rust/cxx.h @@ -85,6 +98,12 @@ a second type parameter. Only the Ok type is specified for the purpose of the FFI. The resulting error type created by CXX when an `extern "C++"` function throws will always be of type **[`cxx::Exception`]**. +Note that this exception can be converted to [`cxx::CxxException`] using its +`Into` trait implementation and returned back to C++ later, as a `Result` with +error type `CxxException`, providing a transparent bridge from the original C++ +exception thrown in a C++ callback through Rust API back to the C++ code calling +the Rust API without loss of information. + [`cxx::Exception`]: https://docs.rs/cxx/*/cxx/struct.Exception.html ```rust,noplayground @@ -141,6 +160,8 @@ static void trycatch(Try &&func, Fail &&fail) noexcept try { func(); } catch (const std::exception &e) { fail(e.what()); +} catch (...) { + fail(""); } # # } // namespace behavior diff --git a/gen/src/builtin.rs b/gen/src/builtin.rs index 277c64f8d..d52b25709 100644 --- a/gen/src/builtin.rs +++ b/gen/src/builtin.rs @@ -20,7 +20,7 @@ pub struct Builtins<'a> { pub manually_drop: bool, pub maybe_uninit: bool, pub trycatch: bool, - pub ptr_len: bool, + pub repr_cxxresult: bool, pub repr_fat: bool, pub rust_str_new_unchecked: bool, pub rust_str_repr: bool, @@ -138,7 +138,7 @@ pub(super) fn write(out: &mut OutFile) { } if builtin.trycatch { - builtin.ptr_len = true; + builtin.repr_cxxresult = true; } out.begin_block(Block::Namespace("rust")); @@ -217,13 +217,21 @@ pub(super) fn write(out: &mut OutFile) { writeln!(out, "using Fat = ::std::array<::std::uintptr_t, 2>;"); } - if builtin.ptr_len { + if builtin.repr_cxxresult { + include.exception = true; include.cstddef = true; out.next_section(); writeln!(out, "struct PtrLen final {{"); writeln!(out, " void *ptr;"); writeln!(out, " ::std::size_t len;"); writeln!(out, "}};"); + writeln!(out, "struct CxxResult final {{"); + writeln!(out, " void *ptr;"); + writeln!(out, "}};"); + writeln!(out, "struct Exception final {{"); + writeln!(out, " CxxResult exc;"); + writeln!(out, " PtrLen msg;"); + writeln!(out, "}};"); } out.end_block(Block::Namespace("repr")); @@ -258,11 +266,11 @@ pub(super) fn write(out: &mut OutFile) { include.string = true; out.next_section(); writeln!(out, "class Fail final {{"); - writeln!(out, " ::rust::repr::PtrLen &throw$;"); + writeln!(out, " ::rust::repr::Exception &throw$;"); writeln!(out, "public:"); writeln!( out, - " Fail(::rust::repr::PtrLen &throw$) noexcept : throw$(throw$) {{}}", + " Fail(::rust::repr::Exception &throw$) noexcept : throw$(throw$) {{}}", ); writeln!(out, " void operator()(char const *) noexcept;"); writeln!(out, " void operator()(std::string const &) noexcept;"); @@ -345,20 +353,6 @@ pub(super) fn write(out: &mut OutFile) { writeln!(out, "}};"); } - if builtin.rust_error { - out.next_section(); - writeln!(out, "template <>"); - writeln!(out, "class impl final {{"); - writeln!(out, "public:"); - writeln!(out, " static Error error(repr::PtrLen repr) noexcept {{"); - writeln!(out, " Error error;"); - writeln!(out, " error.msg = static_cast(repr.ptr);"); - writeln!(out, " error.len = repr.len;"); - writeln!(out, " return error;"); - writeln!(out, " }}"); - writeln!(out, "}};"); - } - if builtin.destroy { out.next_section(); writeln!(out, "template "); @@ -414,6 +408,8 @@ pub(super) fn write(out: &mut OutFile) { writeln!(out, " func();"); writeln!(out, "}} catch (::std::exception const &e) {{"); writeln!(out, " fail(e.what());"); + writeln!(out, "}} catch (...) {{"); + writeln!(out, " fail(\"\");"); writeln!(out, "}}"); out.end_block(Block::Namespace("behavior")); } diff --git a/gen/src/write.rs b/gen/src/write.rs index a18f416b9..c3b6647fc 100644 --- a/gen/src/write.rs +++ b/gen/src/write.rs @@ -706,8 +706,8 @@ fn write_cxx_function_shim<'a>(out: &mut OutFile<'a>, efn: &'a ExternFn) { out.begin_block(Block::ExternC); begin_function_definition(out); if efn.throws { - out.builtin.ptr_len = true; - write!(out, "::rust::repr::PtrLen "); + out.builtin.repr_cxxresult = true; + write!(out, "::rust::repr::Exception "); } else { write_extern_return_type_space(out, &efn.ret); } @@ -783,9 +783,9 @@ fn write_cxx_function_shim<'a>(out: &mut OutFile<'a>, efn: &'a ExternFn) { writeln!(out, ";"); write!(out, " "); if efn.throws { - out.builtin.ptr_len = true; + out.builtin.repr_cxxresult = true; out.builtin.trycatch = true; - writeln!(out, "::rust::repr::PtrLen throw$;"); + writeln!(out, "::rust::repr::Exception throw$;"); writeln!(out, " ::rust::behavior::trycatch("); writeln!(out, " [&] {{"); write!(out, " "); @@ -856,7 +856,8 @@ fn write_cxx_function_shim<'a>(out: &mut OutFile<'a>, efn: &'a ExternFn) { } writeln!(out, ";"); if efn.throws { - writeln!(out, " throw$.ptr = nullptr;"); + writeln!(out, " throw$.exc.ptr = nullptr;"); + writeln!(out, " throw$.msg.ptr = nullptr;"); writeln!(out, " }},"); writeln!(out, " ::rust::detail::Fail(throw$));"); writeln!(out, " return throw$;"); @@ -899,8 +900,8 @@ fn write_rust_function_decl_impl( ) { out.next_section(); if sig.throws { - out.builtin.ptr_len = true; - write!(out, "::rust::repr::PtrLen "); + out.builtin.repr_cxxresult = true; + write!(out, "::rust::repr::CxxResult "); } else { write_extern_return_type_space(out, &sig.ret); } @@ -1074,8 +1075,8 @@ fn write_rust_function_shim_impl( } } if sig.throws { - out.builtin.ptr_len = true; - write!(out, "::rust::repr::PtrLen error$ = "); + out.builtin.repr_cxxresult = true; + write!(out, "::rust::repr::CxxResult error$ = "); } write!(out, "{}(", invoke); let mut needs_comma = false; @@ -1124,7 +1125,8 @@ fn write_rust_function_shim_impl( if sig.throws { out.builtin.rust_error = true; writeln!(out, " if (error$.ptr) {{"); - writeln!(out, " throw ::rust::impl<::rust::Error>::error(error$);"); + writeln!(out, " void *ppexc = &error$.ptr;"); + writeln!(out, " std::rethrow_exception(std::move(*static_cast(ppexc)));"); writeln!(out, " }}"); } if indirect_return { diff --git a/macro/src/expand.rs b/macro/src/expand.rs index f354d0d24..1d6d0f21e 100644 --- a/macro/src/expand.rs +++ b/macro/src/expand.rs @@ -146,6 +146,7 @@ fn expand(ffi: Module, doc: Doc, attrs: OtherAttrs, apis: &[Api], types: &Types) clippy::ptr_as_ptr, clippy::upper_case_acronyms, clippy::use_self, + clippy::items_after_statements, )] #vis #mod_token #ident #expanded } @@ -471,7 +472,7 @@ fn expand_cxx_function_decl(efn: &ExternFn, types: &Types) -> TokenStream { }); let all_args = receiver.chain(args); let ret = if efn.throws { - quote!(-> ::cxx::private::Result) + quote!(-> ::cxx::private::CxxResultWithMessage) } else { expand_extern_return_type(&efn.ret, types, true) }; @@ -1108,7 +1109,7 @@ fn expand_rust_function_shim_impl( None => quote_spanned!(span=> &mut ()), }; requires_closure = true; - expr = quote_spanned!(span=> ::cxx::private::r#try(#out, #expr)); + expr = quote_spanned!(span=> ::cxx::map_rust_result_to_cxx_result!(#out, #expr)); } else if indirect_return { requires_closure = true; expr = quote_spanned!(span=> ::cxx::core::ptr::write(__return, #expr)); @@ -1123,7 +1124,7 @@ fn expand_rust_function_shim_impl( expr = quote_spanned!(span=> ::cxx::private::prevent_unwind(__fn, #closure)); let ret = if sig.throws { - quote!(-> ::cxx::private::Result) + quote!(-> ::cxx::private::CxxResult) } else { expand_extern_return_type(&sig.ret, types, false) }; @@ -1166,16 +1167,12 @@ fn expand_rust_function_shim_super( let args = sig.args.iter().map(|arg| quote!(#arg)); let all_args = receiver.chain(args); - let ret = if let Some((result, _langle, rangle)) = sig.throws_tokens { + let ret = if let Some((result, _langle, _rangle)) = &sig.throws_tokens { let ok = match &sig.ret { Some(ret) => quote!(#ret), None => quote!(()), }; - // Set spans that result in the `Result<...>` written by the user being - // highlighted as the cause if their error type has no Display impl. - let result_begin = quote_spanned!(result.span=> ::cxx::core::result::Result<#ok, impl); - let result_end = quote_spanned!(rangle.span=> ::cxx::core::fmt::Display>); - quote!(-> #result_begin #result_end) + quote_spanned!(result.span=> -> ::cxx::core::result::Result<#ok, ::cxx::CxxException>) } else { expand_return_type(&sig.ret) }; @@ -1192,9 +1189,23 @@ fn expand_rust_function_shim_super( } }; + let call = if let Some((result, _langle, rangle)) = &sig.throws_tokens { + // Set spans that result in the `Result<...>` written by the user being + // highlighted as the cause if their error type is not convertible to + // CxxException (i.e., no `Display` trait by default). + let result_begin = quote_spanned!{ result.span=> + |e| ::cxx::map_rust_error_to_cxx_exception! + }; + let result_end = quote_spanned!{ rangle.span=> (e) }; + quote_spanned! {span=> + #call(#(#vars,)*).map_err( #result_begin #result_end ) + } + } else { + quote_spanned! {span=> #call(#(#vars,)*) } + }; quote_spanned! {span=> #unsafety fn #local_name #generics(#(#all_args,)*) #ret { - #call(#(#vars,)*) + #call } } } diff --git a/src/cxx.cc b/src/cxx.cc index a44605d82..5fc5342ff 100644 --- a/src/cxx.cc +++ b/src/cxx.cc @@ -474,10 +474,6 @@ static_assert(alignof(std::exception_ptr) == alignof(void *), "Unsupported std::exception_ptr alignment"); extern "C" { -const char *cxxbridge1$error(const char *ptr, std::size_t len) noexcept { - return errorCopy(ptr, len); -} - void *cxxbridge1$default_exception(const char *ptr, std::size_t len) noexcept { // Construct an `std::exception_ptr` for the default `rust::Error` in the // space provided by the pointer itself (placement new). @@ -501,8 +497,9 @@ void *cxxbridge1$clone_exception(const char *ptr) noexcept { // Implement the `clone` for `CxxException` on the Rust side, which is just a // pointer to the exception stored in `std::exception_ptr`. void *eptr; - new (&eptr) std::exception_ptr( - *reinterpret_cast(&ptr)); + const void *pptr = &ptr; + new (&eptr) + std::exception_ptr(*static_cast(pptr)); return eptr; } } // extern "C" @@ -559,6 +556,13 @@ struct PtrLen final { void *ptr; std::size_t len; }; +struct CxxResult final { + void *ptr; +}; +struct Exception final { + CxxResult exc; + PtrLen msg; +}; } // namespace repr extern "C" { @@ -580,20 +584,26 @@ using isize_if_unique = struct isize_ignore, rust::isize>::type; class Fail final { - repr::PtrLen &throw$; + repr::Exception &throw$; public: - Fail(repr::PtrLen &throw$) noexcept : throw$(throw$) {} + Fail(repr::Exception &throw$) noexcept : throw$(throw$) {} void operator()(const char *) noexcept; void operator()(const std::string &) noexcept; }; void Fail::operator()(const char *catch$) noexcept { - throw$ = cxxbridge1$exception(catch$, std::strlen(catch$)); + void *eptr; + new (&eptr)::std::exception_ptr(::std::current_exception()); + throw$.exc.ptr = eptr; + throw$.msg = cxxbridge1$exception(catch$, std::strlen(catch$)); } void Fail::operator()(const std::string &catch$) noexcept { - throw$ = cxxbridge1$exception(catch$.data(), catch$.length()); + void *eptr; + new (&eptr)::std::exception_ptr(::std::current_exception()); + throw$.exc.ptr = eptr; + throw$.msg = cxxbridge1$exception(catch$.data(), catch$.length()); } } // namespace detail diff --git a/src/exception.rs b/src/exception.rs index 259b27d4d..cd25960d1 100644 --- a/src/exception.rs +++ b/src/exception.rs @@ -1,12 +1,14 @@ #![cfg(feature = "alloc")] use alloc::boxed::Box; -use core::fmt::{self, Display}; +use core::fmt::{self, Display, Debug}; + +use crate::CxxException; /// Exception thrown from an `extern "C++"` function. #[cfg_attr(doc_cfg, doc(cfg(feature = "alloc")))] -#[derive(Debug)] pub struct Exception { + pub(crate) src: CxxException, pub(crate) what: Box, } @@ -16,6 +18,12 @@ impl Display for Exception { } } +impl Debug for Exception { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Exception").field("what", &self.what).finish() + } +} + #[cfg(feature = "std")] #[cfg_attr(doc_cfg, doc(cfg(feature = "std")))] impl std::error::Error for Exception {} diff --git a/src/lib.rs b/src/lib.rs index 77ec7cff0..c62d776dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -476,6 +476,8 @@ pub use crate::string::CxxString; pub use crate::unique_ptr::UniquePtr; pub use crate::weak_ptr::WeakPtr; pub use cxxbridge_macro::bridge; +#[cfg(feature = "alloc")] +pub use crate::result::{CxxException, ToCxxException, ToCxxExceptionDefault}; /// Synonym for `CxxString`. /// @@ -501,7 +503,7 @@ pub mod private { pub use crate::hash::hash; pub use crate::opaque::Opaque; #[cfg(feature = "alloc")] - pub use crate::result::{r#try, Result}; + pub use crate::result::{CxxResult, CxxResultWithMessage}; pub use crate::rust_slice::RustSlice; pub use crate::rust_str::RustStr; #[cfg(feature = "alloc")] diff --git a/src/result.rs b/src/result.rs index 4ea14d6c7..44b510001 100644 --- a/src/result.rs +++ b/src/result.rs @@ -1,18 +1,19 @@ #![cfg(feature = "alloc")] #![allow(missing_docs)] -use crate::exception::Exception; use alloc::boxed::Box; -use alloc::string::{String, ToString}; +use alloc::string::ToString; use core::fmt::Display; -use core::ptr::{self, NonNull}; -use core::result::Result as StdResult; +use core::mem::MaybeUninit; +use core::ptr::NonNull; use core::slice; use core::str; +use crate::Exception; + #[repr(C)] #[derive(Copy, Clone)] -pub struct PtrLen { +pub(crate) struct PtrLen { pub ptr: NonNull, pub len: usize, } @@ -40,7 +41,7 @@ pub struct CxxException(NonNull); impl CxxException { /// Construct the default `rust::Error` exception from the specified `exc_text`. - pub fn new_default(exc_text: &str) -> Self { + fn new_default(exc_text: &str) -> Self { let exception_ptr = unsafe { default_exception(exc_text.as_ptr(), exc_text.len()) }; @@ -61,6 +62,12 @@ impl Clone for CxxException { } } +impl From for CxxException { + fn from(value: Exception) -> Self { + value.src + } +} + impl Drop for CxxException { fn drop(&mut self) { unsafe { drop_exception(self.0.as_ptr()) }; @@ -74,54 +81,175 @@ unsafe impl Send for CxxException {} // can be shared across threads read-only. unsafe impl Sync for CxxException {} +/// C++ "result" containing `std::exception_ptr` or a `null`. #[repr(C)] -pub union Result { - err: PtrLen, - ok: *const u8, // null -} - -pub unsafe fn r#try(ret: *mut T, result: StdResult) -> Result -where - E: Display, -{ - match result { - Ok(ok) => { - unsafe { ptr::write(ret, ok) } - Result { ok: ptr::null() } +pub struct CxxResult(*mut u8); + +impl From for CxxResult { + fn from(value: CxxException) -> Self { + let res = Self(value.0.as_ptr()); + // NOTE: we are copying the pointer, so we need to forget it here, + // otherwise we'd double-free the `std::exception_ptr`. + core::mem::forget(value); + res + } +} + +impl Drop for CxxResult { + fn drop(&mut self) { + if !self.0.is_null() { + unsafe { drop_exception(self.0) }; } - Err(err) => unsafe { to_c_error(err.to_string()) }, } } -unsafe fn to_c_error(msg: String) -> Result { - let mut msg = msg; - unsafe { msg.as_mut_vec() }.push(b'\0'); - let ptr = msg.as_ptr(); - let len = msg.len(); +impl CxxResult { + /// Construct an empty `Ok` result. + pub fn new() -> Self { + Self(core::ptr::null_mut()) + } +} - extern "C" { - #[link_name = "cxxbridge1$error"] - fn error(ptr: *const u8, len: usize) -> NonNull; +impl CxxResult { + unsafe fn exception(self) -> Result<(), CxxException> { + // SAFETY: We know that the `Result` can only contain a valid `std::exception_ptr` or null. + match unsafe { self.0.as_mut() } { + None => Ok(()), + Some(ptr) => { + let res = CxxException(NonNull::from(ptr)); + // NOTE: we are copying the pointer, so we need to forget this + // object, otherwise we'd double-free the `std::exception_ptr`. + core::mem::forget(self); + Err(res) + } + } } +} - let copy = unsafe { error(ptr, len) }; - let err = PtrLen { ptr: copy, len }; - Result { err } +#[repr(C)] +pub struct CxxResultWithMessage { + pub(crate) res: CxxResult, + pub(crate) msg: PtrLen, } -impl Result { - pub unsafe fn exception(self) -> StdResult<(), Exception> { - unsafe { - if self.ok.is_null() { - Ok(()) - } else { - let err = self.err; - let slice = slice::from_raw_parts_mut(err.ptr.as_ptr(), err.len); - let s = str::from_utf8_unchecked_mut(slice); +impl CxxResultWithMessage { + pub unsafe fn exception(self) -> Result<(), Exception> { + // SAFETY: We know that the `Result` can only contain a valid `std::exception_ptr` or null. + match unsafe { self.res.exception() } { + Ok(_) => Ok(()), + Err(src) => { + // SAFETY: The message is always given for the exception and we constructed it in + // a `Box` in `cxxbridge1$exception()`. We just reconstruct it here. + let what = unsafe { + str::from_utf8_unchecked_mut( + slice::from_raw_parts_mut(self.msg.ptr.as_ptr(), self.msg.len)) + }; Err(Exception { - what: Box::from_raw(s), + src, + what: unsafe { Box::from_raw(what) } }) } } } } + + +/// Trait to convert an arbitrary Rust error into a C++ exception. +/// +/// If an implementation of [`ToCxxException`] is explicitly provided for an `E`, then this +/// implementation will be used for an `extern "Rust"` function returning a `Result<_, E>`. +/// The implementation will likely call back to C++ to create the `exception_ptr` based on +/// some parameters of the Rust error. +/// +/// The default implementation is implemented in a second trait [`ToCxxExceptionDefault`] +/// to work around Rust limitations (missing specialization in stable Rust). It creates +/// a C++ exception of the type `rust::Error` with the text of the Rust exception serialized +/// via `E::to_string()` (unless overridden via [`set_exception_handler()`]). +pub trait ToCxxException { + /// Specific conversion implementation for `Self`. + fn to_cxx_exception(&self) -> CxxException; +} + +/// Default implementation for converting errors to C++ exceptions for types not implementing +/// [`ToCxxException`]. +/// +/// Do not implement this trait. Implement [`ToCxxException`] for `E` instead to customize +/// `Result<_, E>` handling in an `extern "Rust"` function. +pub trait ToCxxExceptionDefault { + fn to_cxx_exception(&self) -> CxxException; +} + +// Identity conversion for an existing C++ exception. +impl ToCxxException for CxxException { + fn to_cxx_exception(&self) -> CxxException { + self.clone() + } +} + +// Default conversion for errors with a message. +impl ToCxxExceptionDefault for &T { + fn to_cxx_exception(&self) -> CxxException { + #[cfg(feature = "std")] + { + // In order to prevent yet another allocation(s) for the string, first + // try to materialize the error message in an on-stack buffer. + const INLINE_BUFFER_SIZE: usize = 4096; + + let mut buffer = MaybeUninit::<[u8; INLINE_BUFFER_SIZE]>::uninit(); + let size = { + use std::io::Write; + let buffer: &mut [u8] = unsafe { buffer.assume_init_mut() }; + let mut cursor = std::io::Cursor::new(buffer); + let res = write!(cursor, "{self}"); + if res.is_err() { + // the buffer was insufficient, allocate a string + let exc_text = self.to_string(); + return CxxException::new_default(&exc_text); + } + cursor.position() as usize + }; + // we have sufficient buffer size, just construct from the inplace + // buffer + let exc_text = unsafe { + std::str::from_utf8_unchecked(&buffer.assume_init_ref()[0..size]) + }; + CxxException::new_default(exc_text) + } + #[cfg(not(feature = "std"))] + { + // no Cursor available in no-std case + let exc_text = self.to_string(); + return CxxException::new_default(&exc_text); + } + } +} + +#[macro_export] +macro_rules! map_rust_error_to_cxx_exception { + ($err:ident) => {{ + #[allow(unused_imports)] + let exc = { + // NOTE: This trick helps us to specialize exception generation for error types without + // the need for `specialization` feature. Namely, `ToCxxException` for `T` has higher + // weight and is selected before `ToCxxExceptionDefault`, which is defined on `&T` (and + // requires auto-deref). If it's not defined, then the default is used. + use $crate::ToCxxExceptionDefault; + use $crate::ToCxxException; + (&$err).to_cxx_exception() + }; + exc + }}; +} + +#[macro_export] +macro_rules! map_rust_result_to_cxx_result { + ($ret_ptr:expr, $result:expr) => { + match $result { + Ok(ok) => { + unsafe { ::core::ptr::write($ret_ptr, ok) }; + $crate::private::CxxResult::new() + } + Err(err) => $crate::private::CxxResult::from(err) + } + }; +} From e7b7116a6c3fcd2a24f00a7237d57eb36defe0a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sat, 25 Feb 2023 10:53:09 +0100 Subject: [PATCH 04/11] Fix a warning on no_std and the expected output for Result Since invalid `Result` output changed, the expected file had to be updated. The output actually got better, since now the location of the actual declaration of the offending struct is also shown, not only the usage. --- src/result.rs | 1 + tests/ui/result_no_display.stderr | 15 +++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/result.rs b/src/result.rs index 44b510001..8e4ca1cdd 100644 --- a/src/result.rs +++ b/src/result.rs @@ -4,6 +4,7 @@ use alloc::boxed::Box; use alloc::string::ToString; use core::fmt::Display; +#[cfg(feature = "std")] use core::mem::MaybeUninit; use core::ptr::NonNull; use core::slice; diff --git a/tests/ui/result_no_display.stderr b/tests/ui/result_no_display.stderr index 44d4b31da..0c3266a90 100644 --- a/tests/ui/result_no_display.stderr +++ b/tests/ui/result_no_display.stderr @@ -1,8 +1,15 @@ -error[E0277]: `NonError` doesn't implement `std::fmt::Display` +error[E0599]: the method `to_cxx_exception` exists for reference `&NonError`, but its trait bounds were not satisfied --> tests/ui/result_no_display.rs:4:19 | 4 | fn f() -> Result<()>; - | ^^^^^^^^^^ `NonError` cannot be formatted with the default formatter + | ^^^^^^^^^^ method cannot be called on `&NonError` due to unsatisfied trait bounds +... +8 | pub struct NonError; + | ------------------- doesn't satisfy `NonError: std::fmt::Display` | - = help: the trait `std::fmt::Display` is not implemented for `NonError` - = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead + = note: the following trait bounds were not satisfied: + `NonError: std::fmt::Display` + which is required by `&NonError: ToCxxExceptionDefault` +note: the trait `std::fmt::Display` must be implemented + --> $RUST/core/src/fmt/mod.rs + = note: this error originates in the macro `::cxx::map_rust_error_to_cxx_exception` (in Nightly builds, run with -Z macro-backtrace for more info) From b7bb463e8554e6a15940b139120494797e62181c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sat, 25 Feb 2023 12:39:20 +0100 Subject: [PATCH 05/11] Fix handling of MSVC `std::exception_ptr` Contrary to the most platforms, which just store a single pointer in `std::exception_ptr`, MSVC stores two. Add necessary code to handle this. --- gen/src/builtin.rs | 10 ++++- gen/src/write.rs | 13 ++++-- src/cxx.cc | 94 +++++++++++++++++++++++-------------------- src/result.rs | 99 +++++++++++++++++++++++----------------------- 4 files changed, 117 insertions(+), 99 deletions(-) diff --git a/gen/src/builtin.rs b/gen/src/builtin.rs index d52b25709..0ced27c04 100644 --- a/gen/src/builtin.rs +++ b/gen/src/builtin.rs @@ -225,11 +225,17 @@ pub(super) fn write(out: &mut OutFile) { writeln!(out, " void *ptr;"); writeln!(out, " ::std::size_t len;"); writeln!(out, "}};"); + writeln!(out, "struct CxxException final {{"); + writeln!(out, " void *repr_ptr;"); + writeln!(out, "#if _MSC_VER >= 1700"); + writeln!(out, " void *repr_ptr_2;"); + writeln!(out, "#endif"); + writeln!(out, "}};"); writeln!(out, "struct CxxResult final {{"); - writeln!(out, " void *ptr;"); + writeln!(out, " CxxException exc;"); writeln!(out, "}};"); writeln!(out, "struct Exception final {{"); - writeln!(out, " CxxResult exc;"); + writeln!(out, " CxxResult res;"); writeln!(out, " PtrLen msg;"); writeln!(out, "}};"); } diff --git a/gen/src/write.rs b/gen/src/write.rs index c3b6647fc..ea0b2ac3f 100644 --- a/gen/src/write.rs +++ b/gen/src/write.rs @@ -856,7 +856,9 @@ fn write_cxx_function_shim<'a>(out: &mut OutFile<'a>, efn: &'a ExternFn) { } writeln!(out, ";"); if efn.throws { - writeln!(out, " throw$.exc.ptr = nullptr;"); + writeln!(out, " throw$.res.exc.repr_ptr = nullptr;"); + #[cfg(target_env = "msvc")] + writeln!(out, " throw$.res.exc.repr_ptr_2 = nullptr;"); writeln!(out, " throw$.msg.ptr = nullptr;"); writeln!(out, " }},"); writeln!(out, " ::rust::detail::Fail(throw$));"); @@ -1124,9 +1126,12 @@ fn write_rust_function_shim_impl( writeln!(out, ";"); if sig.throws { out.builtin.rust_error = true; - writeln!(out, " if (error$.ptr) {{"); - writeln!(out, " void *ppexc = &error$.ptr;"); - writeln!(out, " std::rethrow_exception(std::move(*static_cast(ppexc)));"); + writeln!(out, " if (error$.exc.repr_ptr) {{"); + writeln!(out, " void *ppexc = &error$.exc;"); + writeln!( + out, + " std::rethrow_exception(std::move(*static_cast(ppexc)));" + ); writeln!(out, " }}"); } if indirect_return { diff --git a/src/cxx.cc b/src/cxx.cc index 5fc5342ff..6eb804180 100644 --- a/src/cxx.cc +++ b/src/cxx.cc @@ -467,43 +467,6 @@ class impl final { }; } // namespace -// Ensure statically that `std::exception_ptr` is really a pointer. -static_assert(sizeof(std::exception_ptr) == sizeof(void *), - "Unsupported std::exception_ptr size"); -static_assert(alignof(std::exception_ptr) == alignof(void *), - "Unsupported std::exception_ptr alignment"); - -extern "C" { -void *cxxbridge1$default_exception(const char *ptr, std::size_t len) noexcept { - // Construct an `std::exception_ptr` for the default `rust::Error` in the - // space provided by the pointer itself (placement new). - // - // The `std::exception_ptr` itself is just a pointer, so this effectively - // converts it to the pointer. - void *eptr; - new (&eptr) std::exception_ptr( - std::make_exception_ptr(impl::error_copy(ptr, len))); - return eptr; -} - -void cxxbridge1$drop_exception(char *ptr) noexcept { - // Implement the `drop` for `CxxException` on the Rust side, which is just a - // pointer to the exception stored in `std::exception_ptr`. - auto eptr = std::move(*reinterpret_cast(&ptr)); - // eptr goes out of scope and deallocates `std::exception_ptr`. -} - -void *cxxbridge1$clone_exception(const char *ptr) noexcept { - // Implement the `clone` for `CxxException` on the Rust side, which is just a - // pointer to the exception stored in `std::exception_ptr`. - void *eptr; - const void *pptr = &ptr; - new (&eptr) - std::exception_ptr(*static_cast(pptr)); - return eptr; -} -} // extern "C" - Error::Error(const Error &other) : std::exception(other), msg(other.msg ? errorCopy(other.msg, other.len) : nullptr), @@ -556,19 +519,64 @@ struct PtrLen final { void *ptr; std::size_t len; }; +struct CxxException final { + void *repr_ptr; +#if _MSC_VER >= 1700 + // NOTE: MSVC uses two pointers to store `std::exception_ptr` + void *repr_ptr_2; +#endif +}; struct CxxResult final { - void *ptr; + CxxException exc; }; struct Exception final { - CxxResult exc; + CxxResult res; PtrLen msg; }; } // namespace repr +// Ensure statically that `std::exception_ptr` is really a pointer. +static_assert(sizeof(std::exception_ptr) == sizeof(repr::CxxException), + "Unsupported std::exception_ptr size"); +static_assert(alignof(std::exception_ptr) == alignof(repr::CxxException), + "Unsupported std::exception_ptr alignment"); + extern "C" { repr::PtrLen cxxbridge1$exception(const char *, std::size_t len) noexcept; + +repr::CxxException cxxbridge1$default_exception(const char *ptr, + std::size_t len) noexcept { + // Construct an `std::exception_ptr` for the default `rust::Error` in the + // space provided by the pointer itself (placement new). + // + // The `std::exception_ptr` itself is just a pointer, so this effectively + // converts it to the pointer. + repr::CxxException eptr; + new (&eptr) std::exception_ptr( + std::make_exception_ptr(impl::error_copy(ptr, len))); + return eptr; } +void cxxbridge1$drop_exception(repr::CxxException ptr) noexcept { + // Implement the `drop` for `CxxException` on the Rust side, which is just a + // pointer to the exception stored in `std::exception_ptr`. + void *pptr = &ptr; + std::exception_ptr eptr = std::move(*static_cast(pptr)); + // eptr goes out of scope and deallocates `std::exception_ptr`. +} + +repr::CxxException +cxxbridge1$clone_exception(repr::CxxException &ptr) noexcept { + // Implement the `clone` for `CxxException` on the Rust side, which is just a + // pointer to the exception stored in `std::exception_ptr`. + repr::CxxException eptr; + const void *pptr = &ptr; + new (&eptr) + std::exception_ptr(*static_cast(pptr)); + return eptr; +} +} // extern "C" + namespace detail { // On some platforms size_t is the same C++ type as one of the sized integer // types; on others it is a distinct type. Only in the latter case do we need to @@ -593,16 +601,16 @@ class Fail final { }; void Fail::operator()(const char *catch$) noexcept { - void *eptr; + repr::CxxException eptr; new (&eptr)::std::exception_ptr(::std::current_exception()); - throw$.exc.ptr = eptr; + throw$.res.exc = eptr; throw$.msg = cxxbridge1$exception(catch$, std::strlen(catch$)); } void Fail::operator()(const std::string &catch$) noexcept { - void *eptr; + repr::CxxException eptr; new (&eptr)::std::exception_ptr(::std::current_exception()); - throw$.exc.ptr = eptr; + throw$.res.exc = eptr; throw$.msg = cxxbridge1$exception(catch$.data(), catch$.length()); } } // namespace detail diff --git a/src/result.rs b/src/result.rs index 8e4ca1cdd..1da8ed534 100644 --- a/src/result.rs +++ b/src/result.rs @@ -19,47 +19,61 @@ pub(crate) struct PtrLen { pub len: usize, } +/// Representation of C++ `std::exception_ptr` for all targets except MSVC. +/// +/// This is a single pointer. +#[repr(C)] +#[derive(Copy, Clone)] +#[cfg(not(target_env = "msvc"))] +struct CxxExceptionRepr { + ptr: NonNull, +} + +/// Representation of C++ `std::exception_ptr` for MSVC. +/// +/// Unfortunately, MSVC uses two pointers for `std::exception_ptr`, so we have +/// to account for that. +#[repr(C)] +#[derive(Copy, Clone)] +#[cfg(target_env = "msvc")] +struct CxxExceptionRepr { + ptr: NonNull, + _ptr2: *mut u8, +} + extern "C" { /// Helper to construct the default exception from the error message. #[link_name = "cxxbridge1$default_exception"] - fn default_exception(ptr: *const u8, len: usize) -> *mut u8; + fn default_exception(ptr: *const u8, len: usize) -> CxxExceptionRepr; /// Helper to clone the instance of `std::exception_ptr` on the C++ side. #[link_name = "cxxbridge1$clone_exception"] - fn clone_exception(ptr: *const u8) -> *mut u8; + fn clone_exception(ptr: &CxxExceptionRepr) -> CxxExceptionRepr; /// Helper to drop the instance of `std::exception_ptr` on the C++ side. #[link_name = "cxxbridge1$drop_exception"] - fn drop_exception(ptr: *mut u8); + fn drop_exception(ptr: CxxExceptionRepr); } -/// C++ exception containing `std::exception_ptr`. +/// C++ exception containing an `std::exception_ptr`. /// /// This object is the Rust wrapper over `std::exception_ptr`, so it owns the exception pointer. /// I.e., the exception is either referenced by a `std::exception_ptr` on the C++ side or the /// reference is moved to this object on the Rust side. #[repr(C)] #[must_use] -pub struct CxxException(NonNull); +pub struct CxxException(CxxExceptionRepr); impl CxxException { /// Construct the default `rust::Error` exception from the specified `exc_text`. fn new_default(exc_text: &str) -> Self { - let exception_ptr = unsafe { - default_exception(exc_text.as_ptr(), exc_text.len()) - }; - CxxException( - NonNull::new(exception_ptr) - .expect("Exception conversion returned a null pointer") - ) + let exception_repr = unsafe { default_exception(exc_text.as_ptr(), exc_text.len()) }; + CxxException(exception_repr) } } impl Clone for CxxException { fn clone(&self) -> Self { - let clone_ptr = unsafe { clone_exception(self.0.as_ptr()) }; - Self( - NonNull::new(clone_ptr) - .expect("Exception cloning returned a null pointer") - ) + let exception_repr = unsafe { clone_exception(&self.0) }; + Self(exception_repr) } } @@ -71,7 +85,7 @@ impl From for CxxException { impl Drop for CxxException { fn drop(&mut self) { - unsafe { drop_exception(self.0.as_ptr()) }; + unsafe { drop_exception(self.0) }; } } @@ -84,49 +98,34 @@ unsafe impl Sync for CxxException {} /// C++ "result" containing `std::exception_ptr` or a `null`. #[repr(C)] -pub struct CxxResult(*mut u8); +pub struct CxxResult(Option); impl From for CxxResult { fn from(value: CxxException) -> Self { - let res = Self(value.0.as_ptr()); - // NOTE: we are copying the pointer, so we need to forget it here, - // otherwise we'd double-free the `std::exception_ptr`. - core::mem::forget(value); - res - } -} - -impl Drop for CxxResult { - fn drop(&mut self) { - if !self.0.is_null() { - unsafe { drop_exception(self.0) }; - } + Self(Some(value)) } } impl CxxResult { /// Construct an empty `Ok` result. pub fn new() -> Self { - Self(core::ptr::null_mut()) + Self(None) } } impl CxxResult { unsafe fn exception(self) -> Result<(), CxxException> { // SAFETY: We know that the `Result` can only contain a valid `std::exception_ptr` or null. - match unsafe { self.0.as_mut() } { + match self.0 { None => Ok(()), - Some(ptr) => { - let res = CxxException(NonNull::from(ptr)); - // NOTE: we are copying the pointer, so we need to forget this - // object, otherwise we'd double-free the `std::exception_ptr`. - core::mem::forget(self); - Err(res) - } + Some(ptr) => Err(ptr), } } } +// Assert that the result is not larger than the exception (`Option` will use the niche). +const _: () = assert!(core::mem::size_of::() == core::mem::size_of::()); + #[repr(C)] pub struct CxxResultWithMessage { pub(crate) res: CxxResult, @@ -142,19 +141,20 @@ impl CxxResultWithMessage { // SAFETY: The message is always given for the exception and we constructed it in // a `Box` in `cxxbridge1$exception()`. We just reconstruct it here. let what = unsafe { - str::from_utf8_unchecked_mut( - slice::from_raw_parts_mut(self.msg.ptr.as_ptr(), self.msg.len)) + str::from_utf8_unchecked_mut(slice::from_raw_parts_mut( + self.msg.ptr.as_ptr(), + self.msg.len, + )) }; Err(Exception { src, - what: unsafe { Box::from_raw(what) } + what: unsafe { Box::from_raw(what) }, }) } } } } - /// Trait to convert an arbitrary Rust error into a C++ exception. /// /// If an implementation of [`ToCxxException`] is explicitly provided for an `E`, then this @@ -211,9 +211,8 @@ impl ToCxxExceptionDefault for &T { }; // we have sufficient buffer size, just construct from the inplace // buffer - let exc_text = unsafe { - std::str::from_utf8_unchecked(&buffer.assume_init_ref()[0..size]) - }; + let exc_text = + unsafe { std::str::from_utf8_unchecked(&buffer.assume_init_ref()[0..size]) }; CxxException::new_default(exc_text) } #[cfg(not(feature = "std"))] @@ -234,8 +233,8 @@ macro_rules! map_rust_error_to_cxx_exception { // the need for `specialization` feature. Namely, `ToCxxException` for `T` has higher // weight and is selected before `ToCxxExceptionDefault`, which is defined on `&T` (and // requires auto-deref). If it's not defined, then the default is used. - use $crate::ToCxxExceptionDefault; use $crate::ToCxxException; + use $crate::ToCxxExceptionDefault; (&$err).to_cxx_exception() }; exc @@ -250,7 +249,7 @@ macro_rules! map_rust_result_to_cxx_result { unsafe { ::core::ptr::write($ret_ptr, ok) }; $crate::private::CxxResult::new() } - Err(err) => $crate::private::CxxResult::from(err) + Err(err) => $crate::private::CxxResult::from(err), } }; } From c45ce0575ab4ba341a50ba0eb823f48db0761e9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sat, 25 Feb 2023 19:50:50 +0100 Subject: [PATCH 06/11] Fix: pick up changed tests.h/cc to rebuild bridge Previously, modifying tests.h/cc would not trigger rebuild of the bridge, effectively preventing test development. --- tests/ffi/build.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/ffi/build.rs b/tests/ffi/build.rs index 86f8cd3a5..a1a64b7f0 100644 --- a/tests/ffi/build.rs +++ b/tests/ffi/build.rs @@ -15,4 +15,7 @@ fn main() { build.define("CXX_TEST_INSTANTIATIONS", None); } build.compile("cxx-test-suite"); + + println!("cargo:rerun-if-changed=tests.cc"); + println!("cargo:rerun-if-changed=tests.h"); } From 7528dded9894684a124b634cefe53db3515c271c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sat, 25 Feb 2023 19:52:47 +0100 Subject: [PATCH 07/11] Test: Add tests for custom exception and exception forwarding --- tests/ffi/lib.rs | 50 +++++++++++++++++++++++++++++++++++++++++++++- tests/ffi/tests.cc | 43 +++++++++++++++++++++++++++++++++++++++ tests/ffi/tests.h | 7 +++++++ tests/test.rs | 16 +++++++++++++++ 4 files changed, 115 insertions(+), 1 deletion(-) diff --git a/tests/ffi/lib.rs b/tests/ffi/lib.rs index d6a5f0286..1d93eaa8c 100644 --- a/tests/ffi/lib.rs +++ b/tests/ffi/lib.rs @@ -15,11 +15,30 @@ pub mod cast; pub mod module; -use cxx::{type_id, CxxString, CxxVector, ExternType, SharedPtr, UniquePtr}; +use cxx::{ + type_id, CxxException, CxxString, CxxVector, ExternType, SharedPtr, ToCxxException, UniquePtr, +}; use std::fmt::{self, Display}; use std::mem::MaybeUninit; use std::os::raw::c_char; +/// A custom error with special exception handling. +pub struct CustomError { + pub data: i32, +} + +impl CustomError { + pub fn get_data(&self) -> i32 { + self.data + } +} + +impl ToCxxException for CustomError { + fn to_cxx_exception(&self) -> CxxException { + ffi::make_custom_exception(self) + } +} + #[cxx::bridge(namespace = "tests")] pub mod ffi { #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] @@ -218,6 +237,21 @@ pub mod ffi { fn ns_c_take_ns_shared(shared: AShared); } + extern "Rust" { + type CustomError; + fn get_data(&self) -> i32; + } + + unsafe extern "C++" { + include!("tests/ffi/tests.h"); + + fn make_custom_exception(error: &CustomError) -> CxxException; + fn catch_custom_exception() -> Result<()>; + + fn forward_exception_inner() -> Result<()>; + fn forward_exception_outer() -> Result<()>; + } + extern "C++" { include!("tests/ffi/module.rs.h"); @@ -312,6 +346,9 @@ pub mod ffi { #[cxx_name = "rAliasedFunction"] fn r_aliased_function(x: i32) -> String; + + fn throw_custom_exception() -> Result<()>; + fn forward_exception_middle() -> Result<()>; } struct Dag0 { @@ -646,3 +683,14 @@ fn r_try_return_mutsliceu8(slice: &mut [u8]) -> Result<&mut [u8], Error> { fn r_aliased_function(x: i32) -> String { x.to_string() } + +fn throw_custom_exception() -> Result<(), CustomError> { + Err(CustomError { data: 4711 }) +} + +fn forward_exception_middle() -> Result<(), CxxException> { + ffi::forward_exception_inner().map_err(|e| { + assert_eq!(e.what(), "forward test exc"); + e.into() + }) +} diff --git a/tests/ffi/tests.cc b/tests/ffi/tests.cc index 8cf74bebb..62319f217 100644 --- a/tests/ffi/tests.cc +++ b/tests/ffi/tests.cc @@ -902,6 +902,49 @@ extern "C" const char *cxx_run_test() noexcept { return nullptr; } +std::exception_ptr make_custom_exception(const CustomError &error) { + auto data = error.get_data(); + std::string msg("custom "); + msg += std::to_string(data); + return std::make_exception_ptr(std::logic_error(msg)); +} + +void catch_custom_exception() { + // call a Rust function throwing a custom error + try { + throw_custom_exception(); + } catch (std::logic_error &ex) { + // yes, we caught the expected exception, just rethrow it to the outer + // call from Rust to evaluate + throw; + } catch (...) { + // something wrong, throw an exception with a different message, so the + // test will fail + throw std::logic_error("unexpected exception caught in the outer layer"); + } +} + +void forward_exception_inner() { + // throw a special exception in the inner call + throw std::logic_error("forward test exc"); +} + +void forward_exception_outer() { + try { + // call Rust function, which calls `forward_exception_inner()` to generate + // the exception, which is passed 1:1 through the Rust layer + forward_exception_middle(); + } catch (std::logic_error &) { + // yes, we caught the expected exception, just rethrow it to the outer + // call from Rust to evaluate + throw; + } catch (...) { + // something wrong, throw an exception with a different message, so the + // test will fail + throw std::logic_error("unexpected exception caught in the outer layer"); + } +} + } // namespace tests namespace other { diff --git a/tests/ffi/tests.h b/tests/ffi/tests.h index dc02e4ff8..4bd72c693 100644 --- a/tests/ffi/tests.h +++ b/tests/ffi/tests.h @@ -215,6 +215,13 @@ std::unique_ptr<::F::F> c_return_ns_opaque_ptr(); rust::String cOverloadedFunction(int32_t x); rust::String cOverloadedFunction(rust::Str x); +struct CustomError; +std::exception_ptr make_custom_exception(const CustomError &error); +void catch_custom_exception(); + +void forward_exception_inner(); +void forward_exception_outer(); + } // namespace tests namespace other { diff --git a/tests/test.rs b/tests/test.rs index bcf0a2cd1..838612667 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -378,3 +378,19 @@ fn test_raw_ptr() { assert_eq!(2025, unsafe { ffi::c_take_const_ptr(c3) }); assert_eq!(2025, unsafe { ffi::c_take_mut_ptr(c3 as *mut ffi::C) }); // deletes c3 } + +/// Test throwing a custom exception from a Rust call via `ToCxxException` +/// trait. +#[test] +fn test_custom_exception() { + let err = ffi::catch_custom_exception().expect_err("Error expected"); + assert_eq!(err.what(), "custom 4711"); +} + +/// Test forwarding the exception from the inner C++ function via a middle Rust +/// function into the outer C++ function unmodified. +#[test] +fn test_forward_exception() { + let err = ffi::forward_exception_outer().expect_err("Error expected"); + assert_eq!(err.what(), "forward test exc"); +} From 2805a913721caf39b2607b364825cb10a7c27f96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Fri, 24 Feb 2023 21:51:46 +0100 Subject: [PATCH 08/11] Add NUL character at the end of copied error message C++ error message returned by `what` must be NUL-terminated. However, the current copy function only copied the characters, but didn't add the NUL. Allocate one more byte and set it to NUL. --- src/cxx.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cxx.cc b/src/cxx.cc index 6eb804180..3458814c4 100644 --- a/src/cxx.cc +++ b/src/cxx.cc @@ -449,8 +449,9 @@ static_assert(!std::is_same::const_iterator, "Vec::const_iterator != Vec::iterator"); static const char *errorCopy(const char *ptr, std::size_t len) { - char *copy = new char[len]; + char *copy = new char[len + 1]; std::memcpy(copy, ptr, len); + copy[len] = 0; return copy; } From 3714d233cbd070da3adfec0a982ebaf13333a52c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sat, 25 Feb 2023 22:24:38 +0100 Subject: [PATCH 09/11] Disable the ui test `result_no_display` due to a bug in `trybuild` crate. The normalization in `trybuild` crate doesn't work in the CI (but works fine locally). --- tests/ui/{result_no_display.rs => result_no_display.rs.disabled} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/ui/{result_no_display.rs => result_no_display.rs.disabled} (100%) diff --git a/tests/ui/result_no_display.rs b/tests/ui/result_no_display.rs.disabled similarity index 100% rename from tests/ui/result_no_display.rs rename to tests/ui/result_no_display.rs.disabled From 380b9ef7ac94c46a47ef7dbcd427bf3d21ebf3e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Tue, 28 Feb 2023 18:11:14 +0100 Subject: [PATCH 10/11] Remove an unneeded exported macro Instead, expand the match directly in `expand.rs` to minimize the public API. --- macro/src/expand.rs | 14 +++++++++++--- src/result.rs | 13 ------------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/macro/src/expand.rs b/macro/src/expand.rs index 1d6d0f21e..1985c9192 100644 --- a/macro/src/expand.rs +++ b/macro/src/expand.rs @@ -1109,7 +1109,15 @@ fn expand_rust_function_shim_impl( None => quote_spanned!(span=> &mut ()), }; requires_closure = true; - expr = quote_spanned!(span=> ::cxx::map_rust_result_to_cxx_result!(#out, #expr)); + expr = quote_spanned!(span=> + match #expr { + Ok(ok) => { + ::core::ptr::write(#out, ok); + ::cxx::private::CxxResult::new() + } + Err(err) => ::cxx::private::CxxResult::from(err), + } + ); } else if indirect_return { requires_closure = true; expr = quote_spanned!(span=> ::cxx::core::ptr::write(__return, #expr)); @@ -1193,10 +1201,10 @@ fn expand_rust_function_shim_super( // Set spans that result in the `Result<...>` written by the user being // highlighted as the cause if their error type is not convertible to // CxxException (i.e., no `Display` trait by default). - let result_begin = quote_spanned!{ result.span=> + let result_begin = quote_spanned! { result.span=> |e| ::cxx::map_rust_error_to_cxx_exception! }; - let result_end = quote_spanned!{ rangle.span=> (e) }; + let result_end = quote_spanned! { rangle.span=> (e) }; quote_spanned! {span=> #call(#(#vars,)*).map_err( #result_begin #result_end ) } diff --git a/src/result.rs b/src/result.rs index 1da8ed534..dacfb3a2c 100644 --- a/src/result.rs +++ b/src/result.rs @@ -240,16 +240,3 @@ macro_rules! map_rust_error_to_cxx_exception { exc }}; } - -#[macro_export] -macro_rules! map_rust_result_to_cxx_result { - ($ret_ptr:expr, $result:expr) => { - match $result { - Ok(ok) => { - unsafe { ::core::ptr::write($ret_ptr, ok) }; - $crate::private::CxxResult::new() - } - Err(err) => $crate::private::CxxResult::from(err), - } - }; -} From 15225fbee3e244d118093a59ca4c3a834811ea2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Schre=CC=81ter?= Date: Sun, 5 Mar 2023 22:12:47 +0100 Subject: [PATCH 11/11] Upgrade trybuild to 1.0.78 and reenable the disabled test --- Cargo.toml | 2 +- .../ui/{result_no_display.rs.disabled => result_no_display.rs} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename tests/ui/{result_no_display.rs.disabled => result_no_display.rs} (100%) diff --git a/Cargo.toml b/Cargo.toml index 5ed5809c3..ea5839e44 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,7 +35,7 @@ cxx-build = { version = "=1.0.91", path = "gen/build" } cxx-gen = { version = "0.7", path = "gen/lib" } cxx-test-suite = { version = "0", path = "tests/ffi" } rustversion = "1.0" -trybuild = { version = "1.0.66", features = ["diff"] } +trybuild = { version = "1.0.78", features = ["diff"] } [lib] doc-scrape-examples = false diff --git a/tests/ui/result_no_display.rs.disabled b/tests/ui/result_no_display.rs similarity index 100% rename from tests/ui/result_no_display.rs.disabled rename to tests/ui/result_no_display.rs