From a6157a926dbf9d470ac656013b1b94162602dec3 Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Wed, 24 Apr 2024 20:28:06 -0600 Subject: [PATCH] improve use of unsafe Rust in arena.rs (#2391) --- src/arena.rs | 309 ++++++++++++++++------------------------- src/machine/streams.rs | 17 +-- 2 files changed, 119 insertions(+), 207 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 172e6e9d..d24acfe9 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -11,7 +11,6 @@ use crate::read::*; use crate::parser::dashu::{Integer, Rational}; use ordered_float::OrderedFloat; -use std::alloc; use std::cell::UnsafeCell; use std::fmt; use std::hash::{Hash, Hasher}; @@ -43,6 +42,29 @@ macro_rules! float_alloc { }}; } +pub fn header_offset_from_payload() -> usize { + let payload_offset = mem::offset_of!(TypedAllocSlab, payload); + let slab_offset = mem::offset_of!(TypedAllocSlab, slab); + let header_offset = slab_offset + mem::offset_of!(AllocSlab, header); + + debug_assert!(payload_offset > header_offset); + payload_offset - header_offset +} + +pub fn ptr_to_allocated(slab: &mut AllocSlab) -> TypedArenaPtr { + let typed_slab: &mut TypedAllocSlab = unsafe { mem::transmute(slab) }; + typed_slab.to_typed_arena_ptr() +} + +#[macro_export] +macro_rules! gen_ptr_to_allocated { + ($payload: ty) => { + fn ptr_to_allocated(slab: &mut AllocSlab) -> TypedArenaPtr<$payload> { + ptr_to_allocated::<$payload>(slab) + } + }; +} + use std::sync::Arc; use std::sync::Mutex; use std::sync::Weak; @@ -196,6 +218,7 @@ pub enum ArenaHeaderTag { #[bitfield] #[derive(Copy, Clone, Debug)] pub struct ArenaHeader { + #[allow(dead_code)] size: B56, m: bool, tag: ArenaHeaderTag, @@ -291,16 +314,12 @@ impl TypedArenaPtr { #[inline] pub fn header_ptr(&self) -> *const ArenaHeader { - let mut ptr = self.as_ptr() as *const u8 as usize; - ptr -= T::header_offset_from_payload(); // mem::size_of::<*const ArenaHeader>(); - ptr as *const ArenaHeader + unsafe { self.as_ptr().byte_sub(T::header_offset_from_payload()) as *const _ } } #[inline] fn header_ptr_mut(&mut self) -> *mut ArenaHeader { - let mut ptr = self.as_ptr() as *const u8 as usize; - ptr -= T::header_offset_from_payload(); // mem::size_of::<*const ArenaHeader>(); - ptr as *mut ArenaHeader + unsafe { self.as_ptr().byte_sub(T::header_offset_from_payload()) as *mut _ } } #[inline] @@ -339,35 +358,31 @@ pub trait ArenaAllocated: Sized { type PtrToAllocated; fn tag() -> ArenaHeaderTag; - fn size(&self) -> usize; - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated; + fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated; fn header_offset_from_payload() -> usize { - mem::size_of::() + header_offset_from_payload::() } #[allow(clippy::missing_safety_doc)] - unsafe fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { - let size = value.size() + mem::size_of::(); - - #[cfg(target_pointer_width = "32")] - let align = mem::align_of::() * 2; - - #[cfg(target_pointer_width = "64")] - let align = mem::align_of::(); - let layout = alloc::Layout::from_size_align_unchecked(size, align); - - let slab = alloc::alloc(layout) as *mut AllocSlab; + fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + let size = mem::size_of::>(); + let slab = Box::new(TypedAllocSlab { + slab: AllocSlab { + next: arena.base.take(), + #[cfg(target_pointer_width = "32")] + _padding: 0, + header: ArenaHeader::build_with(size as u64, Self::tag()), + }, + payload: value, + }); - (*slab).next = arena.base; - (*slab).header = ArenaHeader::build_with(value.size() as u64, Self::tag()); + let mut untyped_slab = unsafe { Box::from_raw(Box::into_raw(slab) as *mut AllocSlab) }; + let allocated_ptr = Self::ptr_to_allocated(untyped_slab.as_mut()); - let offset = (*slab).payload_offset(); - let result = value.copy_to_arena(offset); + arena.base = Some(untyped_slab); - arena.base = slab; - - result + allocated_ptr } } @@ -505,141 +520,69 @@ impl fmt::Display for F64Offset { impl ArenaAllocated for Integer { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(Integer); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Integer } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for Rational { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(Rational); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Rational } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for LiveLoadState { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(LiveLoadState); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::LiveLoadState } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for TcpListener { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(TcpListener); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::TcpListener } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } #[cfg(feature = "http")] impl ArenaAllocated for HttpListener { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(HttpListener); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpListener } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } #[cfg(feature = "http")] impl ArenaAllocated for HttpResponse { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(HttpResponse); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpResponse } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for IndexPtr { @@ -651,17 +594,8 @@ impl ArenaAllocated for IndexPtr { } #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } + fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated { + TypedArenaPtr::new(unsafe { mem::transmute(slab.header) }) } #[inline] @@ -669,38 +603,48 @@ impl ArenaAllocated for IndexPtr { 0 } - unsafe fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { - let size = mem::size_of::(); - - let align = mem::align_of::(); - let layout = alloc::Layout::from_size_align_unchecked(size, align); - - let slab = alloc::alloc(layout) as *mut AllocSlab; - - (*slab).next = arena.base; - - let result = value.copy_to_arena( - &(*slab).header as *const crate::arena::ArenaHeader - as *mut crate::machine::machine_indices::IndexPtr, - ); - arena.base = slab; - - result + #[inline] + fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + let mut slab = Box::new(AllocSlab { + next: arena.base.take(), + #[cfg(target_pointer_width = "32")] + padding: 0, + header: unsafe { mem::transmute(value) }, + }); + + let allocated_ptr = + TypedArenaPtr::new(unsafe { mem::transmute(ptr::addr_of_mut!(slab.header)) }); + arena.base = Some(slab); + allocated_ptr } } #[repr(C)] -#[derive(Clone, Copy, Debug)] -struct AllocSlab { - next: *mut AllocSlab, +#[derive(Clone, Debug)] +pub struct AllocSlab { + next: Option>, #[cfg(target_pointer_width = "32")] _padding: u32, header: ArenaHeader, } +#[repr(C)] +#[derive(Clone, Debug)] +pub struct TypedAllocSlab { + slab: AllocSlab, + payload: Payload, +} + +impl TypedAllocSlab { + #[inline] + pub fn to_typed_arena_ptr(&mut self) -> TypedArenaPtr { + TypedArenaPtr::new(&mut self.payload as *mut _) + } +} + #[derive(Debug)] pub struct Arena { - base: *mut AllocSlab, + base: Option>, pub f64_tbl: Arc, } @@ -712,72 +656,77 @@ impl Arena { #[inline] pub fn new() -> Self { Arena { - base: ptr::null_mut(), + base: None, f64_tbl: F64Table::new(), } } } unsafe fn drop_slab_in_place(value: &mut AllocSlab) { - use crate::parser::char_reader::CharReader; + macro_rules! drop_typed_slab_in_place { + ($payload: ty, $value: expr) => { + let slab: &mut TypedAllocSlab<$payload> = mem::transmute($value); + ptr::drop_in_place(ptr::addr_of_mut!(slab.payload)); + }; + } match value.header.tag() { ArenaHeaderTag::Integer => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(Integer, value); } ArenaHeaderTag::Rational => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(Rational, value); } ArenaHeaderTag::InputFileStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(InputFileStream, value); } ArenaHeaderTag::OutputFileStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(OutputFileStream, value); } ArenaHeaderTag::NamedTcpStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(NamedTcpStream, value); } ArenaHeaderTag::NamedTlsStream => { #[cfg(feature = "tls")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(NamedTlsStream, value); } ArenaHeaderTag::HttpReadStream => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(HttpReadStream, value); } ArenaHeaderTag::HttpWriteStream => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(HttpWriteStream, value); } ArenaHeaderTag::ReadlineStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(ReadlineStream, value); } ArenaHeaderTag::StaticStringStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StaticStringStream, value); } ArenaHeaderTag::ByteStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(ByteStream, value); } ArenaHeaderTag::LiveLoadState | ArenaHeaderTag::InactiveLoadState => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(LiveLoadState, value); } ArenaHeaderTag::Dropped => {} ArenaHeaderTag::TcpListener => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(TcpListener, value); } ArenaHeaderTag::HttpListener => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(HttpListener, value); } ArenaHeaderTag::HttpResponse => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(HttpResponse, value); } ArenaHeaderTag::StandardOutputStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StandardOutputStream, value); } ArenaHeaderTag::StandardErrorStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StandardErrorStream, value); } ArenaHeaderTag::NullStream | ArenaHeaderTag::IndexPtrUndefined @@ -789,44 +738,20 @@ unsafe fn drop_slab_in_place(value: &mut AllocSlab) { impl Drop for Arena { fn drop(&mut self) { - let mut ptr = self.base; + let mut ptr = self.base.take(); - while !ptr.is_null() { + while let Some(mut slab) = ptr { unsafe { - let ptr_r = &*ptr; - - let layout = alloc::Layout::from_size_align_unchecked( - ptr_r.slab_size(), - mem::align_of::(), - ); - - drop_slab_in_place(&mut *ptr); - - let next_ptr = ptr_r.next; - alloc::dealloc(ptr as *mut u8, layout); - ptr = next_ptr; + drop_slab_in_place(&mut slab); + ptr = slab.next; } } - self.base = ptr::null_mut(); + self.base = None; } } const_assert!(mem::size_of::() == 16); - -impl AllocSlab { - #[inline] - fn slab_size(&self) -> usize { - self.header.size() as usize + mem::size_of::() - } - - fn payload_offset(&self) -> *mut T { - // This looks really scary, should this method be marked as unsafe? - // Also, this seems to cause UB. - unsafe { (self as *const AllocSlab).add(1) as *mut T } - } -} - const_assert!(mem::size_of::>() == 8); #[cfg(test)] diff --git a/src/machine/streams.rs b/src/machine/streams.rs index ba3659dd..c2adb47e 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -455,25 +455,12 @@ macro_rules! arena_allocated_impl_for_stream { impl ArenaAllocated for StreamLayout<$stream_type> { type PtrToAllocated = TypedArenaPtr>; + gen_ptr_to_allocated!(StreamLayout<$stream_type>); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::$stream_tag } - - #[inline] - fn size(&self) -> usize { - mem::size_of::>() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - // Miri seems to hit this a lot - ptr::write(dst, self); - TypedArenaPtr::new(dst as *mut Self) - } - } } }; } -- 2.54.0