Re: [PATCH v1 3/7] rust: sync: add `Arc::{from_raw, into_raw}`

[Alice Ryhl <aliceryhl@google.com>]

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