-
Notifications
You must be signed in to change notification settings - Fork 53
Description
I looked at r_struct.rs and saw:
Lines 33 to 64 in 2370a35
| // Ruby provides some inline functions to get a pointer to the struct's | |
| // contents, but we have to reimplement those for Rust. The for that we need | |
| // the definition of RStruct, but that isn't public, so we have to duplicate it | |
| // here. | |
| mod sys { | |
| use rb_sys::{RBasic, VALUE, ruby_fl_type, ruby_fl_ushift::RUBY_FL_USHIFT}; | |
| pub const EMBED_LEN_MAX: u32 = rb_sys::ruby_rvalue_flags::RVALUE_EMBED_LEN_MAX as u32; | |
| pub const EMBED_LEN_MASK: u32 = | |
| ruby_fl_type::RUBY_FL_USER2 as u32 | ruby_fl_type::RUBY_FL_USER1 as u32; | |
| pub const EMBED_LEN_SHIFT: u32 = RUBY_FL_USHIFT as u32 + 1; | |
| #[repr(C)] | |
| #[derive(Copy, Clone)] | |
| pub struct RStruct { | |
| pub basic: RBasic, | |
| pub as_: As, | |
| } | |
| #[repr(C)] | |
| #[derive(Copy, Clone)] | |
| pub union As { | |
| pub heap: Heap, | |
| pub ary: [VALUE; EMBED_LEN_MAX as usize], | |
| } | |
| #[repr(C)] | |
| #[derive(Debug, Copy, Clone)] | |
| pub struct Heap { | |
| pub len: std::ffi::c_long, | |
| pub ptr: *const VALUE, | |
| } | |
| } |
Lines 111 to 139 in 2370a35
| /// Return the members of the struct as a slice of [`Value`]s. The order | |
| /// will be the order the of the member names when the struct class was | |
| /// defined. | |
| /// | |
| /// # Safety | |
| /// | |
| /// Ruby may modify or free the memory backing the returned slice, the | |
| /// caller must ensure this does not happen. | |
| unsafe fn as_slice(&self) -> &[Value] { | |
| unsafe { self.as_slice_unconstrained() } | |
| } | |
| unsafe fn as_slice_unconstrained<'a>(self) -> &'a [Value] { | |
| unsafe { | |
| debug_assert_value!(self); | |
| let r_basic = self.r_basic_unchecked(); | |
| let flags = r_basic.as_ref().flags; | |
| if (flags & sys::EMBED_LEN_MASK as VALUE) != 0 { | |
| let len = (flags & sys::EMBED_LEN_MASK as VALUE) >> sys::EMBED_LEN_SHIFT as VALUE; | |
| slice::from_raw_parts( | |
| &self.as_internal().as_ref().as_.ary as *const VALUE as *const Value, | |
| len as usize, | |
| ) | |
| } else { | |
| let h = self.as_internal().as_ref().as_.heap; | |
| slice::from_raw_parts(h.ptr as *const Value, h.len as usize) | |
| } | |
| } | |
| } |
This:
- Violates CRuby internals
- is extremely brittle and would likely segfault if CRuby does any change in that area (e.g. change flags)
- Exposes things that probably just don't really need to be exposed
- Contributes to the reputation of Rust-based extensions being frequently broken (e.g. ruby-bench recently dropped commonmarker because of that).
For manipulating a Struct, the public C API (RSTRUCT_GET/rb_struct_aref, RSTRUCT_SET/rb_struct_aset, RSTRUCT_LEN/rb_struct_size, etc) seems enough.
The fix here seems clear: remove RStruct#as_slice since it's not reasonable to support and I'd guess nobody uses/needs it, and then remove this hardcoded duplication of CRuby internals.
Does anyone actually use RStruct#as_slice? For what?
For context I'm looking at this in the context of truffleruby/truffleruby#3396.
From the POV of TruffleRuby this is even clearer, such internals are clearly marked as internals, and they cannot even be implemented on TruffleRuby (Struct's are not backed by native memory and there is no array of struct values).