Fix handling of MSVC std::exception_ptr

schreter · schreter · commit b7bb463e8554 · 2023-02-25T12:59:01.000+01:00
Contrary to the most platforms, which just store a single pointer in
`std::exception_ptr`, MSVC stores two. Add necessary code to handle
this.
diff --git 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
@@ -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<std::exception_ptr*>(ppexc)));");
+        writeln!(out, "  if (error$.exc.repr_ptr) {{");
+        writeln!(out, "    void *ppexc = &error$.exc;");
+        writeln!(
+            out,
+            "    std::rethrow_exception(std::move(*static_cast<std::exception_ptr*>(ppexc)));"
+        );
         writeln!(out, "  }}");
     }
     if indirect_return {
diff --git a/src/cxx.cc b/src/cxx.cc
@@ -467,43 +467,6 @@ class impl<Error> 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>::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<std::exception_ptr *>(&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<const std::exception_ptr *>(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>::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<std::exception_ptr *>(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<const std::exception_ptr *>(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
@@ -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<u8>,
+}
+
+/// 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<u8>,
+    _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<u8>);
+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<Exception> 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<CxxException>);
 
 impl From<CxxException> 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::<CxxResult>() == core::mem::size_of::<CxxException>());
+
 #[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<T: Display> 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),
         }
     };
 }
