Merge pull request #335 from mozilla/jna-arm64-workaround; r=eoger

rfk · web-flow · commit 937f9b76f56e · 2020-10-27T09:25:12.000+11:00
Add workaround for JNA bug when passing small structs by value.
diff --git 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,52 +93,37 @@ 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]);
     }
 
     #[test]
     fn test_foreignbytes_empty() {
         let v = Vec::<u8>::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();
     }
 
     #[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
@@ -58,6 +58,9 @@ pub struct RustBuffer {
     len: i32,
     /// The pointer to the allocated buffer of the `Vec<u8>`.
     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<u8>`.
@@ -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,34 +232,22 @@ 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]);
     }
 
     #[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
@@ -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
@@ -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)
         }
     }
-}
+}
diff --git 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])
+        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
@@ -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<UInt8>) {
-        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)
     }
-}
+}
diff --git 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

Original file line number	Diff line number	Diff line change
`@@ -113,7 +113,7 @@ internal open class {{e.name()}} : RustError() {`
`113`	`113`	`{% for value in e.values() -%}`
`114`	`114`	`{{loop.index}} -> return {{e.name()}}Exception.{{value}}(message) as E`
`115`	`115`	`{% endfor -%}`
`116`		`- else -> throw RuntimeException("Invalid error received...")`
	`116`	`+ else -> throw RuntimeException("Invalid error received: $code, $message")`
`117`	`117`	`}`
`118`	`118`	`}`
`119`	`119`	`}`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,8 @@ extension RustBuffer {`
`6`	`6`	`{{ ci.ffi_rustbuffer_from_bytes().name() }}(ForeignBytes(bufferPointer: ptr), err)`
`7`	`7`	`}`
`8`	`8`	`}`
`9`		`- self.init(capacity: rbuf.capacity, len: rbuf.len, data: rbuf.data)`
	`9`	`+ // Ref https://github.com/mozilla/uniffi-rs/issues/334 for the extra "padding" arg.`
	`10`	`+ self.init(capacity: rbuf.capacity, len: rbuf.len, data: rbuf.data, padding: 0)`
`10`	`11`	`}`
`11`	`12`
`12`	`13`	`// Frees the buffer in place.`
`@@ -20,6 +21,7 @@ extension RustBuffer {`
`20`	`21`
`21`	`22`	`extension ForeignBytes {`
`22`	`23`	`init(bufferPointer: UnsafeBufferPointer<UInt8>) {`
`23`		`- self.init(len: Int32(bufferPointer.count), data: bufferPointer.baseAddress)`
	`24`	`+ // Ref https://github.com/mozilla/uniffi-rs/issues/334 for the extra "padding" args.`
	`25`	`+ self.init(len: Int32(bufferPointer.count), data: bufferPointer.baseAddress, padding: 0, padding2: 0)`
`24`	`26`	`}`
`25`		`-}`
	`27`	`+}`