Fix cross-compilation of C++ interpreter API to 32-bit architecture #3810
Conversation
|
Draft, not ready for review yet. One CI test run, then patch with fix coming up. |
cbindgen doesn't reliably tranfser the 64-bit/32-bit #ifdefs around ValueOpaque. Instead, move Value onto the heap by using a Box.
tronical
force-pushed
the
simon/32bit-cross
branch
from
ec02b53
to
0bd0349
Compare
tronical
changed the title
|
This requires a very very careful review. I might have botched up the ownership.... |
internal/interpreter/ffi.rs
Outdated
| } | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn slint_interpreter_value_eq(a: &ValueOpaque, b: &ValueOpaque) -> bool { | ||
| a.as_value() == b.as_value() | ||
| pub extern "C" fn slint_interpreter_value_eq(a: &Box<Value>, b: &Box<Value>) -> bool { |
There was a problem hiding this comment.
i think that works, but i'd pass the value by reference instead of Box
| pub extern "C" fn slint_interpreter_value_eq(a: &Box<Value>, b: &Box<Value>) -> bool { | |
| pub extern "C" fn slint_interpreter_value_eq(a: &Value, b: &Value) -> bool { |
passing a reference to a box is one indirection too many.
This is valid for most calls that don't take ownership
There was a problem hiding this comment.
You're right. Will change.
| val: &ValueOpaque, | ||
| out: *mut SharedVector<ValueOpaque>, | ||
| val: &Box<Value>, | ||
| out: *mut SharedVector<Box<Value>>, |
There was a problem hiding this comment.
SharedVector<Box<Value>>
this is not ideal because the destructor of SharedVector in C++ will not destroy the Value :-(
But since the only caller actualy re-interprets-cast this to the right thing, this is probably fine.
| stru.as_struct() | ||
| .get_field(std::str::from_utf8(&name).unwrap()) | ||
| .and_then(|v| unsafe { (v as *const Value as *const ValueOpaque).as_ref() }) | ||
| ) -> *mut Value { |
There was a problem hiding this comment.
(could be Option<Box<Value>>, but i don't know if cbingen would generate the right stuff)
There was a problem hiding this comment.
I tried that as well, but unfortunately it's only available for C. (See the box test in cbindgen repo)
| let args = std::mem::transmute::<Slice<ValueOpaque>, Slice<Value>>(args); | ||
| args: Slice<Box<Value>>, | ||
| ) -> *mut Value { | ||
| let args = args.iter().map(|vb| vb.as_ref().clone()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
this is unfortunate that we need to clone everything, but i can't think of a better way.
|
Good point about valgrind. I'll run that tomorrow. |
…f reference to boxed value.
The memory the box allocated for the out data was never freed, instead the pointer was just overwritten. Simplify by returning an owned pointer.
cbindgen doesn't reliably tranfser the 64-bit/32-bit #ifdefs around ValueOpaque.
Instead, move Value onto the heap by using a Box.
Fixes #3653