From ab7a44789a7be7932106ffb411803072c46a193e Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Mon, 26 Oct 2020 14:56:43 +1100 Subject: [PATCH] Add workaround for JNA bug when passing small structs by value. The current version of JNA appears to have a bug when passing small structs as arguments by value, on arm64 platforms, when the function call has too many arguments to pass the struct in registers. It turns out that our generated code does this a lot thanks to the way we pass around `RustBuffer` structs. This commit adds a temporary workaround for the issue, by padding our structs to be larger than 16 bytes, which causes them to be passed as pointers rather than by value. See https://github.com/mozilla/application-services/issues/3646 for investigation of the JNA issue, and https://github.com/mozilla/uniffi-rs/issues/334 to track the work of removing this workaround once a fix is released in upstream JNA. --- uniffi/src/ffi/foreignbytes.rs | 46 +++++++------ uniffi/src/ffi/rustbuffer.rs | 68 ++++++++----------- .../kotlin/templates/ErrorTemplate.kt | 2 +- .../kotlin/templates/RustBufferTemplate.kt | 11 ++- .../python/templates/RustBufferTemplate.py | 7 +- .../swift/templates/RustBufferTemplate.swift | 8 ++- .../templates/Template-Bridging-Header.h | 5 ++ 7 files changed, 79 insertions(+), 68 deletions(-) diff --git a/uniffi/src/ffi/foreignbytes.rs b/uniffi/src/ffi/foreignbytes.rs index bf3b52191f..de604dd682 100644 --- a/uniffi/src/ffi/foreignbytes.rs +++ b/uniffi/src/ffi/foreignbytes.rs @@ -31,9 +31,30 @@ pub struct ForeignBytes { len: i32, /// The pointer to the foreign-owned bytes. data: *const u8, + /// This forces the struct to be larger than 16 bytes, as a temporary workaround for a bug in JNA. + /// See https://github.com/mozilla/uniffi-rs/issues/334 for details. + padding: i64, + padding2: i32, } impl ForeignBytes { + /// Creates a `ForeignBytes` from its constituent fields. + /// + /// This is intended mainly as an internal convenience function and should not + /// be used outside of this module. + /// + /// # Safety + /// + /// You must ensure that the raw parts uphold the documented invariants of this class. + pub unsafe fn from_raw_parts(data: *const u8, len: i32) -> Self { + Self { + len, + data, + padding: 0, + padding2: 0, + } + } + /// View the foreign bytes as a `&[u8]`. /// /// # Panics @@ -72,10 +93,7 @@ mod test { #[test] fn test_foreignbytes_access() { let v = vec![1u8, 2, 3]; - let fbuf = ForeignBytes { - len: 3, - data: v.as_ptr(), - }; + let fbuf = unsafe { ForeignBytes::from_raw_parts(v.as_ptr(), 3) }; assert_eq!(fbuf.len(), 3); assert_eq!(fbuf.as_slice(), &[1u8, 2, 3]); } @@ -83,30 +101,21 @@ mod test { #[test] fn test_foreignbytes_empty() { let v = Vec::::new(); - let fbuf = ForeignBytes { - len: 0, - data: v.as_ptr(), - }; + let fbuf = unsafe { ForeignBytes::from_raw_parts(v.as_ptr(), 0) }; assert_eq!(fbuf.len(), 0); assert_eq!(fbuf.as_slice(), &[0u8; 0]); } #[test] fn test_foreignbytes_null_means_empty() { - let fbuf = ForeignBytes { - len: 0, - data: std::ptr::null_mut(), - }; + let fbuf = unsafe { ForeignBytes::from_raw_parts(std::ptr::null_mut(), 0) }; assert_eq!(fbuf.as_slice(), &[0u8; 0]); } #[test] #[should_panic] fn test_foreignbytes_null_must_have_zero_length() { - let fbuf = ForeignBytes { - len: 12, - data: std::ptr::null_mut(), - }; + let fbuf = unsafe { ForeignBytes::from_raw_parts(std::ptr::null_mut(), 12) }; fbuf.as_slice(); } @@ -114,10 +123,7 @@ mod test { #[should_panic] fn test_foreignbytes_provided_len_must_be_non_negative() { let v = vec![0u8, 1, 2]; - let fbuf = ForeignBytes { - len: -1, - data: v.as_ptr(), - }; + let fbuf = unsafe { ForeignBytes::from_raw_parts(v.as_ptr(), -1) }; fbuf.as_slice(); } } diff --git a/uniffi/src/ffi/rustbuffer.rs b/uniffi/src/ffi/rustbuffer.rs index 4040d58d29..079870ebc4 100644 --- a/uniffi/src/ffi/rustbuffer.rs +++ b/uniffi/src/ffi/rustbuffer.rs @@ -58,6 +58,9 @@ pub struct RustBuffer { len: i32, /// The pointer to the allocated buffer of the `Vec`. data: *mut u8, + /// This forces the struct to be larger than 16 bytes, as a temporary workaround for a bug in JNA. + /// See https://github.com/mozilla/uniffi-rs/issues/334 for details. + padding: i64, } impl RustBuffer { @@ -69,6 +72,23 @@ impl RustBuffer { Self::from_vec(Vec::new()) } + /// Creates a `RustBuffer` from its constituent fields. + /// + /// This is intended mainly as an internal convenience function and should not + /// be used outside of this module. + /// + /// # Safety + /// + /// You must ensure that the raw parts uphold the documented invariants of this class. + pub unsafe fn from_raw_parts(data: *mut u8, len: i32, capacity: i32) -> Self { + Self { + capacity, + len, + data, + padding: 0, + } + } + /// Get the current length of the buffer, as a `usize`. /// /// This is mostly a helper function to convert the `i32` length field @@ -119,11 +139,7 @@ impl RustBuffer { let capacity = i32::try_from(v.capacity()).expect("buffer capacity cannot fit into a i32."); let len = i32::try_from(v.len()).expect("buffer length cannot fit into a i32."); let mut v = std::mem::ManuallyDrop::new(v); - Self { - capacity, - len, - data: v.as_mut_ptr(), - } + unsafe { Self::from_raw_parts(v.as_mut_ptr(), len, capacity) } } /// Converts this `RustBuffer` back into an owned `Vec`. @@ -177,11 +193,7 @@ impl Default for RustBuffer { unsafe impl crate::deps::ffi_support::IntoFfi for RustBuffer { type Value = Self; fn ffi_default() -> Self { - Self { - capacity: 0, - len: 0, - data: std::ptr::null_mut(), - } + unsafe { Self::from_raw_parts(std::ptr::null_mut(), 0, 0) } } fn into_ffi_value(self) -> Self::Value { self @@ -220,11 +232,7 @@ mod test { #[test] fn test_rustbuffer_null_means_empty() { // This is how foreign-language code might cheaply indicate an empty buffer. - let rbuf = RustBuffer { - capacity: 0, - len: 0, - data: std::ptr::null_mut(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(std::ptr::null_mut(), 0, 0) }; assert_eq!(rbuf.destroy_into_vec().as_slice(), &[0u8; 0]); } @@ -232,22 +240,14 @@ mod test { #[should_panic] fn test_rustbuffer_null_must_have_no_capacity() { // We guard against foreign-language code providing this kind of invalid struct. - let rbuf = RustBuffer { - capacity: 1, - len: 0, - data: std::ptr::null_mut(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(std::ptr::null_mut(), 0, 1) }; rbuf.destroy_into_vec(); } #[test] #[should_panic] fn test_rustbuffer_null_must_have_zero_length() { // We guard against foreign-language code providing this kind of invalid struct. - let rbuf = RustBuffer { - capacity: 0, - len: 12, - data: std::ptr::null_mut(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(std::ptr::null_mut(), 12, 0) }; rbuf.destroy_into_vec(); } @@ -256,11 +256,7 @@ mod test { fn test_rustbuffer_provided_capacity_must_be_non_negative() { // We guard against foreign-language code providing this kind of invalid struct. let mut v = vec![0u8, 1, 2]; - let rbuf = RustBuffer { - capacity: -7, - len: 3, - data: v.as_mut_ptr(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(v.as_mut_ptr(), 3, -7) }; rbuf.destroy_into_vec(); } @@ -269,11 +265,7 @@ mod test { fn test_rustbuffer_provided_len_must_be_non_negative() { // We guard against foreign-language code providing this kind of invalid struct. let mut v = vec![0u8, 1, 2]; - let rbuf = RustBuffer { - capacity: 3, - len: -1, - data: v.as_mut_ptr(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(v.as_mut_ptr(), -1, 3) }; rbuf.destroy_into_vec(); } @@ -282,11 +274,7 @@ mod test { fn test_rustbuffer_provided_len_must_not_exceed_capacity() { // We guard against foreign-language code providing this kind of invalid struct. let mut v = vec![0u8, 1, 2]; - let rbuf = RustBuffer { - capacity: 2, - len: 3, - data: v.as_mut_ptr(), - }; + let rbuf = unsafe { RustBuffer::from_raw_parts(v.as_mut_ptr(), 3, 2) }; rbuf.destroy_into_vec(); } diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ErrorTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ErrorTemplate.kt index c0e72e9c2c..1f51fc6db0 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ErrorTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ErrorTemplate.kt @@ -113,7 +113,7 @@ internal open class {{e.name()}} : RustError() { {% for value in e.values() -%} {{loop.index}} -> return {{e.name()}}Exception.{{value}}(message) as E {% endfor -%} - else -> throw RuntimeException("Invalid error received...") + else -> throw RuntimeException("Invalid error received: $code, $message") } } } diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/RustBufferTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/RustBufferTemplate.kt index ae65bd2ffd..6a493fece6 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/RustBufferTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/RustBufferTemplate.kt @@ -2,11 +2,13 @@ // A rust-owned buffer is represented by its capacity, its current length, and a // pointer to the underlying data. -@Structure.FieldOrder("capacity", "len", "data") +@Structure.FieldOrder("capacity", "len", "data", "padding") open class RustBuffer : Structure() { @JvmField var capacity: Int = 0 @JvmField var len: Int = 0 @JvmField var data: Pointer? = null + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for this weird "padding" field. + @JvmField var padding: Long = 0 class ByValue : RustBuffer(), Structure.ByValue @@ -37,10 +39,13 @@ open class RustBuffer : Structure() { // then we might as well copy it into a `RustBuffer`. But it's here for API // completeness. -@Structure.FieldOrder("len", "data") +@Structure.FieldOrder("len", "data", "padding", "padding2") open class ForeignBytes : Structure() { @JvmField var len: Int = 0 @JvmField var data: Pointer? = null + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for these weird "padding" fields. + @JvmField var padding: Long = 0 + @JvmField var padding2: Int = 0 class ByValue : ForeignBytes(), Structure.ByValue } @@ -137,4 +142,4 @@ class RustBufferBuilder() { bbuf.put(v) } } -} \ No newline at end of file +} diff --git a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py index 7a603a2d2a..6baac71ddc 100644 --- a/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/RustBufferTemplate.py @@ -4,6 +4,8 @@ class RustBuffer(ctypes.Structure): ("capacity", ctypes.c_int32), ("len", ctypes.c_int32), ("data", ctypes.POINTER(ctypes.c_char)), + # Ref https://github.com/mozilla/uniffi-rs/issues/334 for this weird "padding" field. + ("padding", ctypes.c_int64), ] @staticmethod @@ -139,7 +141,10 @@ class ForeignBytes(ctypes.Structure): _fields_ = [ ("len", ctypes.c_int32), ("data", ctypes.POINTER(ctypes.c_char)), + # Ref https://github.com/mozilla/uniffi-rs/issues/334 for these weird "padding" fields. + ("padding", ctypes.c_int64), + ("padding2", ctypes.c_int32), ] def __str__(self): - return "ForeignBytes(len={}, data={})".format(self.len, self.data[0:self.len]) \ No newline at end of file + return "ForeignBytes(len={}, data={})".format(self.len, self.data[0:self.len]) diff --git a/uniffi_bindgen/src/bindings/swift/templates/RustBufferTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/RustBufferTemplate.swift index 650c831b76..259512838b 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/RustBufferTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/RustBufferTemplate.swift @@ -6,7 +6,8 @@ extension RustBuffer { {{ ci.ffi_rustbuffer_from_bytes().name() }}(ForeignBytes(bufferPointer: ptr), err) } } - self.init(capacity: rbuf.capacity, len: rbuf.len, data: rbuf.data) + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for the extra "padding" arg. + self.init(capacity: rbuf.capacity, len: rbuf.len, data: rbuf.data, padding: 0) } // Frees the buffer in place. @@ -20,6 +21,7 @@ extension RustBuffer { extension ForeignBytes { init(bufferPointer: UnsafeBufferPointer) { - self.init(len: Int32(bufferPointer.count), data: bufferPointer.baseAddress) + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for the extra "padding" args. + self.init(len: Int32(bufferPointer.count), data: bufferPointer.baseAddress, padding: 0, padding2: 0) } -} \ No newline at end of file +} diff --git a/uniffi_bindgen/src/bindings/swift/templates/Template-Bridging-Header.h b/uniffi_bindgen/src/bindings/swift/templates/Template-Bridging-Header.h index cee6699235..45264f765b 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/Template-Bridging-Header.h +++ b/uniffi_bindgen/src/bindings/swift/templates/Template-Bridging-Header.h @@ -11,12 +11,17 @@ typedef struct RustBuffer int32_t capacity; int32_t len; uint8_t *_Nullable data; + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for this weird "padding" field. + int64_t padding; } RustBuffer; typedef struct ForeignBytes { int32_t len; const uint8_t *_Nullable data; + // Ref https://github.com/mozilla/uniffi-rs/issues/334 for these weird "padding" fields. + int64_t padding; + int32_t padding2; } ForeignBytes; // Error definitions