Messages in this thread | | | Date | Wed, 24 May 2023 11:19:13 +0000 | Subject | Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}` | From | Alice Ryhl <> |
| |
Gary Guo <gary@garyguo.net> writes: > On Wed, 17 May 2023 20:31:15 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: >> + /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`]. >> + /// >> + /// This code relies on the `repr(C)` layout of structs as described in >> + /// <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs>. >> + /// >> + /// # Safety >> + /// >> + /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it >> + /// can only be called once for each previous call to [`Arc::into_raw`]. >> + pub unsafe fn from_raw(ptr: *const T) -> Self { >> + // SAFETY: The safety requirement ensures that the pointer is valid. >> + let val_align = core::mem::align_of_val(unsafe { &*ptr }); >> + let refcount_size = core::mem::size_of::<Opaque<bindings::refcount_t>>(); >> + >> + // Use the `repr(C)` algorithm to compute the offset of `data` in `ArcInner`. >> + // >> + // Pseudo-code for the `#[repr(C)]` algorithm can be found here: >> + // <https://doc.rust-lang.org/reference/type-layout.html#reprc-structs> >> + let mut val_offset = refcount_size; >> + let val_misalign = val_offset % val_align; >> + if val_misalign > 0 { >> + val_offset += val_align - val_misalign; >> + } > > Given the layout of the whole ArcInner can be calculated as > > Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().0.pad_to_align() > > The offset of `data` could be more intuitively calculated by > > Layout::new::<bindings::refcount_t>().extend(Layout::for_value(&*ptr)).unwrap_unchecked().1 > > or > > Layout::new::<bindings::refcount_t>().align_to(val_align).unwrap_unchecked().pad_to_align().size()
I'm not a big fan of the `pad_to_align` version (which is also what the rust branch uses), but I like the version you posted with `extend`, and I agree that it is clear and intuitive. I will use that in the next version of the patchset. Thanks for the suggestion.
Alice
| |