From dd2548453b623de0ee6a5b100c352c5efc1835bb Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bennet=20Ble=C3=9Fmann?= Date: Fri, 5 Jul 2024 23:45:02 +0200 Subject: [PATCH] rework some unsafe parts - removed some unsafe - added some safety comments - add explicit types to transmute calls - reworked UntypedArenaPtr -> TypedArenaPtr conversion might help with mthom/scryer-prolog#2438, I noticed fewer complains from miri after changing the default impl for `ArenaAllocated::alloc` --- src/arena.rs | 126 ++++++++++++++++----------------- src/machine/machine_indices.rs | 2 +- src/machine/streams.rs | 30 ++++---- src/macros.rs | 53 +++++--------- src/types.rs | 7 ++ 5 files changed, 102 insertions(+), 116 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 7bae991c..b65dec71 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -7,12 +7,14 @@ use crate::raw_block::*; use crate::rcu::Rcu; use crate::rcu::RcuRef; use crate::read::*; +use crate::types::UntypedArenaPtr; use crate::parser::dashu::{Integer, Rational}; use ordered_float::OrderedFloat; use std::cell::UnsafeCell; use std::fmt; +use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::mem; use std::net::TcpListener; @@ -45,20 +47,6 @@ pub fn header_offset_from_payload() -> usize { 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; @@ -294,10 +282,11 @@ impl fmt::Display for TypedArenaPtr { } impl TypedArenaPtr { - // data must be allocated in the arena already. - #[allow(clippy::not_unsafe_ptr_arg_deref)] + /// # Safety + /// - the pointers referee type is correct, safe code depends on the correctness of the type argument + /// - the pointer is allocated in the arena #[inline] - pub const fn new(data: *mut T) -> Self { + pub const unsafe fn new(data: *mut T) -> Self { unsafe { TypedArenaPtr(ptr::NonNull::new_unchecked(data)) } } @@ -349,30 +338,38 @@ impl TypedArenaPtr { } pub trait ArenaAllocated: Sized { - type PtrToAllocated; - fn tag() -> ArenaHeaderTag; - fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated; fn header_offset_from_payload() -> usize { header_offset_from_payload::() } + /// # Safety + /// - the caller must guarantee that the pointee type of UntypedArenaPtr is Self + unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr { + // safety: + // - allocated in an arena as from an UntypedArenaPtr + // - caller guarantees the type is correct + unsafe { TypedArenaPtr::new(ptr.payload_offset().cast_mut().cast::()) } + } + #[allow(clippy::missing_safety_doc)] - fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { let size = mem::size_of::>(); - let slab = Box::new(TypedAllocSlab { + let mut 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()), + header: HeaderOrIdxPtr { + header: ArenaHeader::build_with(size as u64, Self::tag()), + }, }, payload: value, }); - 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 allocated_ptr = slab.to_typed_arena_ptr(); + let untyped_slab = unsafe { Box::from_raw(Box::into_raw(slab) as *mut AllocSlab) }; arena.base = Some(untyped_slab); @@ -512,10 +509,6 @@ impl fmt::Display for F64Offset { } impl ArenaAllocated for Integer { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(Integer); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Integer @@ -523,10 +516,6 @@ impl ArenaAllocated for Integer { } impl ArenaAllocated for Rational { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(Rational); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Rational @@ -534,10 +523,6 @@ impl ArenaAllocated for Rational { } impl ArenaAllocated for LiveLoadState { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(LiveLoadState); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::LiveLoadState @@ -545,10 +530,6 @@ impl ArenaAllocated for LiveLoadState { } impl ArenaAllocated for TcpListener { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(TcpListener); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::TcpListener @@ -557,10 +538,6 @@ impl ArenaAllocated for TcpListener { #[cfg(feature = "http")] impl ArenaAllocated for HttpListener { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(HttpListener); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpListener @@ -569,10 +546,6 @@ impl ArenaAllocated for HttpListener { #[cfg(feature = "http")] impl ArenaAllocated for HttpResponse { - type PtrToAllocated = TypedArenaPtr; - - gen_ptr_to_allocated!(HttpResponse); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpResponse @@ -580,46 +553,71 @@ impl ArenaAllocated for HttpResponse { } impl ArenaAllocated for IndexPtr { - type PtrToAllocated = TypedArenaPtr; - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::IndexPtrUndefined } - #[inline] - fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated { - TypedArenaPtr::new(ptr::addr_of_mut!(slab.header) as *mut _) - } - #[inline] fn header_offset_from_payload() -> usize { 0 } + /// # Safety + /// - the caller must guarantee that the pointee type of UntypedArenaPtr is T + unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr { + unsafe { TypedArenaPtr::new(std::mem::transmute::<_, *mut IndexPtr>(ptr.get_ptr())) } + } + #[inline] - fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { let mut slab = Box::new(AllocSlab { next: arena.base.take(), #[cfg(target_pointer_width = "32")] _padding: 0, - header: unsafe { mem::transmute(value) }, + header: HeaderOrIdxPtr { idx_ptr: value }, }); - let allocated_ptr = - TypedArenaPtr::new(unsafe { mem::transmute(ptr::addr_of_mut!(slab.header)) }); + let allocated_ptr = unsafe { TypedArenaPtr::new(ptr::addr_of_mut!(slab.header.idx_ptr)) }; arena.base = Some(slab); allocated_ptr } } +#[repr(C)] +union HeaderOrIdxPtr { + header: ArenaHeader, + idx_ptr: IndexPtr, +} + +const _: () = { + if std::mem::size_of::() != std::mem::size_of::() { + panic!("Size of ArenaHeader != IndexPtr") + } +}; + +impl Debug for HeaderOrIdxPtr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + unsafe { &self.header }.fmt(f) + } +} + +impl Clone for HeaderOrIdxPtr { + fn clone(&self) -> Self { + // safety: + // - we created the pointer from a valid reference + // - both ArenaHeader and IndexPtr are plain old datatypes, i.e. no managed resources that need to be cloned + unsafe { std::ptr::read(self) } + } +} + #[repr(C)] #[derive(Clone, Debug)] pub struct AllocSlab { next: Option>, #[cfg(target_pointer_width = "32")] _padding: u32, - header: ArenaHeader, + header: HeaderOrIdxPtr, } #[repr(C)] @@ -632,7 +630,9 @@ pub struct TypedAllocSlab { impl TypedAllocSlab { #[inline] pub fn to_typed_arena_ptr(&mut self) -> TypedArenaPtr { - TypedArenaPtr::new(&mut self.payload as *mut _) + // safety: + // - this is the arena allocation of corresponding type + unsafe { TypedArenaPtr::new(&mut self.payload) } } } @@ -664,7 +664,7 @@ unsafe fn drop_slab_in_place(value: &mut AllocSlab) { }; } - match value.header.tag() { + match value.header.header.tag() { ArenaHeaderTag::Integer => { drop_typed_slab_in_place!(Integer, value); } diff --git a/src/machine/machine_indices.rs b/src/machine/machine_indices.rs index 856c7766..be47f4da 100644 --- a/src/machine/machine_indices.rs +++ b/src/machine/machine_indices.rs @@ -160,7 +160,7 @@ impl From for UntypedArenaPtr { impl From for CodeIndex { #[inline(always)] fn from(ptr: UntypedArenaPtr) -> CodeIndex { - CodeIndex(TypedArenaPtr::new(ptr.get_ptr() as *mut IndexPtr)) + CodeIndex(unsafe { ptr.as_typed_ptr() }) } } diff --git a/src/machine/streams.rs b/src/machine/streams.rs index be5591cf..aec6464a 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -452,10 +452,6 @@ impl DerefMut for StreamLayout { macro_rules! arena_allocated_impl_for_stream { ($stream_type:ty, $stream_tag:ident) => { impl ArenaAllocated for StreamLayout<$stream_type> { - type PtrToAllocated = TypedArenaPtr>; - - gen_ptr_to_allocated!(StreamLayout<$stream_type>); - #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::$stream_tag @@ -539,29 +535,27 @@ impl Stream { )) } - pub fn from_tag(tag: ArenaHeaderTag, ptr: *const u8) -> Self { + pub fn from_tag(tag: ArenaHeaderTag, ptr: UntypedArenaPtr) -> Self { match tag { - ArenaHeaderTag::ByteStream => Stream::Byte(TypedArenaPtr::new(ptr as *mut _)), - ArenaHeaderTag::InputFileStream => Stream::InputFile(TypedArenaPtr::new(ptr as *mut _)), - ArenaHeaderTag::OutputFileStream => { - Stream::OutputFile(TypedArenaPtr::new(ptr as *mut _)) - } - ArenaHeaderTag::NamedTcpStream => Stream::NamedTcp(TypedArenaPtr::new(ptr as *mut _)), + ArenaHeaderTag::ByteStream => Stream::Byte(unsafe { ptr.as_typed_ptr() }), + ArenaHeaderTag::InputFileStream => Stream::InputFile(unsafe { ptr.as_typed_ptr() }), + ArenaHeaderTag::OutputFileStream => Stream::OutputFile(unsafe { ptr.as_typed_ptr() }), + ArenaHeaderTag::NamedTcpStream => Stream::NamedTcp(unsafe { ptr.as_typed_ptr() }), #[cfg(feature = "tls")] - ArenaHeaderTag::NamedTlsStream => Stream::NamedTls(TypedArenaPtr::new(ptr as *mut _)), + ArenaHeaderTag::NamedTlsStream => Stream::NamedTls(unsafe { ptr.as_typed_ptr() }), #[cfg(feature = "http")] - ArenaHeaderTag::HttpReadStream => Stream::HttpRead(TypedArenaPtr::new(ptr as *mut _)), + ArenaHeaderTag::HttpReadStream => Stream::HttpRead(unsafe { ptr.as_typed_ptr() }), #[cfg(feature = "http")] - ArenaHeaderTag::HttpWriteStream => Stream::HttpWrite(TypedArenaPtr::new(ptr as *mut _)), - ArenaHeaderTag::ReadlineStream => Stream::Readline(TypedArenaPtr::new(ptr as *mut _)), + ArenaHeaderTag::HttpWriteStream => Stream::HttpWrite(unsafe { ptr.as_typed_ptr() }), + ArenaHeaderTag::ReadlineStream => Stream::Readline(unsafe { ptr.as_typed_ptr() }), ArenaHeaderTag::StaticStringStream => { - Stream::StaticString(TypedArenaPtr::new(ptr as *mut _)) + Stream::StaticString(unsafe { ptr.as_typed_ptr() }) } ArenaHeaderTag::StandardOutputStream => { - Stream::StandardOutput(TypedArenaPtr::new(ptr as *mut _)) + Stream::StandardOutput(unsafe { ptr.as_typed_ptr() }) } ArenaHeaderTag::StandardErrorStream => { - Stream::StandardError(TypedArenaPtr::new(ptr as *mut _)) + Stream::StandardError(unsafe { ptr.as_typed_ptr() }) } ArenaHeaderTag::Dropped | ArenaHeaderTag::NullStream => { Stream::Null(StreamOptions::default()) diff --git a/src/macros.rs b/src/macros.rs index 553d6d37..3523cbd4 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -173,13 +173,15 @@ macro_rules! typed_arena_ptr_as_cell { macro_rules! raw_ptr_as_cell { ($ptr:expr) => { // Cell is 64-bit, but raw ptr is 32-bit in 32-bit systems - HeapCellValue::from_raw_ptr_bytes(unsafe { std::mem::transmute($ptr) }) + // TODO use <*{const,mut} _>::addr instead of as when the strict_provenance feature is stable rust-lang/rust#95228 + // we might need <*{const,mut} _>::expose_provenance for strict provenance, dependening on how we recreate a pointer later + HeapCellValue::from_raw_ptr_bytes(($ptr as usize).to_ne_bytes()) }; } macro_rules! untyped_arena_ptr_as_cell { ($ptr:expr) => { - HeapCellValue::from_bytes(unsafe { std::mem::transmute($ptr) }) + HeapCellValue::from_bytes(UntypedArenaPtr::into_bytes($ptr)) }; } @@ -224,86 +226,69 @@ macro_rules! stream_as_cell { macro_rules! cell_as_stream { ($cell:expr) => {{ let ptr = cell_as_untyped_arena_ptr!($cell); - Stream::from_tag(ptr.get_tag(), ptr.payload_offset()) + Stream::from_tag(ptr.get_tag(), ptr) }}; } macro_rules! cell_as_load_state_payload { - ($cell:expr) => { - unsafe { - let ptr = cell_as_untyped_arena_ptr!($cell); - let ptr = std::mem::transmute::<_, *mut LiveLoadState>(ptr.payload_offset()); - - TypedArenaPtr::new(ptr) - } - }; + ($cell:expr) => {{ + let ptr = cell_as_untyped_arena_ptr!($cell); + unsafe { ptr.as_typed_ptr::() } + }}; } macro_rules! match_untyped_arena_ptr_pat_body { ($ptr:ident, Integer, $n:ident, $code:expr) => {{ - let payload_ptr = unsafe { std::mem::transmute::<_, *mut Integer>($ptr.payload_offset()) }; - let $n = TypedArenaPtr::new(payload_ptr); + let $n = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, Rational, $n:ident, $code:expr) => {{ - let payload_ptr = unsafe { std::mem::transmute::<_, *mut Rational>($ptr.payload_offset()) }; - let $n = TypedArenaPtr::new(payload_ptr); + let $n = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, OssifiedOpDir, $n:ident, $code:expr) => {{ - let payload_ptr = - unsafe { std::mem::transmute::<_, *mut OssifiedOpDir>($ptr.payload_offset()) }; - let $n = TypedArenaPtr::new(payload_ptr); + let $n = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, LiveLoadState, $n:ident, $code:expr) => {{ - let payload_ptr = - unsafe { std::mem::transmute::<_, *mut LiveLoadState>($ptr.payload_offset()) }; - let $n = TypedArenaPtr::new(payload_ptr); + let $n = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, Stream, $s:ident, $code:expr) => {{ - let $s = Stream::from_tag($ptr.get_tag(), $ptr.payload_offset()); + let $s = Stream::from_tag($ptr.get_tag(), $ptr); #[allow(unused_braces)] $code }}; ($ptr:ident, TcpListener, $listener:ident, $code:expr) => {{ - let payload_ptr = - unsafe { std::mem::transmute::<_, *mut TcpListener>($ptr.payload_offset()) }; #[allow(unused_mut)] - let mut $listener = TypedArenaPtr::new(payload_ptr); + let mut $listener = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, HttpListener, $listener:ident, $code:expr) => {{ - let payload_ptr = - unsafe { std::mem::transmute::<_, *mut HttpListener>($ptr.payload_offset()) }; #[allow(unused_mut)] - let mut $listener = TypedArenaPtr::new(payload_ptr); + let mut $listener = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, HttpResponse, $listener:ident, $code:expr) => {{ - let payload_ptr = - unsafe { std::mem::transmute::<_, *mut HttpResponse>($ptr.payload_offset()) }; #[allow(unused_mut)] - let mut $listener = TypedArenaPtr::new(payload_ptr); + let mut $listener = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, IndexPtr, $ip:ident, $code:expr) => {{ #[allow(unused_mut)] - let mut $ip = - TypedArenaPtr::new(unsafe { std::mem::transmute::<_, *mut IndexPtr>($ptr.get_ptr()) }); + let mut $ip = unsafe { $ptr.as_typed_ptr::() }; #[allow(unused_braces)] $code }}; ($ptr:ident, $($tags:tt)|+, $s:ident, $code:expr) => {{ - let $s = Stream::from_tag($ptr.get_tag(), $ptr.payload_offset()); + let $s = Stream::from_tag($ptr.get_tag(), $ptr); #[allow(unused_braces)] $code }}; diff --git a/src/types.rs b/src/types.rs index 9e488bca..badf58be 100644 --- a/src/types.rs +++ b/src/types.rs @@ -743,6 +743,13 @@ impl UntypedArenaPtr { unsafe { self.get_ptr().add(mem::size_of::()) } } + /// Safety + /// - this UntypedArenaPtr actuall pointee type is T + #[inline] + pub unsafe fn as_typed_ptr(self) -> TypedArenaPtr { + T::typed_ptr(self) + } + #[inline] pub fn get_mark_bit(self) -> bool { self.m() -- 2.54.0