From a93585080c7fbf01d62c8e93569aeaa439eef538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Fri, 5 Jul 2024 21:29:38 +0200 Subject: [PATCH 01/45] remove remains of num feature commit c41aba6b90d2b29f893394d34a80c774b4506d7a removed the num feature but a few cfgs remained --- src/arithmetic.rs | 40 ++++------------------------------------ 1 file changed, 4 insertions(+), 36 deletions(-) diff --git a/src/arithmetic.rs b/src/arithmetic.rs index 799dfbd6..747ef65d 100644 --- a/src/arithmetic.rs +++ b/src/arithmetic.rs @@ -545,26 +545,8 @@ impl PartialEq for Number { (&Number::Float(n1), Number::Integer(ref n2)) => { n1.eq(&OrderedFloat(n2.to_f64().value())) } - (Number::Integer(ref n1), Number::Rational(ref n2)) => { - #[cfg(feature = "num")] - { - &Rational::from(&**n1) == &**n2 - } - #[cfg(not(feature = "num"))] - { - n1.num_eq(&**n2) - } - } - (Number::Rational(ref n1), Number::Integer(ref n2)) => { - #[cfg(feature = "num")] - { - n1 == &Rational::from(&**n2) - } - #[cfg(not(feature = "num"))] - { - n1.num_eq(&**n2) - } - } + (Number::Integer(ref n1), Number::Rational(ref n2)) => n1.num_eq(&**n2), + (Number::Rational(ref n1), Number::Integer(ref n2)) => n1.num_eq(&**n2), (Number::Rational(ref n1), &Number::Float(n2)) => { OrderedFloat(n1.to_f64().value()).eq(&n2) } @@ -643,24 +625,10 @@ impl Ord for Number { n1.cmp(&OrderedFloat(n2.to_f64().value())) } (&Number::Integer(n1), &Number::Rational(n2)) => { - #[cfg(feature = "num")] - { - Rational::from(&**n1).cmp(n2) - } - #[cfg(not(feature = "num"))] - { - (*n1).num_partial_cmp(&*n2).unwrap_or(Ordering::Less) - } + (*n1).num_partial_cmp(&*n2).unwrap_or(Ordering::Less) } (&Number::Rational(n1), &Number::Integer(n2)) => { - #[cfg(feature = "num")] - { - (&**n1).cmp(&Rational::from(&**n2)) - } - #[cfg(not(feature = "num"))] - { - (*n1).num_partial_cmp(&*n2).unwrap_or(Ordering::Less) - } + (*n1).num_partial_cmp(&*n2).unwrap_or(Ordering::Less) } (&Number::Rational(n1), &Number::Float(n2)) => { OrderedFloat(n1.to_f64().value()).cmp(&n2) From dd2548453b623de0ee6a5b100c352c5efc1835bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Fri, 5 Jul 2024 23:45:02 +0200 Subject: [PATCH 02/45] 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() From 7509cc1a07150c2fc4cacb20c4499c0d70219b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 00:13:46 +0200 Subject: [PATCH 03/45] adjust alignment calculation - I think this used to overallocate when the alignment was already met --- src/atom_table.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/atom_table.rs b/src/atom_table.rs index 6de8f5f9..fbb7793a 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -371,8 +371,7 @@ impl AtomTable { } let size = mem::size_of::() + string.len(); - let align_offset = 8 * mem::align_of::(); - let size = (size & !(align_offset - 1)) + align_offset; + let size = size.next_multiple_of(AtomTable::align()); unsafe { let len_ptr = loop { From b8a3067744978d77d39e3f8b591716e1bc4f9112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 01:21:03 +0200 Subject: [PATCH 04/45] resolve miri error for pstr_iter_tests in atom_table relevant to mthom/scryer-prolog#2438 --- src/atom_table.rs | 49 ++++++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/src/atom_table.rs b/src/atom_table.rs index fbb7793a..f29e40bf 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -8,7 +8,6 @@ use std::hash::{Hash, Hasher}; use std::mem; use std::ops::Deref; use std::ptr; -use std::slice; use std::str; use std::sync::Arc; use std::sync::Mutex; @@ -99,6 +98,12 @@ struct AtomHeader { padding: B13, } +#[repr(C)] +pub struct AtomData { + header: AtomHeader, + data: str, +} + impl AtomHeader { fn build_with(len: u64) -> Self { AtomHeader::new().with_len(len).with_m(false) @@ -177,19 +182,23 @@ impl Atom { } #[inline(always)] - pub fn as_ptr(self) -> Option> { + pub fn as_ptr(self) -> Option> { if self.is_static() { None } else { let atom_table = arc_atom_table().expect("We should only have an Atom while there is an AtomTable"); - unsafe { - AtomTableRef::try_map(atom_table.buf(), |buf| { - (buf as *const u8) - .add((self.index as usize) - (STRINGS.len() << 3)) - .as_ref() - }) - } + + AtomTableRef::try_map(atom_table.inner.active_epoch(), |buf| unsafe { + let ptr = buf + .block + .base + .add((self.index as usize) - (STRINGS.len() << 3)); + // TODO use std::ptr::from_raw_parts instead when feature ptr_metadata is stable rust-lang/rust#81513 + let atom_data = &*(std::ptr::slice_from_raw_parts(ptr, 0) as *const AtomData); + let len = atom_data.header.len(); + Some(&*(std::ptr::slice_from_raw_parts(ptr, len as usize) as *const AtomData)) + }) } } @@ -203,9 +212,8 @@ impl Atom { if self.is_static() { STRINGS[(self.index >> 3) as usize].len() } else { - let ptr = self.as_ptr().unwrap(); - let ptr = ptr.deref() as *const u8 as *const AtomHeader; - unsafe { ptr::read(ptr) }.len() as _ + let len: u64 = self.as_ptr().unwrap().header.len(); + len as usize } } @@ -237,15 +245,7 @@ impl Atom { if self.is_static() { AtomString::Static(STRINGS[(self.index >> 3) as usize]) } else if let Some(ptr) = self.as_ptr() { - AtomString::Dynamic(AtomTableRef::map(ptr, |ptr| { - let header = - // Miri seems to hit this line a lot - unsafe { ptr::read::(ptr as *const u8 as *const AtomHeader) }; - let len = header.len() as usize; - let buf = unsafe { (ptr as *const u8).add(mem::size_of::()) }; - - unsafe { str::from_utf8_unchecked(slice::from_raw_parts(buf, len)) } - })) + AtomString::Dynamic(AtomTableRef::map(ptr, |ptr| &ptr.data)) } else { AtomString::Static(STRINGS[(self.index >> 3) as usize]) } @@ -335,13 +335,6 @@ impl AtomTable { } } - #[inline] - pub fn buf(&self) -> AtomTableRef { - AtomTableRef::::map(self.inner.active_epoch(), |inner| { - unsafe { inner.block.base.as_ref() }.unwrap() - }) - } - pub fn active_table(&self) -> RcuRef, IndexSet> { self.inner.active_epoch().table.active_epoch() } From 3de0a4e2adc000da04a923439a05381c18f9f030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 01:38:13 +0200 Subject: [PATCH 05/45] replace transmut with pointer cast calls --- src/arena.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/arena.rs b/src/arena.rs index b65dec71..b9f2ae78 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -566,7 +566,7 @@ impl ArenaAllocated for IndexPtr { /// # 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())) } + unsafe { TypedArenaPtr::new(ptr.get_ptr().cast_mut().cast::()) } } #[inline] From 821358c06228053c3de3d2fcd2498c82dbceb281 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 01:41:05 +0200 Subject: [PATCH 06/45] unify pointer width --- src/macros.rs | 7 ++++--- src/types.rs | 56 ++++++++------------------------------------------- 2 files changed, 12 insertions(+), 51 deletions(-) diff --git a/src/macros.rs b/src/macros.rs index 3523cbd4..54fcaa63 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -171,12 +171,13 @@ macro_rules! typed_arena_ptr_as_cell { } macro_rules! raw_ptr_as_cell { - ($ptr:expr) => { + ($ptr:expr) => {{ // Cell is 64-bit, but raw ptr is 32-bit in 32-bit systems // 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()) - }; + let ptr : *const _ = $ptr; + HeapCellValue::from_ptr_addr(ptr as usize) + }}; } macro_rules! untyped_arena_ptr_as_cell { diff --git a/src/types.rs b/src/types.rs index badf58be..40209ebf 100644 --- a/src/types.rs +++ b/src/types.rs @@ -88,18 +88,10 @@ impl ConsPtr { .with_tag(tag) } - #[cfg(target_pointer_width = "32")] #[inline(always)] pub fn as_ptr(self) -> *mut u8 { - let bytes = self.into_bytes(); - let raw_ptr_bytes = [bytes[1], bytes[2], bytes[3], bytes[4]]; - unsafe { mem::transmute(raw_ptr_bytes) } - } - - #[cfg(target_pointer_width = "64")] - #[inline(always)] - pub fn as_ptr(self) -> *mut u8 { - self.ptr() as *mut _ + let addr: u64 = self.ptr(); + addr as usize as *mut _ } #[inline(always)] @@ -534,37 +526,13 @@ impl HeapCellValue { } } - #[cfg(target_pointer_width = "32")] #[inline] - pub fn from_raw_ptr_bytes(ptr_bytes: [u8; 4]) -> Self { - HeapCellValue::from_bytes([ - ptr_bytes[0], - ptr_bytes[1], - ptr_bytes[2], - ptr_bytes[3], - 0, - 0, - 0, - 0, - ]) - } - #[cfg(target_pointer_width = "64")] - #[inline] - pub fn from_raw_ptr_bytes(ptr_bytes: [u8; 8]) -> Self { - HeapCellValue::from_bytes(ptr_bytes) + pub fn from_ptr_addr(ptr_bytes: usize) -> Self { + HeapCellValue::from_bytes((ptr_bytes as u64).to_ne_bytes()) } - #[inline] - #[cfg(target_pointer_width = "32")] - pub fn to_raw_ptr_bytes(self) -> [u8; 4] { - let bytes = self.into_bytes(); - [bytes[0], bytes[1], bytes[2], bytes[3]] - } - - #[inline] - #[cfg(target_pointer_width = "64")] - pub fn to_raw_ptr_bytes(self) -> [u8; 8] { - self.into_bytes() + pub fn to_ptr_addr(self) -> usize { + u64::from_ne_bytes(self.into_bytes()) as usize } #[inline] @@ -716,18 +684,10 @@ impl UntypedArenaPtr { self.set_m(m); } - #[cfg(target_pointer_width = "32")] #[inline] pub fn get_ptr(self) -> *const u8 { - let bytes = self.into_bytes(); - let raw_ptr_bytes = [bytes[0], bytes[1], bytes[2], bytes[3]]; - unsafe { mem::transmute(raw_ptr_bytes) } - } - - #[cfg(target_pointer_width = "64")] - #[inline] - pub fn get_ptr(self) -> *const u8 { - self.ptr() as *const u8 + let addr: u64 = self.ptr(); + addr as usize as *const u8 } #[inline] From 91253917b4c6d37a0185a7500de90ab725ca006a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 02:24:26 +0200 Subject: [PATCH 07/45] remove rust_beta_channel feature - the msrv (i.e. rust-version in Cargo.toml) is high enough that all gated code can now be used on stabe --- Cargo.toml | 1 - src/arena.rs | 15 ++------------- src/atom_table.rs | 15 ++------------- 3 files changed, 4 insertions(+), 27 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6753ee01..3fe35772 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,6 @@ repl = ["dep:crossterm", "dep:ctrlc", "dep:rustyline"] hostname = ["dep:hostname"] tls = ["dep:native-tls"] http = ["dep:warp", "dep:reqwest"] -rust_beta_channel = [] crypto-full = [] [build-dependencies] diff --git a/src/arena.rs b/src/arena.rs index b9f2ae78..07a144eb 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -56,19 +56,8 @@ const F64_TABLE_ALIGN: usize = 8; #[inline(always)] fn global_f64table() -> &'static RwLock> { - #[cfg(feature = "rust_beta_channel")] - { - // const Weak::new will be stabilized in 1.73 which is currently in beta, - // till then we need a OnceLock for initialization - static GLOBAL_ATOM_TABLE: RwLock> = RwLock::const_new(Weak::new()); - &GLOBAL_ATOM_TABLE - } - #[cfg(not(feature = "rust_beta_channel"))] - { - use std::sync::OnceLock; - static GLOBAL_ATOM_TABLE: OnceLock>> = OnceLock::new(); - GLOBAL_ATOM_TABLE.get_or_init(|| RwLock::new(Weak::new())) - } + static GLOBAL_ATOM_TABLE: RwLock> = RwLock::new(Weak::new()); + &GLOBAL_ATOM_TABLE } impl RawBlockTraits for F64Table { diff --git a/src/atom_table.rs b/src/atom_table.rs index f29e40bf..0f8c4951 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -56,19 +56,8 @@ const ATOM_TABLE_ALIGN: usize = 8; #[inline(always)] fn global_atom_table() -> &'static RwLock> { - #[cfg(feature = "rust_beta_channel")] - { - // const Weak::new will be stabilized in 1.73 which is currently in beta, - // till then we need a OnceLock for initialization - static GLOBAL_ATOM_TABLE: RwLock> = RwLock::const_new(Weak::new()); - &GLOBAL_ATOM_TABLE - } - #[cfg(not(feature = "rust_beta_channel"))] - { - use std::sync::OnceLock; - static GLOBAL_ATOM_TABLE: OnceLock>> = OnceLock::new(); - GLOBAL_ATOM_TABLE.get_or_init(|| RwLock::new(Weak::new())) - } + static GLOBAL_ATOM_TABLE: RwLock> = RwLock::new(Weak::new()); + &GLOBAL_ATOM_TABLE } #[inline(always)] From cba81b4dc7285a391a2b7aa5c781e2adb57c0469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 02:25:58 +0200 Subject: [PATCH 08/45] some test pass miri --- src/heap_iter.rs | 3 --- src/machine/partial_string.rs | 1 - 2 files changed, 4 deletions(-) diff --git a/src/heap_iter.rs b/src/heap_iter.rs index 024262f6..4c1425ab 100644 --- a/src/heap_iter.rs +++ b/src/heap_iter.rs @@ -1758,7 +1758,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on atom_table.rs UB")] fn heap_stackful_iter_tests() { let mut wam = MockWAM::new(); @@ -2351,7 +2350,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on atom_table.rs UB")] fn heap_stackful_post_order_iter() { let mut wam = MockWAM::new(); @@ -2835,7 +2833,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on atom_table.rs UB")] fn heap_stackless_post_order_iter() { let mut wam = MockWAM::new(); diff --git a/src/machine/partial_string.rs b/src/machine/partial_string.rs index 80d86e69..1d6403d7 100644 --- a/src/machine/partial_string.rs +++ b/src/machine/partial_string.rs @@ -801,7 +801,6 @@ mod test { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn pstr_iter_tests() { let mut wam = MockWAM::new(); From 6575b1b57304d8c7c89032f22c0751b83fe591b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 03:41:12 +0200 Subject: [PATCH 09/45] fix some more miri errors probably relevant to mthom/scryer-prolog#2438 --- src/arena.rs | 36 +++++++++++++++++++----------------- src/heap_print.rs | 2 +- src/machine/copier.rs | 1 - src/machine/lib_machine.rs | 2 +- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 07a144eb..9cf74ea8 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -20,6 +20,8 @@ use std::mem; use std::net::TcpListener; use std::ops::{Deref, DerefMut}; use std::ptr; +use std::ptr::addr_of_mut; +use std::ptr::NonNull; use std::sync::RwLock; #[macro_export] @@ -345,7 +347,7 @@ pub trait ArenaAllocated: Sized { #[allow(clippy::missing_safety_doc)] fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { let size = mem::size_of::>(); - let mut slab = Box::new(TypedAllocSlab { + let slab = Box::new(TypedAllocSlab { slab: AllocSlab { next: arena.base.take(), #[cfg(target_pointer_width = "32")] @@ -357,10 +359,10 @@ pub trait ArenaAllocated: Sized { payload: value, }); - let allocated_ptr = slab.to_typed_arena_ptr(); - let untyped_slab = unsafe { Box::from_raw(Box::into_raw(slab) as *mut AllocSlab) }; + let raw_box = Box::into_raw(slab); + let allocated_ptr = TypedAllocSlab::to_typed_arena_ptr(raw_box); - arena.base = Some(untyped_slab); + arena.base = Some(NonNull::new(raw_box.cast::()).unwrap()); allocated_ptr } @@ -568,7 +570,7 @@ impl ArenaAllocated for IndexPtr { }); let allocated_ptr = unsafe { TypedArenaPtr::new(ptr::addr_of_mut!(slab.header.idx_ptr)) }; - arena.base = Some(slab); + arena.base = Some(NonNull::new(Box::into_raw(slab)).unwrap()); allocated_ptr } } @@ -603,7 +605,7 @@ impl Clone for HeaderOrIdxPtr { #[repr(C)] #[derive(Clone, Debug)] pub struct AllocSlab { - next: Option>, + next: Option>, #[cfg(target_pointer_width = "32")] _padding: u32, header: HeaderOrIdxPtr, @@ -618,16 +620,16 @@ pub struct TypedAllocSlab { impl TypedAllocSlab { #[inline] - pub fn to_typed_arena_ptr(&mut self) -> TypedArenaPtr { + pub fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { // safety: // - this is the arena allocation of corresponding type - unsafe { TypedArenaPtr::new(&mut self.payload) } + unsafe { TypedArenaPtr::new(addr_of_mut!((*ptr).payload)) } } } #[derive(Debug)] pub struct Arena { - base: Option>, + base: Option>, pub f64_tbl: Arc, } @@ -645,15 +647,16 @@ impl Arena { } } -unsafe fn drop_slab_in_place(value: &mut AllocSlab) { +unsafe fn drop_slab_in_place(value: NonNull) { macro_rules! drop_typed_slab_in_place { ($payload: ty, $value: expr) => { - let slab: &mut TypedAllocSlab<$payload> = mem::transmute($value); - ptr::drop_in_place(&mut slab.payload); + drop(Box::from_raw( + $value.as_ptr().cast::>(), + )); }; } - match value.header.header.tag() { + match (unsafe { value.as_ref() }).header.header.tag() { ArenaHeaderTag::Integer => { drop_typed_slab_in_place!(Integer, value); } @@ -723,10 +726,10 @@ impl Drop for Arena { fn drop(&mut self) { let mut ptr = self.base.take(); - while let Some(mut slab) = ptr { + while let Some(slab) = ptr { unsafe { - drop_slab_in_place(&mut slab); - ptr = slab.next; + ptr = slab.as_ref().next; + drop_slab_in_place(slab); } } } @@ -814,7 +817,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on arena.rs UB")] fn heap_put_literal_tests() { let mut wam = MockWAM::new(); diff --git a/src/heap_print.rs b/src/heap_print.rs index c844aede..4982b2de 100644 --- a/src/heap_print.rs +++ b/src/heap_print.rs @@ -1841,7 +1841,7 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn term_printing_tests() { let mut wam = MockWAM::new(); diff --git a/src/machine/copier.rs b/src/machine/copier.rs index b64be4a0..c02854ff 100644 --- a/src/machine/copier.rs +++ b/src/machine/copier.rs @@ -398,7 +398,6 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on atom_table.rs UB")] fn copier_tests() { let mut wam = MockWAM::new(); diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index d65fc539..f05b26c2 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -292,7 +292,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore)] + #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn complex_results() { let mut machine = Machine::new_lib(); machine.load_module_string( From 52fa51853e589ea2b49210c261e14d9b110d56a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 03:42:13 +0200 Subject: [PATCH 10/45] don't use env::current_dir() in miri --- src/machine/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index 5326c83c..eee60b8e 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -110,7 +110,11 @@ impl LoadContext { #[inline] fn current_dir() -> PathBuf { + if !cfg!(miri) { env::current_dir().unwrap_or(PathBuf::from("./")) + } else { + PathBuf::from("./") + } } include!(concat!(env!("OUT_DIR"), "/libraries.rs")); From d2d451b1828d2bd51856e5e9d881d87a2e20eedc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 03:44:53 +0200 Subject: [PATCH 11/45] adjust the generation of the LIBRARIES map pre-genrate constants instaed of driectly genrating the literal for the insert --- build/main.rs | 71 +++++++++++++++++++++++++++++++++++----------- src/machine/mod.rs | 10 +++++-- 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/build/main.rs b/build/main.rs index 9bcbac15..bd254c93 100644 --- a/build/main.rs +++ b/build/main.rs @@ -11,34 +11,45 @@ use std::io::Write; use std::path::Path; use std::process::{Command, Stdio}; -fn find_prolog_files(libraries: &mut File, prefix: &str, current_dir: &Path) { +fn find_prolog_files( + libraries: &mut File, + path_prefix: &str, + const_prefix: &str, + current_dir: &Path, +) -> Vec<(String, String)> { + let mut constants = vec![]; + let entries = match current_dir.read_dir() { Ok(entries) => entries, - Err(_) => return, + Err(_) => return constants, }; for entry in entries.filter_map(Result::ok).map(|e| e.path()) { if entry.is_dir() { if let Some(file_name) = entry.file_name() { - let new_prefix = prefix.to_owned() + file_name.to_str().unwrap() + "/"; - find_prolog_files(libraries, &new_prefix, &entry); + let file_name = file_name.to_str().unwrap(); + let new_path_prefix = format!("{path_prefix}{file_name}/"); + let new_const_prefix = format!("{const_prefix}_{}", file_name.to_uppercase()); + let new_consts = + find_prolog_files(libraries, &new_path_prefix, &new_const_prefix, &entry); + constants.extend(new_consts); } } else if entry.is_file() { let ext = std::ffi::OsStr::new("pl"); if entry.extension() == Some(ext) { let contain = String::from_utf8(fs::read(&entry).unwrap()).unwrap(); let name = entry.file_stem().unwrap().to_str().unwrap(); + let lib_name = format!("{path_prefix}{name}"); + let const_name = format!("{const_prefix}_{}", name.to_uppercase()); - let line = format!( - " m.insert(\"{}\",\n{:?});\n", - prefix.to_owned() + name, - contain - ); + writeln!(libraries, "const {const_name}: &str = {contain:?};").unwrap(); - libraries.write_all(line.as_bytes()).unwrap(); + constants.push((lib_name, const_name)); } } } + + constants } fn main() { @@ -58,16 +69,42 @@ fn main() { let mut libraries = File::create(dest_path).unwrap(); let lib_path = Path::new("src/lib"); - libraries - .write_all( - b"ref_thread_local::ref_thread_local! { - pub(crate) static managed LIBRARIES: IndexMap<&'static str, &'static str> = { - let mut m = IndexMap::new();\n", + writeln!( + libraries, + "\ +use indexmap::IndexMap;\ + " + ) + .unwrap(); + + let constants = find_prolog_files(&mut libraries, "", "LIB", lib_path); + + writeln!( + libraries, + "\ +ref_thread_local::ref_thread_local! {{ + pub(crate) static managed LIBRARIES: IndexMap<&'static str, &'static str> = {{ + let mut m = IndexMap::new();" + ) + .unwrap(); + + for (name, constant) in constants { + writeln!( + libraries, + "\ + m.insert(\"{name}\",{constant});" ) .unwrap(); + } - find_prolog_files(&mut libraries, "", lib_path); - libraries.write_all(b"\n m\n };\n}\n").unwrap(); + writeln!( + libraries, + " + m + }}; +}}" + ) + .unwrap(); let instructions_path = Path::new(&out_dir).join("instructions.rs"); let mut instructions_file = File::create(&instructions_path).unwrap(); diff --git a/src/machine/mod.rs b/src/machine/mod.rs index eee60b8e..d3d22953 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -111,13 +111,17 @@ impl LoadContext { #[inline] fn current_dir() -> PathBuf { if !cfg!(miri) { - env::current_dir().unwrap_or(PathBuf::from("./")) + env::current_dir().unwrap_or(PathBuf::from("./")) } else { PathBuf::from("./") } } -include!(concat!(env!("OUT_DIR"), "/libraries.rs")); +mod libraries { + include!(concat!(env!("OUT_DIR"), "/libraries.rs")); +} + +pub(crate) use libraries::LIBRARIES; pub static BREAK_FROM_DISPATCH_LOOP_LOC: usize = 0; pub static INSTALL_VERIFY_ATTR_INTERRUPT: usize = 1; @@ -492,7 +496,7 @@ impl Machine { bootstrapping_compile( Stream::from_static_string( - LIBRARIES.borrow()["ops_and_meta_predicates"], + libraries::LIBRARIES.borrow()["ops_and_meta_predicates"], &mut wam.machine_st.arena, ), &mut wam, From ce40f8f10c2ddc6d25d6d9041a048da7dd73a722 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 04:01:41 +0200 Subject: [PATCH 12/45] update ignore reason --- src/machine/lib_machine.rs | 16 ++++++++-------- src/machine/stack.rs | 2 +- src/parser/char_reader.rs | 3 --- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index f05b26c2..938a5597 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -238,7 +238,7 @@ mod tests { use crate::machine::{QueryMatch, QueryResolution, Value}; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn programatic_query() { let mut machine = Machine::new_lib(); @@ -278,7 +278,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn failing_query() { let mut machine = Machine::new_lib(); let query = String::from(r#"triple("a",P,"b")."#); @@ -292,7 +292,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn complex_results() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -349,7 +349,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn empty_predicate() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -365,7 +365,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn list_results() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -394,7 +394,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn consult() { let mut machine = Machine::new_lib(); @@ -453,7 +453,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn integration_test() { let mut machine = Machine::new_lib(); @@ -500,7 +500,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn findall() { let mut machine = Machine::new_lib(); diff --git a/src/machine/stack.rs b/src/machine/stack.rs index 12aba456..604a52be 100644 --- a/src/machine/stack.rs +++ b/src/machine/stack.rs @@ -281,7 +281,7 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore)] + #[cfg_attr(miri, ignore = "blocked on stack.rs UB")] fn stack_tests() { let mut wam = MockWAM::new(); diff --git a/src/parser/char_reader.rs b/src/parser/char_reader.rs index bc9c1601..da746e6c 100644 --- a/src/parser/char_reader.rs +++ b/src/parser/char_reader.rs @@ -379,7 +379,6 @@ mod tests { use std::io::Cursor; #[test] - #[cfg_attr(miri, ignore = "slow and not very relevant")] fn plain_string() { let mut read_string = CharReader::new(Cursor::new("a string")); @@ -392,7 +391,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "slow and not very relevant")] fn greek_string() { let mut read_string = CharReader::new(Cursor::new("λέξη")); @@ -405,7 +403,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "slow and not very relevant")] fn russian_string() { let mut read_string = CharReader::new(Cursor::new("слово")); From 467a82fe461ddb3c86b30d5d2ce52c9ae38e3928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 04:18:02 +0200 Subject: [PATCH 13/45] run rustfmt --- src/ffi.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/ffi.rs b/src/ffi.rs index 418accce..3782966b 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -436,18 +436,20 @@ impl ForeignFunctionTable { } libffi::raw::FFI_TYPE_SINT64 => read_and_push_int!(i64), libffi::raw::FFI_TYPE_POINTER => read_and_push_int!(i64), - libffi::raw::FFI_TYPE_FLOAT => { - field_ptr = field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); - let n = std::ptr::read(field_ptr as *mut f32); - returns.push(Value::Float(f32::from(n).into())); - field_ptr = field_ptr.add(std::mem::size_of::()); - } - libffi::raw::FFI_TYPE_DOUBLE => { - field_ptr = field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); - let n = std::ptr::read(field_ptr as *mut f64); - returns.push(Value::Float(f64::from(n))); - field_ptr = field_ptr.add(std::mem::size_of::()); - } + libffi::raw::FFI_TYPE_FLOAT => { + field_ptr = + field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); + let n = std::ptr::read(field_ptr as *mut f32); + returns.push(Value::Float(f32::from(n).into())); + field_ptr = field_ptr.add(std::mem::size_of::()); + } + libffi::raw::FFI_TYPE_DOUBLE => { + field_ptr = + field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); + let n = std::ptr::read(field_ptr as *mut f64); + returns.push(Value::Float(f64::from(n))); + field_ptr = field_ptr.add(std::mem::size_of::()); + } libffi::raw::FFI_TYPE_STRUCT => { let substruct = struct_type.atom_fields[i].as_str(); let struct_type = self From ceac9c9095a5384e017325cc7a7dd959b892434e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:01:19 +0200 Subject: [PATCH 14/45] clippy: ignore unused fields on PrologBenchmark struct --- benches/setup.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/benches/setup.rs b/benches/setup.rs index b9c95c74..a61e0c63 100644 --- a/benches/setup.rs +++ b/benches/setup.rs @@ -51,6 +51,7 @@ pub enum Strategy { Reuse, } +#[allow(dead_code)] pub struct PrologBenchmark { pub name: &'static str, pub filename: &'static str, From 1b16c6c73f265fab90b76a1988e5a4a3d20d3d80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:03:14 +0200 Subject: [PATCH 15/45] clippy: use clone_from rather than clone --- src/machine/lib_machine.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index 938a5597..27909c8a 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -191,7 +191,7 @@ impl Machine { printer.quoted = true; printer.max_depth = 1000; // NOTE: set this to 0 for unbounded depth printer.double_quotes = true; - printer.var_names = var_names.clone(); + printer.var_names.clone_from(&var_names); let outputter = printer.print(); @@ -488,12 +488,8 @@ mod tests { } else if let Some(result) = block.strip_prefix("result") { i += 1; if let Some(Ok(ref last_result)) = last_result { - println!( - "\n\n=====Result No. {}=======\n{}\n===============", - i, - last_result.to_string().trim() - ); - assert_eq!(last_result.to_string().trim(), result.to_string().trim(),) + println!("\n\n=====Result No. {i}=======\n{last_result}\n==============="); + assert_eq!(last_result.to_string(), result.to_string().trim(),) } } } From 1ff995fedba7a2ebcf70a318ddc9572857ea83ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:04:55 +0200 Subject: [PATCH 16/45] clippy: allow new without default --- src/arena.rs | 2 ++ src/arithmetic.rs | 2 ++ src/atom_table.rs | 2 ++ src/heap_iter.rs | 2 ++ src/machine/machine_indices.rs | 2 ++ src/parser/ast.rs | 2 ++ src/types.rs | 2 ++ 7 files changed, 14 insertions(+) diff --git a/src/arena.rs b/src/arena.rs index 9cf74ea8..bf78f39c 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + #[cfg(feature = "http")] use crate::http::{HttpListener, HttpResponse}; use crate::machine::loader::LiveLoadState; diff --git a/src/arithmetic.rs b/src/arithmetic.rs index 747ef65d..6b595c36 100644 --- a/src/arithmetic.rs +++ b/src/arithmetic.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + use crate::allocator::*; use crate::arena::*; use crate::atom_table::*; diff --git a/src/atom_table.rs b/src/atom_table.rs index 0f8c4951..5216f6a3 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + use crate::parser::ast::MAX_ARITY; use crate::raw_block::*; use crate::rcu::{Rcu, RcuRef}; diff --git a/src/heap_iter.rs b/src/heap_iter.rs index 4c1425ab..61fde0eb 100644 --- a/src/heap_iter.rs +++ b/src/heap_iter.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + #[cfg(test)] pub(crate) use crate::machine::gc::StacklessPreOrderHeapIter; diff --git a/src/machine/machine_indices.rs b/src/machine/machine_indices.rs index be47f4da..cecdef22 100644 --- a/src/machine/machine_indices.rs +++ b/src/machine/machine_indices.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + use crate::parser::ast::*; use crate::arena::*; diff --git a/src/parser/ast.rs b/src/parser/ast.rs index 7b3022ea..0e98aca8 100644 --- a/src/parser/ast.rs +++ b/src/parser/ast.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + use crate::arena::*; use crate::atom_table::*; use crate::machine::machine_indices::*; diff --git a/src/types.rs b/src/types.rs index 40209ebf..c83fb483 100644 --- a/src/types.rs +++ b/src/types.rs @@ -1,3 +1,5 @@ +#![allow(clippy::new_without_default)] // annotating structs annotated with #[bitfield] doesn't work + use crate::arena::*; use crate::atom_table::*; use crate::forms::*; From d2f236d11640d5972bcba2d5f3c2131e06136511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:06:13 +0200 Subject: [PATCH 17/45] clippy: explicit ptr addrs comparision relevant for wide pointers i.e. pointers with metadata --- src/arena.rs | 2 +- src/rcu.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index bf78f39c..f88973dd 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -227,7 +227,7 @@ impl PartialOrd for TypedArenaPtr { impl PartialEq for TypedArenaPtr { fn eq(&self, other: &TypedArenaPtr) -> bool { - self.0 == other.0 || **self == **other + std::ptr::addr_eq(self.0.as_ptr(), other.0.as_ptr()) || **self == **other } } diff --git a/src/rcu.rs b/src/rcu.rs index a3e8d2ab..75ecef31 100644 --- a/src/rcu.rs +++ b/src/rcu.rs @@ -193,7 +193,7 @@ impl RcuRef { } pub fn ptr_eq(this: &Self, other: &Self) -> bool { - this.data == other.data + std::ptr::addr_eq(this.data.as_ptr(), other.data.as_ptr()) } pub fn clone(this: &Self) -> Self { From 9e7d41025ae73e7c75a2c1bdf4e92a1049f39abb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:06:24 +0200 Subject: [PATCH 18/45] fix saftey comment --- src/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types.rs b/src/types.rs index c83fb483..2872f541 100644 --- a/src/types.rs +++ b/src/types.rs @@ -188,7 +188,7 @@ pub enum TrailRef { BlackboardOffset(Atom, HeapCellValue), // key atom, key value } -#[allow(clippy::enum_variant_names)] +#[allow(clippy::enum_variant_names)] // allow the common "Trailed" prefix #[derive(BitfieldSpecifier, Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] #[bits = 6] pub(crate) enum TrailEntryTag { @@ -705,7 +705,7 @@ impl UntypedArenaPtr { unsafe { self.get_ptr().add(mem::size_of::()) } } - /// Safety + /// # Safety /// - this UntypedArenaPtr actuall pointee type is T #[inline] pub unsafe fn as_typed_ptr(self) -> TypedArenaPtr { From ed57ef3d01fba4752b9074e667dda736d5ac86ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:06:50 +0200 Subject: [PATCH 19/45] clippy: identity conversion --- src/ffi.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ffi.rs b/src/ffi.rs index 3782966b..054f98d9 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -439,15 +439,15 @@ impl ForeignFunctionTable { libffi::raw::FFI_TYPE_FLOAT => { field_ptr = field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); - let n = std::ptr::read(field_ptr as *mut f32); - returns.push(Value::Float(f32::from(n).into())); + let n: f32 = std::ptr::read(field_ptr as *mut f32); + returns.push(Value::Float(n.into())); field_ptr = field_ptr.add(std::mem::size_of::()); } libffi::raw::FFI_TYPE_DOUBLE => { field_ptr = field_ptr.add(field_ptr.align_offset(std::mem::align_of::())); - let n = std::ptr::read(field_ptr as *mut f64); - returns.push(Value::Float(f64::from(n))); + let n: f64 = std::ptr::read(field_ptr as *mut f64); + returns.push(Value::Float(n)); field_ptr = field_ptr.add(std::mem::size_of::()); } libffi::raw::FFI_TYPE_STRUCT => { From 8943962704b5a63abd7ced170df202a0aeba2466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:07:26 +0200 Subject: [PATCH 20/45] clippy: ptr dereference in safe function --- src/arena.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index f88973dd..84e02e84 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -362,7 +362,8 @@ pub trait ArenaAllocated: Sized { }); let raw_box = Box::into_raw(slab); - let allocated_ptr = TypedAllocSlab::to_typed_arena_ptr(raw_box); + // safety: Box::into_raw retuns a pointer to a valid allocation + let allocated_ptr = unsafe { TypedAllocSlab::to_typed_arena_ptr(raw_box) }; arena.base = Some(NonNull::new(raw_box.cast::()).unwrap()); @@ -621,8 +622,10 @@ pub struct TypedAllocSlab { } impl TypedAllocSlab { + /// # Safety + /// - ptr points to a valid allocation of Self #[inline] - pub fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { + pub unsafe fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { // safety: // - this is the arena allocation of corresponding type unsafe { TypedArenaPtr::new(addr_of_mut!((*ptr).payload)) } From 722975d77e9057608ddc3611bdc288d0236030ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:08:59 +0200 Subject: [PATCH 21/45] clippy: use type associated constants --- src/arithmetic.rs | 2 +- src/machine/arithmetic_ops.rs | 35 +++++++++++++++-------------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/arithmetic.rs b/src/arithmetic.rs index 6b595c36..8044f645 100644 --- a/src/arithmetic.rs +++ b/src/arithmetic.rs @@ -168,7 +168,7 @@ fn push_literal(interm: &mut Vec, c: &Literal) -> Result<(), Ari Number::Float(OrderedFloat(std::f64::consts::PI)), )), Literal::Atom(name) if name == &atom!("epsilon") => interm.push(ArithmeticTerm::Number( - Number::Float(OrderedFloat(std::f64::EPSILON)), + Number::Float(OrderedFloat(f64::EPSILON)), )), _ => return Err(ArithmeticError::NonEvaluableFunctor(*c, 0)), } diff --git a/src/machine/arithmetic_ops.rs b/src/machine/arithmetic_ops.rs index 8af9280c..498eb036 100644 --- a/src/machine/arithmetic_ops.rs +++ b/src/machine/arithmetic_ops.rs @@ -641,10 +641,17 @@ pub(crate) fn shr(n1: Number, n2: Number, arena: &mut Arena) -> Result> n2, arena)) } else { - Ok(Number::arena_from(n1_i >> usize::max_value(), arena)) + Ok(Number::arena_from(n1_i >> usize::MAX, arena)) } } (Number::Fixnum(n1), Number::Integer(n2)) => { @@ -654,25 +661,19 @@ pub(crate) fn shr(n1: Number, n2: Number, arena: &mut Arena) -> Result Ok(Number::arena_from(n1 >> n2, arena)), - Err(_) => Ok(Number::arena_from(n1 >> usize::max_value(), arena)), + Err(_) => Ok(Number::arena_from(n1 >> usize::MAX, arena)), } } (Number::Integer(n1), Number::Fixnum(n2)) => match usize::try_from(n2.get_num()) { Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 >> n2), arena)), - _ => Ok(Number::arena_from( - Integer::from(&*n1 >> usize::max_value()), - arena, - )), + _ => Ok(Number::arena_from(Integer::from(&*n1 >> usize::MAX), arena)), }, (Number::Integer(n1), Number::Integer(n2)) => { let result: Result = (&*n2).try_into(); match result { Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 >> n2), arena)), - Err(_) => Ok(Number::arena_from( - Integer::from(&*n1 >> usize::max_value()), - arena, - )), + Err(_) => Ok(Number::arena_from(Integer::from(&*n1 >> usize::MAX), arena)), } } (Number::Integer(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)), @@ -700,7 +701,7 @@ pub(crate) fn shl(n1: Number, n2: Number, arena: &mut Arena) -> Result { @@ -708,22 +709,16 @@ pub(crate) fn shl(n1: Number, n2: Number, arena: &mut Arena) -> Result { Ok(n2) => Ok(Number::arena_from(n1 << n2, arena)), - _ => Ok(Number::arena_from(n1 << usize::max_value(), arena)), + _ => Ok(Number::arena_from(n1 << usize::MAX, arena)), } } (Number::Integer(n1), Number::Fixnum(n2)) => match usize::try_from(n2.get_num()) { Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 << n2), arena)), - _ => Ok(Number::arena_from( - Integer::from(&*n1 << usize::max_value()), - arena, - )), + _ => Ok(Number::arena_from(Integer::from(&*n1 << usize::MAX), arena)), }, (Number::Integer(n1), Number::Integer(n2)) => match (&*n2).try_into() as Result { Ok(n2) => Ok(Number::arena_from(Integer::from(&*n1 << n2), arena)), - _ => Ok(Number::arena_from( - Integer::from(&*n1 << usize::max_value()), - arena, - )), + _ => Ok(Number::arena_from(Integer::from(&*n1 << usize::MAX), arena)), }, (Number::Integer(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)), (Number::Fixnum(_), n2) => Err(numerical_type_error(ValidType::Integer, n2, stub_gen)), From 6386e70584788be025a5f9be20e77c43b3c557e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:28:44 +0200 Subject: [PATCH 22/45] clippy: change ToString impl to Display - write directly to formatter, eliminating intermediate String allocations - take Value by reference eliminating clones - remove trim() called on the result of QueryResolution::to_string - we only emit "true", "false", or "[]" neither of which contains trailing or leading withespace, so the calls was effectively a noop --- src/machine/parsed_results.rs | 99 ++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/src/machine/parsed_results.rs b/src/machine/parsed_results.rs index 9c635f05..681b27e4 100644 --- a/src/machine/parsed_results.rs +++ b/src/machine/parsed_results.rs @@ -3,6 +3,8 @@ use dashu::*; use ordered_float::OrderedFloat; use std::collections::BTreeMap; use std::collections::HashMap; +use std::fmt::Display; +use std::fmt::Write; pub type QueryResult = Result; @@ -13,17 +15,21 @@ pub enum QueryResolution { Matches(Vec), } -pub fn prolog_value_to_json_string(value: Value) -> String { +pub fn write_prolog_value_as_json( + writer: &mut W, + value: &Value, +) -> Result<(), std::fmt::Error> { match value { - Value::Integer(i) => format!("{}", i), - Value::Float(f) => format!("{}", f), - Value::Rational(r) => format!("{}", r), - Value::Atom(a) => format!("{}", a.as_str()), + Value::Integer(i) => write!(writer, "{}", i), + Value::Float(f) => write!(writer, "{}", f), + Value::Rational(r) => write!(writer, "{}", r), + Value::Atom(a) => writer.write_str(&a.as_str()), Value::String(s) => { if let Err(_e) = serde_json::from_str::(s.as_str()) { //treat as string literal //escape double quotes - format!( + write!( + writer, "\"{}\"", s.replace('\"', "\\\"") .replace('\n', "\\n") @@ -32,60 +38,71 @@ pub fn prolog_value_to_json_string(value: Value) -> String { ) } else { //return valid json string - s + writer.write_str(&s) } } Value::List(l) => { - let mut string_result = "[".to_string(); - for (i, v) in l.iter().enumerate() { - if i > 0 { - string_result.push(','); + writer.write_char('[')?; + if let Some((first, rest)) = l.split_first() { + write_prolog_value_as_json(writer, first)?; + + for other in rest { + writer.write_char(',')?; + write_prolog_value_as_json(writer, other)?; } - string_result.push_str(&prolog_value_to_json_string(v.clone())); } - string_result.push(']'); - string_result + writer.write_char(']') } Value::Structure(s, l) => { - let mut string_result = format!("\"{}\":[", s.as_str()); - for (i, v) in l.iter().enumerate() { - if i > 0 { - string_result.push(','); + write!(writer, "\"{}\":[", s.as_str())?; + + if let Some((first, rest)) = l.split_first() { + write_prolog_value_as_json(writer, first)?; + for other in rest { + writer.write_char(',')?; + write_prolog_value_as_json(writer, other)?; } - string_result.push_str(&prolog_value_to_json_string(v.clone())); } - string_result.push(']'); - string_result + writer.write_char(']') } - _ => "null".to_string(), + _ => writer.write_str("null"), } } -fn prolog_match_to_json_string(query_match: &QueryMatch) -> String { - let mut string_result = "{".to_string(); - for (i, (k, v)) in query_match.bindings.iter().enumerate() { - if i > 0 { - string_result.push(','); +fn write_prolog_match_as_json( + writer: &mut W, + query_match: &QueryMatch, +) -> Result<(), std::fmt::Error> { + writer.write_char('{')?; + let mut iter = query_match.bindings.iter(); + + if let Some((k, v)) = iter.next() { + write!(writer, "\"{k}\":")?; + write_prolog_value_as_json(writer, v)?; + + for (k, v) in iter { + write!(writer, ",\"{k}\":")?; + write_prolog_value_as_json(writer, v)?; } - string_result.push_str(&format!( - "\"{}\":{}", - k, - prolog_value_to_json_string(v.clone()) - )); } - string_result.push('}'); - string_result + writer.write_char('}') } -impl ToString for QueryResolution { - fn to_string(&self) -> String { +impl Display for QueryResolution { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - QueryResolution::True => "true".to_string(), - QueryResolution::False => "false".to_string(), + QueryResolution::True => f.write_str("true"), + QueryResolution::False => f.write_str("false"), QueryResolution::Matches(matches) => { - let matches_json: Vec = - matches.iter().map(prolog_match_to_json_string).collect(); - format!("[{}]", matches_json.join(",")) + f.write_char('[')?; + if let Some((first, rest)) = matches.split_first() { + write_prolog_match_as_json(f, first)?; + for other in rest { + f.write_char(',')?; + write_prolog_match_as_json(f, other)?; + } + } + f.write_char(']') } } } From 87b4c9d73686217a1c2280d997e46029dc03c725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 13:41:35 +0200 Subject: [PATCH 23/45] somehow this was never marked as bad --- src/machine/lib_machine.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index 27909c8a..80e0d1c6 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -529,6 +529,7 @@ mod tests { } #[test] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn dont_return_partial_matches() { let mut machine = Machine::new_lib(); @@ -552,6 +553,7 @@ mod tests { } #[test] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn dont_return_partial_matches_without_discountiguous() { let mut machine = Machine::new_lib(); @@ -583,6 +585,7 @@ mod tests { } #[test] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn non_existent_predicate_should_not_cause_panic_when_other_predicates_are_defined() { let mut machine = Machine::new_lib(); @@ -607,6 +610,7 @@ mod tests { } #[test] + #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] fn issue_2341() { let mut machine = Machine::new_lib(); From fcb41542c3a113a99b1821e249e5ffcc9c80c873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 14:01:51 +0200 Subject: [PATCH 24/45] fix stream.rs UB --- src/arena.rs | 34 +++++++++++++++++++--------------- src/machine/arithmetic_ops.rs | 1 - src/machine/gc.rs | 1 - src/machine/mock_wam.rs | 2 -- tests/scryer/issues.rs | 2 +- tests/scryer/src_tests.rs | 20 ++++++++++---------- 6 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 84e02e84..e0cdeef9 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -5,6 +5,7 @@ use crate::http::{HttpListener, HttpResponse}; use crate::machine::loader::LiveLoadState; use crate::machine::machine_indices::*; use crate::machine::streams::*; +use crate::parser::char_reader::CharReader; use crate::raw_block::*; use crate::rcu::Rcu; use crate::rcu::RcuRef; @@ -369,6 +370,12 @@ pub trait ArenaAllocated: Sized { allocated_ptr } + + /// # Safety + /// - ptr points to an allocated slab of the correct kind + unsafe fn dealloc(ptr: NonNull>) { + drop(unsafe { Box::from_raw(ptr.as_ptr()) }); + } } #[derive(Debug)] @@ -655,9 +662,7 @@ impl Arena { unsafe fn drop_slab_in_place(value: NonNull) { macro_rules! drop_typed_slab_in_place { ($payload: ty, $value: expr) => { - drop(Box::from_raw( - $value.as_ptr().cast::>(), - )); + <$payload as ArenaAllocated>::dealloc($value.cast::>()) }; } @@ -669,34 +674,34 @@ unsafe fn drop_slab_in_place(value: NonNull) { drop_typed_slab_in_place!(Rational, value); } ArenaHeaderTag::InputFileStream => { - drop_typed_slab_in_place!(InputFileStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::OutputFileStream => { - drop_typed_slab_in_place!(OutputFileStream, value); + drop_typed_slab_in_place!(StreamLayout, value); } ArenaHeaderTag::NamedTcpStream => { - drop_typed_slab_in_place!(NamedTcpStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::NamedTlsStream => { #[cfg(feature = "tls")] - drop_typed_slab_in_place!(NamedTlsStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::HttpReadStream => { #[cfg(feature = "http")] - drop_typed_slab_in_place!(HttpReadStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::HttpWriteStream => { #[cfg(feature = "http")] - drop_typed_slab_in_place!(HttpWriteStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::ReadlineStream => { - drop_typed_slab_in_place!(ReadlineStream, value); + drop_typed_slab_in_place!(StreamLayout, value); } ArenaHeaderTag::StaticStringStream => { - drop_typed_slab_in_place!(StaticStringStream, value); + drop_typed_slab_in_place!(StreamLayout, value); } ArenaHeaderTag::ByteStream => { - drop_typed_slab_in_place!(ByteStream, value); + drop_typed_slab_in_place!(StreamLayout>, value); } ArenaHeaderTag::LiveLoadState | ArenaHeaderTag::InactiveLoadState => { drop_typed_slab_in_place!(LiveLoadState, value); @@ -714,10 +719,10 @@ unsafe fn drop_slab_in_place(value: NonNull) { drop_typed_slab_in_place!(HttpResponse, value); } ArenaHeaderTag::StandardOutputStream => { - drop_typed_slab_in_place!(StandardOutputStream, value); + drop_typed_slab_in_place!(StreamLayout, value); } ArenaHeaderTag::StandardErrorStream => { - drop_typed_slab_in_place!(StandardErrorStream, value); + drop_typed_slab_in_place!(StreamLayout, value); } ArenaHeaderTag::NullStream | ArenaHeaderTag::IndexPtrUndefined @@ -778,7 +783,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn heap_cell_value_const_cast() { let mut wam = MockWAM::new(); #[cfg(target_pointer_width = "32")] diff --git a/src/machine/arithmetic_ops.rs b/src/machine/arithmetic_ops.rs index 498eb036..0ab17712 100644 --- a/src/machine/arithmetic_ops.rs +++ b/src/machine/arithmetic_ops.rs @@ -1415,7 +1415,6 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn arith_eval_by_metacall_tests() { let mut wam = MachineState::new(); let mut op_dir = default_op_dir(); diff --git a/src/machine/gc.rs b/src/machine/gc.rs index f62231ea..32142e1d 100644 --- a/src/machine/gc.rs +++ b/src/machine/gc.rs @@ -369,7 +369,6 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn heap_marking_tests() { let mut wam = MockWAM::new(); diff --git a/src/machine/mock_wam.rs b/src/machine/mock_wam.rs index 2679be43..0da42e08 100644 --- a/src/machine/mock_wam.rs +++ b/src/machine/mock_wam.rs @@ -260,7 +260,6 @@ mod tests { use super::*; #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn unify_tests() { let mut wam = MachineState::new(); let mut op_dir = default_op_dir(); @@ -482,7 +481,6 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on streams.rs UB")] fn test_unify_with_occurs_check() { let mut wam = MachineState::new(); let mut op_dir = default_op_dir(); diff --git a/tests/scryer/issues.rs b/tests/scryer/issues.rs index 556bf9d5..40e3f5ce 100644 --- a/tests/scryer/issues.rs +++ b/tests/scryer/issues.rs @@ -4,7 +4,7 @@ use serial_test::serial; // issue #831 #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn call_0() { load_module_test( "tests-pl/issue831-call0.pl", diff --git a/tests/scryer/src_tests.rs b/tests/scryer/src_tests.rs index a3d6ddbf..2434cb1f 100644 --- a/tests/scryer/src_tests.rs +++ b/tests/scryer/src_tests.rs @@ -3,35 +3,35 @@ use serial_test::serial; #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn builtins() { load_module_test("src/tests/builtins.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn call_with_inference_limit() { load_module_test("src/tests/call_with_inference_limit.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn facts() { load_module_test("src/tests/facts.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn hello_world() { load_module_test("src/tests/hello_world.pl", "Hello World!\n"); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn syntax_error() { load_module_test( "tests-pl/syntax_error.pl", @@ -41,21 +41,21 @@ fn syntax_error() { #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn predicates() { load_module_test("src/tests/predicates.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn rules() { load_module_test("src/tests/rules.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn setup_call_cleanup_load() { load_module_test( "src/tests/setup_call_cleanup.pl", @@ -65,14 +65,14 @@ fn setup_call_cleanup_load() { #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn clpz_load() { load_module_test("src/tests/clpz/test_clpz.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on streams.rs UB")] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn iso_conformity_tests() { load_module_test("tests-pl/iso-conformity-tests.pl", "All tests passed"); } From d87400afa0fef3edffd6295c747e27b229ee7c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 14:49:19 +0200 Subject: [PATCH 25/45] switch Rcu to the arcu crate The arcu crate is a more general implementation of the Rcu I implemented in here in scryer. It contains some bug-fixes regarding race-conditions in the Rcu update function, which could cause leaks and uses after free. Source of the problem was the Relaxed load/strore/update of the reference count in side the Arc not being properly ordered with other load/stores. --- Cargo.lock | 7 ++ Cargo.toml | 1 + src/arena.rs | 16 ++-- src/atom_table.rs | 44 +++++----- src/lib.rs | 2 - src/rcu.rs | 220 ---------------------------------------------- 6 files changed, 41 insertions(+), 249 deletions(-) delete mode 100644 src/rcu.rs diff --git a/Cargo.lock b/Cargo.lock index 43806baf..eb2bd1b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -108,6 +108,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "arcu" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8727c0fb4c436605c8f11c579ec86edcb729134aec4ee66e454efd99a91859f" + [[package]] name = "arrayvec" version = "0.5.2" @@ -2558,6 +2564,7 @@ dependencies = [ name = "scryer-prolog" version = "0.9.4" dependencies = [ + "arcu", "assert_cmd", "base64 0.12.3", "bit-set", diff --git a/Cargo.toml b/Cargo.toml index 3fe35772..45ba6d6e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,6 +74,7 @@ static_assertions = "1.1.0" serde_json = "1.0.95" serde = "1.0.159" +arcu = { version = "0.1.1", features = ["thread_local_counter"] } [target.'cfg(not(target_arch = "wasm32"))'.dependencies] crossterm = { version = "0.20.0", optional = true } diff --git a/src/arena.rs b/src/arena.rs index e0cdeef9..571595a6 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -7,12 +7,14 @@ use crate::machine::machine_indices::*; use crate::machine::streams::*; use crate::parser::char_reader::CharReader; 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 arcu::atomic::Arcu; +use arcu::epoch_counters::GlobalEpochCounterPool; +use arcu::rcu_ref::RcuRef; +use arcu::Rcu; use ordered_float::OrderedFloat; use std::cell::UnsafeCell; @@ -79,7 +81,7 @@ impl RawBlockTraits for F64Table { #[derive(Debug)] pub struct F64Table { - block: Rcu>, + block: Arcu, GlobalEpochCounterPool>, update: Mutex<()>, } @@ -93,7 +95,7 @@ pub fn lookup_float( .upgrade() .expect("We should only be looking up floats while there is a float table"); - RcuRef::try_map(f64table.block.active_epoch(), |raw_block| unsafe { + RcuRef::try_map(f64table.block.read(), |raw_block| unsafe { raw_block .base .add(offset.0) @@ -118,7 +120,7 @@ impl F64Table { atom_table } else { let atom_table = Arc::new(Self { - block: Rcu::new(RawBlock::new()), + block: Arcu::new(RawBlock::new(), GlobalEpochCounterPool), update: Mutex::new(()), }); *guard = Arc::downgrade(&atom_table); @@ -133,7 +135,7 @@ impl F64Table { // we don't have an index table for lookups as AtomTable does so // just get the epoch after we take the upgrade lock - let mut block_epoch = self.block.active_epoch(); + let mut block_epoch = self.block.read(); let mut ptr; @@ -143,7 +145,7 @@ impl F64Table { if ptr.is_null() { let new_block = block_epoch.grow_new().unwrap(); self.block.replace(new_block); - block_epoch = self.block.active_epoch(); + block_epoch = self.block.read(); } else { break; } diff --git a/src/atom_table.rs b/src/atom_table.rs index 5216f6a3..4a426cd0 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -2,7 +2,6 @@ use crate::parser::ast::MAX_ARITY; use crate::raw_block::*; -use crate::rcu::{Rcu, RcuRef}; use crate::types::*; use std::cmp::Ordering; @@ -16,6 +15,10 @@ use std::sync::Mutex; use std::sync::RwLock; use std::sync::Weak; +use arcu::atomic::Arcu; +use arcu::epoch_counters::GlobalEpochCounterPool; +use arcu::rcu_ref::RcuRef; +use arcu::Rcu; use indexmap::IndexSet; use scryer_modular_bitfield::prelude::*; @@ -180,7 +183,7 @@ impl Atom { let atom_table = arc_atom_table().expect("We should only have an Atom while there is an AtomTable"); - AtomTableRef::try_map(atom_table.inner.active_epoch(), |buf| unsafe { + AtomTableRef::try_map(atom_table.inner.read(), |buf| unsafe { let ptr = buf .block .base @@ -278,17 +281,17 @@ impl Ord for Atom { #[derive(Debug)] pub struct InnerAtomTable { block: RawBlock, - pub table: Rcu>, + pub table: Arcu, GlobalEpochCounterPool>, } #[derive(Debug)] pub struct AtomTable { - inner: Rcu, + inner: Arcu, // this lock is taking during resizing update: Mutex<()>, } -pub type AtomTableRef = RcuRef; +pub type AtomTableRef = arcu::rcu_ref::RcuRef; impl InnerAtomTable { #[inline(always)] @@ -296,7 +299,7 @@ impl InnerAtomTable { STATIC_ATOMS_MAP .get(string) .cloned() - .or_else(|| self.table.active_epoch().get(string).cloned()) + .or_else(|| self.table.read().get(string).cloned()) } } @@ -314,10 +317,13 @@ impl AtomTable { atom_table } else { let atom_table = Arc::new(Self { - inner: Rcu::new(InnerAtomTable { - block: RawBlock::new(), - table: Rcu::new(IndexSet::new()), - }), + inner: Arcu::new( + InnerAtomTable { + block: RawBlock::new(), + table: Arcu::new(IndexSet::new(), GlobalEpochCounterPool), + }, + GlobalEpochCounterPool, + ), update: Mutex::new(()), }); *guard = Arc::downgrade(&atom_table); @@ -327,13 +333,13 @@ impl AtomTable { } pub fn active_table(&self) -> RcuRef, IndexSet> { - self.inner.active_epoch().table.active_epoch() + self.inner.read().table.read() } pub fn build_with(atom_table: &AtomTable, string: &str) -> Atom { loop { - let mut block_epoch = atom_table.inner.active_epoch(); - let mut table_epoch = block_epoch.table.active_epoch(); + let mut block_epoch = atom_table.inner.read(); + let mut table_epoch = block_epoch.table.read(); if let Some(atom) = block_epoch.lookup_str(string) { return atom; @@ -342,10 +348,8 @@ impl AtomTable { // take a lock to prevent concurrent updates let update_guard = atom_table.update.lock().unwrap(); - let is_same_allocation = - RcuRef::same_epoch(&block_epoch, &atom_table.inner.active_epoch()); - let is_same_atom_list = - RcuRef::same_epoch(&table_epoch, &block_epoch.table.active_epoch()); + let is_same_allocation = RcuRef::same_epoch(&block_epoch, &atom_table.inner.read()); + let is_same_atom_list = RcuRef::same_epoch(&table_epoch, &block_epoch.table.read()); if !(is_same_allocation && is_same_atom_list) { // some other thread raced us between our lookup and @@ -364,14 +368,14 @@ impl AtomTable { if ptr.is_null() { // garbage collection would go here let new_block = block_epoch.block.grow_new().unwrap(); - let new_table = Rcu::new(table_epoch.clone()); + let new_table = Arcu::new(table_epoch.clone(), GlobalEpochCounterPool); let new_alloc = InnerAtomTable { block: new_block, table: new_table, }; atom_table.inner.replace(new_alloc); - block_epoch = atom_table.inner.active_epoch(); - table_epoch = block_epoch.table.active_epoch(); + block_epoch = atom_table.inner.read(); + table_epoch = block_epoch.table.read(); } else { break ptr; } diff --git a/src/lib.rs b/src/lib.rs index 56d967eb..90f498a3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,8 +42,6 @@ pub mod types; use instructions::instr; -mod rcu; - #[cfg(target_arch = "wasm32")] use wasm_bindgen::prelude::*; diff --git a/src/rcu.rs b/src/rcu.rs deleted file mode 100644 index 75ecef31..00000000 --- a/src/rcu.rs +++ /dev/null @@ -1,220 +0,0 @@ -use std::{ - cell::OnceCell, - fmt::Debug, - mem::ManuallyDrop, - ops::Deref, - ptr::NonNull, - sync::{ - atomic::{AtomicPtr, AtomicU8}, - Arc, RwLock, Weak, - }, -}; - -// the epoch counters of all threads that have ever accessed an Rcu -// threads that have finished will have a dangling Weak reference and can be cleand up -// having this be shared between all Rcu's is a tradeof, -// writes will be slower as more epoch counters need to be waited for -// reads should be faster as a thread only needs to register itself once on the first read -// -static EPOCH_COUNTERS: RwLock>> = RwLock::new(Vec::new()); - -thread_local! { - // odd value means the current thread is about to access the active_epoch of an Rcu - // a thread has a single epoch counter for all Rcu it accesses, - // as a thread can only access one Rcu at a time - static THREAD_EPOCH_COUNTER: OnceCell> = const { OnceCell::new() }; -} - -pub struct Rcu { - active_value: AtomicPtr, -} - -impl std::fmt::Debug for Rcu { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let active_epoch = self.active_epoch(); - f.debug_struct("Rcu") - .field("active_value", &active_epoch) - .finish() - } -} - -impl Rcu { - pub fn new(initial_value: T) -> Self { - Rcu { - active_value: AtomicPtr::new(Arc::into_raw(Arc::new(initial_value)).cast_mut()), - } - } - - pub fn active_epoch(&self) -> RcuRef { - THREAD_EPOCH_COUNTER.with(|epoch_counter| { - let epoch_counter = epoch_counter.get_or_init(|| { - let epoch_counter = Arc::new(AtomicU8::new(0)); - // register the current threads epoch counter on init - EPOCH_COUNTERS - .write() - .unwrap() - .push(Arc::downgrade(&epoch_counter)); - epoch_counter - }); - - let old = epoch_counter.fetch_add(1, std::sync::atomic::Ordering::AcqRel); - assert!(old % 2 == 0, "Old Epoch counter value should be even!"); - }); - - let arc_ptr = self.active_value.load(std::sync::atomic::Ordering::Acquire); - - let arc = unsafe { - // Safety: - // - the ptr was created in Rcu::new or Rcu::replace with Arc::into_raw - // - the Rcu is responsible for of the arc's strong refrences - // - the Rcu is alive as this function takes a reference to the Rcu - // - replace will wait with decrementing the old values strong count until our epoich counter is even again - Arc::increment_strong_count(arc_ptr); - // Safety: - // - the ptr was created in Rcu::new or Rcu::replace with Arc::into_raw - // - we have just ensured an additional strong count by incrementing the count - Arc::from_raw(arc_ptr) - }; - - THREAD_EPOCH_COUNTER.with(|epoch_counter| { - let old = epoch_counter - .get().expect("we initialized the OnceCell when we incremented the epoch counter the fist time") - .fetch_add(1, std::sync::atomic::Ordering::AcqRel); - assert!(old % 2 != 0, "Old Epoch counter value should be odd!"); - }); - - RcuRef { - data: arc.deref().into(), - arc, - } - } - - /* - * replace the Rcu'S content with a new value - * - * This does not syncronize write and last to update the active_value pointer wins, - * all writes that do not win will be lost, though not leaked. - * This will block untill the old value can be reclaimed, - * i.e. all threads whitnest to be in the read critical sections - * have been witnest to have left the critical section at least once - */ - pub fn replace(&self, new_value: T) { - let arc_ptr = self.active_value.swap( - Arc::into_raw(Arc::new(new_value)).cast_mut(), - std::sync::atomic::Ordering::AcqRel, - ); - - // maually drop as we need to ensure not to drop the arc while - // we have not witnest all threads to be or have been outside the read critical section - // i.e. even epoch counter or different odd epoch counter - // Safety: - // - the ptr was created in Rcu::new or Rcu::replace with Arc::into_raw - // - the Rcu itself holds one strong count - let arc = unsafe { ManuallyDrop::new(Arc::from_raw(arc_ptr)) }; - - let epochs = EPOCH_COUNTERS.read().unwrap().clone(); - let mut epochs = epochs - .into_iter() - .flat_map(|elem| { - let arc = elem.upgrade()?; - let init_val = arc.load(std::sync::atomic::Ordering::Acquire); - if init_val % 2 == 0 { - // already even can be ignored - return None; - } - // odd initial value thread is in read critical section - // need to wait for the value to change before we can drop the arc - Some((init_val, elem)) - }) - .collect::>(); - - while !epochs.is_empty() { - epochs.retain(|elem| { - let Some(arc) = elem.1.upgrade() else { - // as the thread is dead it can't have a ref to old arc - return false; - }; - // the epoch counter has not changed so the thread is still in the same instance of the critical section - // any different value is ok as - // - even values indicate the thread is outside the critical section - // - a diffrent odd value indicates the thread has left the critical section and can subsequently only read the new active_value - arc.load(std::sync::atomic::Ordering::Acquire) == elem.0 - }) - } - - // Safety: - // - we have not dropped the arc another way - // - we witnessed all threads either with an even epoch count or with a new odd count - // as such they must have left the critical section at some point - ManuallyDrop::into_inner(arc); - } -} - -pub struct RcuRef -where - T: ?Sized, - M: ?Sized, -{ - arc: Arc, - data: NonNull, -} - -impl Debug for RcuRef { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("RcuRef") - .field("data", &self.deref()) - .finish() - } -} - -// use assoiated functions rather than methods so that we don't overlap -// with functions of the Deref Target type -impl RcuRef { - pub fn map FnOnce(&'a M) -> &'a N>(referece: Self, f: F) -> RcuRef { - RcuRef { - arc: referece.arc, - data: f(unsafe { referece.data.as_ref() }).into(), - } - } - - pub fn try_map FnOnce(&'a M) -> Option<&'a N>>( - referece: Self, - f: F, - ) -> Option> { - let val = f(unsafe { referece.data.as_ref() })?; - Some(RcuRef { - arc: Arc::clone(&referece.arc), - data: val.into(), - }) - } - - pub fn same_epoch(this: &Self, other: &RcuRef) -> bool { - Arc::ptr_eq(&this.arc, &other.arc) - } - - pub fn ptr_eq(this: &Self, other: &Self) -> bool { - std::ptr::addr_eq(this.data.as_ptr(), other.data.as_ptr()) - } - - pub fn clone(this: &Self) -> Self { - Self { - arc: Arc::clone(&this.arc), - data: this.data, - } - } - - pub fn get_root(this: &Self) -> &T { - &this.arc - } -} - -impl Deref for RcuRef { - type Target = M; - - fn deref(&self) -> &Self::Target { - // Safety: The pointer points into the arc we are holding - // while we are alive so is the target - // as the content is in an Rcu no mutable acess is given out - unsafe { self.data.as_ref() } - } -} From 285f11ccdc18c11dd0a2a91840a7f158fb9a2b5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 15:37:08 +0200 Subject: [PATCH 26/45] add associated Payload type to ArenaAllocated --- src/arena.rs | 120 ++++++++++++++++++++++++------------ src/machine/loader.rs | 2 +- src/machine/streams.rs | 30 ++++----- src/machine/system_calls.rs | 50 +++++++-------- src/types.rs | 10 ++- 5 files changed, 131 insertions(+), 81 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 571595a6..58f321f0 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -5,7 +5,6 @@ use crate::http::{HttpListener, HttpResponse}; use crate::machine::loader::LiveLoadState; use crate::machine::machine_indices::*; use crate::machine::streams::*; -use crate::parser::char_reader::CharReader; use crate::raw_block::*; use crate::read::*; use crate::types::UntypedArenaPtr; @@ -45,9 +44,12 @@ 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); +pub fn header_offset_from_payload() -> usize +where + T::Payload: Sized, +{ + 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); @@ -220,60 +222,75 @@ impl ArenaHeader { } #[derive(Debug)] -pub struct TypedArenaPtr(ptr::NonNull); +pub struct TypedArenaPtr(ptr::NonNull); -impl PartialOrd for TypedArenaPtr { +impl PartialOrd for TypedArenaPtr +where + T::Payload: PartialOrd, +{ fn partial_cmp(&self, other: &Self) -> Option { (**self).partial_cmp(&**other) } } -impl PartialEq for TypedArenaPtr { +impl PartialEq for TypedArenaPtr +where + T::Payload: PartialEq, +{ fn eq(&self, other: &TypedArenaPtr) -> bool { std::ptr::addr_eq(self.0.as_ptr(), other.0.as_ptr()) || **self == **other } } -impl Eq for TypedArenaPtr {} +impl Eq for TypedArenaPtr where T::Payload: Eq {} -impl Ord for TypedArenaPtr { +impl Ord for TypedArenaPtr +where + T::Payload: Ord, +{ fn cmp(&self, other: &Self) -> std::cmp::Ordering { (**self).cmp(&**other) } } -impl Hash for TypedArenaPtr { +impl Hash for TypedArenaPtr +where + T::Payload: Hash, +{ #[inline(always)] fn hash(&self, hasher: &mut H) { - (self as &T).hash(hasher) + (self as &T::Payload).hash(hasher) } } -impl Clone for TypedArenaPtr { +impl Clone for TypedArenaPtr { fn clone(&self) -> Self { *self } } -impl Copy for TypedArenaPtr {} +impl Copy for TypedArenaPtr {} -impl Deref for TypedArenaPtr { - type Target = T; +impl Deref for TypedArenaPtr { + type Target = T::Payload; fn deref(&self) -> &Self::Target { unsafe { self.0.as_ref() } } } -impl DerefMut for TypedArenaPtr { +impl DerefMut for TypedArenaPtr { fn deref_mut(&mut self) -> &mut Self::Target { unsafe { self.0.as_mut() } } } -impl fmt::Display for TypedArenaPtr { +impl fmt::Display for TypedArenaPtr +where + T::Payload: fmt::Display, +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", **self) + write!(f, "{}", (self as &T::Payload)) } } @@ -282,15 +299,20 @@ impl TypedArenaPtr { /// - 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 unsafe fn new(data: *mut T) -> Self { + pub const unsafe fn new(data: *mut T::Payload) -> Self { unsafe { TypedArenaPtr(ptr::NonNull::new_unchecked(data)) } } #[inline] - pub fn as_ptr(&self) -> *mut T { + pub fn as_ptr(&self) -> *mut T::Payload { self.0.as_ptr() } +} +impl TypedArenaPtr +where + T::Payload: Sized, +{ #[inline] pub fn header_ptr(&self) -> *const ArenaHeader { unsafe { self.as_ptr().byte_sub(T::header_offset_from_payload()) as *const _ } @@ -333,24 +355,35 @@ impl TypedArenaPtr { } } -pub trait ArenaAllocated: Sized { +pub trait ArenaAllocated { + type Payload: ?Sized; + fn tag() -> ArenaHeaderTag; - fn header_offset_from_payload() -> usize { + fn header_offset_from_payload() -> usize + where + Self::Payload: Sized, + { header_offset_from_payload::() } /// # Safety /// - the caller must guarantee that the pointee type of UntypedArenaPtr is Self - unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr { + unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr + where + Self::Payload: Sized, + { // safety: // - allocated in an arena as from an UntypedArenaPtr // - caller guarantees the type is correct - unsafe { TypedArenaPtr::new(ptr.payload_offset().cast_mut().cast::()) } + unsafe { TypedArenaPtr::new(ptr.payload_offset().cast_mut().cast::()) } } #[allow(clippy::missing_safety_doc)] - fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { + fn alloc(arena: &mut Arena, value: Self::Payload) -> TypedArenaPtr + where + Self::Payload: Sized, + { let size = mem::size_of::>(); let slab = Box::new(TypedAllocSlab { slab: AllocSlab { @@ -512,6 +545,7 @@ impl fmt::Display for F64Offset { } impl ArenaAllocated for Integer { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Integer @@ -519,6 +553,7 @@ impl ArenaAllocated for Integer { } impl ArenaAllocated for Rational { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Rational @@ -526,6 +561,7 @@ impl ArenaAllocated for Rational { } impl ArenaAllocated for LiveLoadState { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::LiveLoadState @@ -533,6 +569,7 @@ impl ArenaAllocated for LiveLoadState { } impl ArenaAllocated for TcpListener { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::TcpListener @@ -541,6 +578,7 @@ impl ArenaAllocated for TcpListener { #[cfg(feature = "http")] impl ArenaAllocated for HttpListener { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpListener @@ -549,6 +587,7 @@ impl ArenaAllocated for HttpListener { #[cfg(feature = "http")] impl ArenaAllocated for HttpResponse { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpResponse @@ -556,6 +595,7 @@ impl ArenaAllocated for HttpResponse { } impl ArenaAllocated for IndexPtr { + type Payload = Self; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::IndexPtrUndefined @@ -625,16 +665,16 @@ pub struct AllocSlab { #[repr(C)] #[derive(Clone, Debug)] -pub struct TypedAllocSlab { +pub struct TypedAllocSlab { slab: AllocSlab, - payload: Payload, + payload: T::Payload, } -impl TypedAllocSlab { +impl TypedAllocSlab { /// # Safety /// - ptr points to a valid allocation of Self #[inline] - pub unsafe fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { + pub unsafe fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { // safety: // - this is the arena allocation of corresponding type unsafe { TypedArenaPtr::new(addr_of_mut!((*ptr).payload)) } @@ -676,34 +716,34 @@ unsafe fn drop_slab_in_place(value: NonNull) { drop_typed_slab_in_place!(Rational, value); } ArenaHeaderTag::InputFileStream => { - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(InputFileStream, value); } ArenaHeaderTag::OutputFileStream => { - drop_typed_slab_in_place!(StreamLayout, value); + drop_typed_slab_in_place!(OutputFileStream, value); } ArenaHeaderTag::NamedTcpStream => { - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(NamedTcpStream, value); } ArenaHeaderTag::NamedTlsStream => { #[cfg(feature = "tls")] - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(NamedTlsStream, value); } ArenaHeaderTag::HttpReadStream => { #[cfg(feature = "http")] - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(HttpReadStream, value); } ArenaHeaderTag::HttpWriteStream => { #[cfg(feature = "http")] - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(HttpWriteStream, value); } ArenaHeaderTag::ReadlineStream => { - drop_typed_slab_in_place!(StreamLayout, value); + drop_typed_slab_in_place!(ReadlineStream, value); } ArenaHeaderTag::StaticStringStream => { - drop_typed_slab_in_place!(StreamLayout, value); + drop_typed_slab_in_place!(StaticStringStream, value); } ArenaHeaderTag::ByteStream => { - drop_typed_slab_in_place!(StreamLayout>, value); + drop_typed_slab_in_place!(ByteStream, value); } ArenaHeaderTag::LiveLoadState | ArenaHeaderTag::InactiveLoadState => { drop_typed_slab_in_place!(LiveLoadState, value); @@ -721,10 +761,10 @@ unsafe fn drop_slab_in_place(value: NonNull) { drop_typed_slab_in_place!(HttpResponse, value); } ArenaHeaderTag::StandardOutputStream => { - drop_typed_slab_in_place!(StreamLayout, value); + drop_typed_slab_in_place!(StandardOutputStream, value); } ArenaHeaderTag::StandardErrorStream => { - drop_typed_slab_in_place!(StreamLayout, value); + drop_typed_slab_in_place!(StandardErrorStream, value); } ArenaHeaderTag::NullStream | ArenaHeaderTag::IndexPtrUndefined diff --git a/src/machine/loader.rs b/src/machine/loader.rs index cd4f419a..dd4f54bd 100644 --- a/src/machine/loader.rs +++ b/src/machine/loader.rs @@ -1757,7 +1757,7 @@ impl Machine { #[inline] pub(crate) fn push_load_state_payload(&mut self) { - let payload = arena_alloc!( + let payload: TypedArenaPtr = arena_alloc!( LoadStatePayload::new(self.code.len(), LiveTermStream::new(ListingSource::User),), &mut self.machine_st.arena ); diff --git a/src/machine/streams.rs b/src/machine/streams.rs index aec6464a..ad09e858 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -451,7 +451,9 @@ impl DerefMut for StreamLayout { macro_rules! arena_allocated_impl_for_stream { ($stream_type:ty, $stream_tag:ident) => { - impl ArenaAllocated for StreamLayout<$stream_type> { + impl ArenaAllocated for $stream_tag { + type Payload = StreamLayout<$stream_type>; + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::$stream_tag @@ -477,26 +479,26 @@ arena_allocated_impl_for_stream!(StandardErrorStream, StandardErrorStream); #[derive(Debug, Copy, Clone)] pub enum Stream { - Byte(TypedArenaPtr>>), - InputFile(TypedArenaPtr>>), - OutputFile(TypedArenaPtr>), - StaticString(TypedArenaPtr>), - NamedTcp(TypedArenaPtr>>), + Byte(TypedArenaPtr), + InputFile(TypedArenaPtr), + OutputFile(TypedArenaPtr), + StaticString(TypedArenaPtr), + NamedTcp(TypedArenaPtr), #[cfg(feature = "tls")] - NamedTls(TypedArenaPtr>>), + NamedTls(TypedArenaPtr), #[cfg(feature = "http")] - HttpRead(TypedArenaPtr>>), + HttpRead(TypedArenaPtr), #[cfg(feature = "http")] - HttpWrite(TypedArenaPtr>>), + HttpWrite(TypedArenaPtr), Null(StreamOptions), - Readline(TypedArenaPtr>), - StandardOutput(TypedArenaPtr>), - StandardError(TypedArenaPtr>), + Readline(TypedArenaPtr), + StandardOutput(TypedArenaPtr), + StandardError(TypedArenaPtr), } -impl From>> for Stream { +impl From> for Stream { #[inline] - fn from(stream: TypedArenaPtr>) -> Stream { + fn from(stream: TypedArenaPtr) -> Stream { Stream::Readline(stream) } } diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index 80b579e7..5cddb644 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -4519,7 +4519,8 @@ impl Machine { }); let http_listener = HttpListener { incoming: rx }; - let http_listener = arena_alloc!(http_listener, &mut self.machine_st.arena); + let http_listener: TypedArenaPtr = + arena_alloc!(http_listener, &mut self.machine_st.arena); let addr = self.deref_register(2); self.machine_st.bind( @@ -4584,7 +4585,7 @@ impl Machine { self.indices.streams.insert(stream); let stream = stream_as_cell!(stream); - let handle = arena_alloc!(request.response, &mut self.machine_st.arena); + let handle: TypedArenaPtr = arena_alloc!(request.response, &mut self.machine_st.arena); self.machine_st.bind(method.as_var().unwrap(), atom_as_cell!(method_atom)); self.machine_st.bind(path.as_var().unwrap(), path_cell); @@ -6516,32 +6517,33 @@ impl Machine { format!("{}:{}", socket_atom.as_str(), port) }; - let (tcp_listener, port) = match TcpListener::bind(server_addr).map_err(|e| e.kind()) { - Ok(tcp_listener) => { - let port = tcp_listener.local_addr().map(|addr| addr.port()).ok(); + let (tcp_listener, port): (TypedArenaPtr, _) = + match TcpListener::bind(server_addr).map_err(|e| e.kind()) { + Ok(tcp_listener) => { + let port = tcp_listener.local_addr().map(|addr| addr.port()).ok(); - if let Some(port) = port { - ( - arena_alloc!(tcp_listener, &mut self.machine_st.arena), - port as usize, - ) - } else { + if let Some(port) = port { + ( + arena_alloc!(tcp_listener, &mut self.machine_st.arena), + port as usize, + ) + } else { + self.machine_st.fail = true; + return Ok(()); + } + } + Err(ErrorKind::PermissionDenied) => { + return Err(self.machine_st.open_permission_error( + addr, + atom!("socket_server_open"), + 2, + )); + } + _ => { self.machine_st.fail = true; return Ok(()); } - } - Err(ErrorKind::PermissionDenied) => { - return Err(self.machine_st.open_permission_error( - addr, - atom!("socket_server_open"), - 2, - )); - } - _ => { - self.machine_st.fail = true; - return Ok(()); - } - }; + }; let addr = self.deref_register(3); self.machine_st.bind( diff --git a/src/types.rs b/src/types.rs index 2872f541..07328a07 100644 --- a/src/types.rs +++ b/src/types.rs @@ -300,7 +300,10 @@ impl fmt::Debug for HeapCellValue { } } -impl From> for HeapCellValue { +impl From> for HeapCellValue +where + T::Payload: Sized, +{ #[inline] fn from(arena_ptr: TypedArenaPtr) -> HeapCellValue { HeapCellValue::from(arena_ptr.header_ptr() as u64) @@ -708,7 +711,10 @@ impl UntypedArenaPtr { /// # Safety /// - this UntypedArenaPtr actuall pointee type is T #[inline] - pub unsafe fn as_typed_ptr(self) -> TypedArenaPtr { + pub unsafe fn as_typed_ptr(self) -> TypedArenaPtr + where + T::Payload: Sized, + { T::typed_ptr(self) } From 33793193cc8bdb77c3e98a99090d0b0a90ff4d8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 15:46:04 +0200 Subject: [PATCH 27/45] add miri to CI --- .github/workflows/ci.yml | 7 ++++++- tests/scryer/issues.rs | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d0ce4ad..7cdbc9a2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: - { os: ubuntu-22.04, rust-version: "1.77", target: 'x86_64-unknown-linux-gnu'} # rust versions - { os: ubuntu-22.04, rust-version: beta, target: 'x86_64-unknown-linux-gnu'} - - { os: ubuntu-22.04, rust-version: nightly, target: 'x86_64-unknown-linux-gnu'} + - { os: ubuntu-22.04, rust-version: nightly, target: 'x86_64-unknown-linux-gnu', miri: true, components: ["miri"]} defaults: run: shell: bash @@ -66,6 +66,7 @@ jobs: rust-version: ${{ matrix.rust-version }} targets: ${{ matrix.target }} cache-context: ${{ matrix.os }} + components: ${{ matrix.components }} # Build and test. - name: Build library @@ -73,6 +74,10 @@ jobs: - name: Test run: cargo test --target ${{ matrix.target }} ${{ matrix.test-args }} --all + - name: Check miri + if: matrix.miri + run: cargo miri test + # On stable rust builds, build a binary and publish as a github actions # artifact. These binaries could be useful for testing the pipeline but # are only retained by github for 90 days. diff --git a/tests/scryer/issues.rs b/tests/scryer/issues.rs index 40e3f5ce..c21a8381 100644 --- a/tests/scryer/issues.rs +++ b/tests/scryer/issues.rs @@ -15,6 +15,7 @@ fn call_0() { // issue #2361 #[serial] #[test] +#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] fn call_qualification() { load_module_test("tests-pl/issue2361-call-qualified.pl", ""); } From 8e53d12776c23bac9d7b7cfd398941a31e6e5b66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 16:01:52 +0200 Subject: [PATCH 28/45] make components a comma seperated string instead of a list --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7cdbc9a2..0345cdaf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ jobs: - { os: ubuntu-22.04, rust-version: "1.77", target: 'x86_64-unknown-linux-gnu'} # rust versions - { os: ubuntu-22.04, rust-version: beta, target: 'x86_64-unknown-linux-gnu'} - - { os: ubuntu-22.04, rust-version: nightly, target: 'x86_64-unknown-linux-gnu', miri: true, components: ["miri"]} + - { os: ubuntu-22.04, rust-version: nightly, target: 'x86_64-unknown-linux-gnu', miri: true, components: "miri"} defaults: run: shell: bash From fee7ba58b0fdb612e4ca1f521340cf4de0f51ba5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 16:21:20 +0200 Subject: [PATCH 29/45] fix stack alignement - adjust align() in RawBlockTraits impl for Stack - ensure ptr is always aligned in RawBlock::allock --- src/machine/stack.rs | 5 +++-- src/raw_block.rs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/machine/stack.rs b/src/machine/stack.rs index 604a52be..7f35cdbc 100644 --- a/src/machine/stack.rs +++ b/src/machine/stack.rs @@ -15,7 +15,9 @@ impl RawBlockTraits for Stack { #[inline] fn align() -> usize { - mem::align_of::() + mem::align_of::() + .max(mem::align_of::()) + .max(mem::align_of::()) } } @@ -281,7 +283,6 @@ mod tests { use crate::machine::mock_wam::*; #[test] - #[cfg_attr(miri, ignore = "blocked on stack.rs UB")] fn stack_tests() { let mut wam = MockWAM::new(); diff --git a/src/raw_block.rs b/src/raw_block.rs index 2c10f494..0c27bdde 100644 --- a/src/raw_block.rs +++ b/src/raw_block.rs @@ -96,9 +96,10 @@ impl RawBlock { } pub unsafe fn alloc(&self, size: usize) -> *mut u8 { - if self.free_space() >= size { + let aligned_size = size.next_multiple_of(size); + if self.free_space() >= aligned_size { let ptr = *self.ptr.get(); - *self.ptr.get() = ptr.add(size) as *mut _; + *self.ptr.get() = ptr.add(aligned_size) as *mut _; ptr } else { ptr::null_mut() From 1d66f91a41858a06770e227a4a37f8831d1df04a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 16:56:41 +0200 Subject: [PATCH 30/45] replace `ref_thread_local::ref_thread_local!` with `std::thread_local!` removes `ref_thread_local` which was still at 0.0.0 released October 2018 while latest 0.1.1 was released mid November 2021 fixes libraries.rs UB --- Cargo.lock | 7 ------- Cargo.toml | 1 - build/main.rs | 4 ++-- src/machine/lib_machine.rs | 2 +- src/machine/load_state.rs | 5 ++--- src/machine/loader.rs | 2 +- src/machine/mod.rs | 30 ++++++++++++++++++++++++------ src/machine/system_calls.rs | 9 +++------ 8 files changed, 33 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb2bd1b2..6b3d3394 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2297,12 +2297,6 @@ dependencies = [ "thiserror", ] -[[package]] -name = "ref_thread_local" -version = "0.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d813022b2e00774a48eaf43caaa3c20b45f040ba8cbf398e2e8911a06668dbe6" - [[package]] name = "regex" version = "1.10.2" @@ -2605,7 +2599,6 @@ dependencies = [ "proc-macro2", "quote", "rand", - "ref_thread_local", "regex", "reqwest", "ring 0.17.7", diff --git a/Cargo.toml b/Cargo.toml index 45ba6d6e..67c3e60d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -61,7 +61,6 @@ num-order = { version = "1.2.0" } ordered-float = "2.6.0" phf = { version = "0.9", features = ["macros"] } rand = "0.8.5" -ref_thread_local = "0.0.0" regex = "1.9.1" ring = { version = "0.17.5", features = ["wasm32_unknown_unknown_js"] } ripemd160 = "0.8.0" diff --git a/build/main.rs b/build/main.rs index bd254c93..b7a66f03 100644 --- a/build/main.rs +++ b/build/main.rs @@ -82,8 +82,8 @@ use indexmap::IndexMap;\ writeln!( libraries, "\ -ref_thread_local::ref_thread_local! {{ - pub(crate) static managed LIBRARIES: IndexMap<&'static str, &'static str> = {{ +std::thread_local!{{ + static LIBRARIES: IndexMap<&'static str, &'static str> = {{ let mut m = IndexMap::new();" ) .unwrap(); diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index 80e0d1c6..57bb45a9 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -238,7 +238,7 @@ mod tests { use crate::machine::{QueryMatch, QueryResolution, Value}; #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn programatic_query() { let mut machine = Machine::new_lib(); diff --git a/src/machine/load_state.rs b/src/machine/load_state.rs index f340ad03..121c0c1b 100644 --- a/src/machine/load_state.rs +++ b/src/machine/load_state.rs @@ -9,7 +9,6 @@ use crate::parser::ast::*; use fxhash::FxBuildHasher; use indexmap::IndexSet; -pub use ref_thread_local::RefThreadLocal; use std::collections::VecDeque; use std::fs::File; @@ -1176,7 +1175,7 @@ impl<'a, LS: LoadState<'a>> Loader<'a, LS> { ListingSource::File(filename, path_buf), ) } - ModuleSource::Library(library) => match LIBRARIES.borrow().get(&*library.as_str()) { + ModuleSource::Library(library) => match libraries::get(&library.as_str()) { Some(code) => { if let Some(module) = self.wam_prelude.indices.modules.get(&library) { if let ListingSource::DynamicallyGenerated = &module.listing_src { @@ -1257,7 +1256,7 @@ impl<'a, LS: LoadState<'a>> Loader<'a, LS> { ListingSource::File(filename, path_buf), ) } - ModuleSource::Library(library) => match LIBRARIES.borrow().get(&*library.as_str()) { + ModuleSource::Library(library) => match libraries::get(&library.as_str()) { Some(code) => { if self.wam_prelude.indices.modules.contains_key(&library) { return self.import_qualified_module(library, exports); diff --git a/src/machine/loader.rs b/src/machine/loader.rs index dd4f54bd..88666330 100644 --- a/src/machine/loader.rs +++ b/src/machine/loader.rs @@ -353,7 +353,7 @@ impl<'a> LoadState<'a> for LiveLoadAndMachineState<'a> { #[inline] fn err_on_builtin_module_overwrite(module_name: Atom) -> Result<(), SessionError> { - if LIBRARIES.borrow().contains_key(&*module_name.as_str()) { + if libraries::contains(&module_name.as_str()) { Err(SessionError::CannotOverwriteBuiltInModule(module_name)) } else { Ok(()) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index d3d22953..6a9c74e6 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -119,9 +119,25 @@ fn current_dir() -> PathBuf { mod libraries { include!(concat!(env!("OUT_DIR"), "/libraries.rs")); -} -pub(crate) use libraries::LIBRARIES; + pub(crate) fn contains(name: &str) -> bool { + LIBRARIES.with(|libs| libs.contains_key(name)) + } + + pub(crate) fn get(name: &str) -> Option<&'static str> { + LIBRARIES.with(|libs| libs.get(name).map(|&lib| lib)) + } + + #[cfg(test)] + std::thread_local! { + #[allow(dead_code)] + static LIBRARIES2 : IndexMap<&'static str, &'static str> = { + let mut m = IndexMap::new(); + m.insert("test", "test2"); + m + }; + } +} pub static BREAK_FROM_DISPATCH_LOOP_LOC: usize = 0; pub static INSTALL_VERIFY_ATTR_INTERRUPT: usize = 1; @@ -456,8 +472,6 @@ impl Machine { #[allow(clippy::new_without_default)] pub fn new(config: MachineConfig) -> Self { - use ref_thread_local::RefThreadLocal; - let args = MachineArgs::new(); let mut machine_st = MachineState::new(); @@ -496,7 +510,8 @@ impl Machine { bootstrapping_compile( Stream::from_static_string( - libraries::LIBRARIES.borrow()["ops_and_meta_predicates"], + libraries::get("ops_and_meta_predicates") + .expect("library ops_and_meta_predicates should exist"), &mut wam.machine_st.arena, ), &mut wam, @@ -508,7 +523,10 @@ impl Machine { .unwrap(); bootstrapping_compile( - Stream::from_static_string(LIBRARIES.borrow()["builtins"], &mut wam.machine_st.arena), + Stream::from_static_string( + libraries::get("builtins").expect("library builtins should exist"), + &mut wam.machine_st.arena, + ), &mut wam, ListingSource::from_file_and_path(atom!("builtins.pl"), lib_path.clone()), ) diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index 5cddb644..17095a36 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -39,8 +39,6 @@ use ordered_float::OrderedFloat; use fxhash::{FxBuildHasher, FxHasher}; use indexmap::IndexSet; -pub(crate) use ref_thread_local::RefThreadLocal; - use std::cell::Cell; use std::cmp::Ordering; use std::collections::BTreeSet; @@ -105,6 +103,8 @@ use warp::hyper::{HeaderMap, Method}; #[cfg(feature = "http")] use warp::{Buf, Filter}; +use super::libraries; + #[cfg(feature = "repl")] pub(crate) fn get_key() -> KeyEvent { let key; @@ -7998,10 +7998,7 @@ impl Machine { pub(crate) fn load_library_as_stream(&mut self) -> CallResult { let library_name = cell_as_atom!(self.deref_register(1)); - use crate::machine::LIBRARIES; - - let lib_ref = LIBRARIES.borrow(); - let lib = lib_ref.get(&*library_name.as_str()); + let lib = libraries::get(&library_name.as_str()); match lib { Some(library) => { let lib_stream = Stream::from_static_string(library, &mut self.machine_st.arena); From ea041a40f9852ac706d0d4f1e201af384bd19a11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 17:43:13 +0200 Subject: [PATCH 31/45] if UB un IndexPtr ArenaAllocated impl --- src/arena.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 58f321f0..fb42ac17 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -614,15 +614,17 @@ impl ArenaAllocated for IndexPtr { #[inline] fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { - let mut slab = Box::new(AllocSlab { + let slab = Box::new(AllocSlab { next: arena.base.take(), #[cfg(target_pointer_width = "32")] _padding: 0, header: HeaderOrIdxPtr { idx_ptr: value }, }); - let allocated_ptr = unsafe { TypedArenaPtr::new(ptr::addr_of_mut!(slab.header.idx_ptr)) }; - arena.base = Some(NonNull::new(Box::into_raw(slab)).unwrap()); + let raw_box = Box::into_raw(slab); + let allocated_ptr = + unsafe { TypedArenaPtr::new(ptr::addr_of_mut!((*raw_box).header.idx_ptr)) }; + arena.base = Some(NonNull::new(raw_box).unwrap()); allocated_ptr } } From b32498b37cb018c11f10396cd8c5de70bfad1fd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 18:24:16 +0200 Subject: [PATCH 32/45] adjust ignore reason --- src/machine/lib_machine.rs | 22 +++++++++++----------- tests/scryer/issues.rs | 4 ++-- tests/scryer/main.rs | 5 ++++- tests/scryer/src_tests.rs | 20 ++++++++++---------- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/machine/lib_machine.rs b/src/machine/lib_machine.rs index 57bb45a9..8480950b 100644 --- a/src/machine/lib_machine.rs +++ b/src/machine/lib_machine.rs @@ -278,7 +278,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn failing_query() { let mut machine = Machine::new_lib(); let query = String::from(r#"triple("a",P,"b")."#); @@ -292,7 +292,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn complex_results() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -349,7 +349,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn empty_predicate() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -365,7 +365,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn list_results() { let mut machine = Machine::new_lib(); machine.load_module_string( @@ -394,7 +394,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn consult() { let mut machine = Machine::new_lib(); @@ -453,7 +453,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn integration_test() { let mut machine = Machine::new_lib(); @@ -496,7 +496,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn findall() { let mut machine = Machine::new_lib(); @@ -529,7 +529,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn dont_return_partial_matches() { let mut machine = Machine::new_lib(); @@ -553,7 +553,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn dont_return_partial_matches_without_discountiguous() { let mut machine = Machine::new_lib(); @@ -585,7 +585,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn non_existent_predicate_should_not_cause_panic_when_other_predicates_are_defined() { let mut machine = Machine::new_lib(); @@ -610,7 +610,7 @@ mod tests { } #[test] - #[cfg_attr(miri, ignore = "blocked on libraries.rs UB")] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn issue_2341() { let mut machine = Machine::new_lib(); diff --git a/tests/scryer/issues.rs b/tests/scryer/issues.rs index c21a8381..5e21577e 100644 --- a/tests/scryer/issues.rs +++ b/tests/scryer/issues.rs @@ -4,7 +4,7 @@ use serial_test::serial; // issue #831 #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn call_0() { load_module_test( "tests-pl/issue831-call0.pl", @@ -15,7 +15,7 @@ fn call_0() { // issue #2361 #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn call_qualification() { load_module_test("tests-pl/issue2361-call-qualified.pl", ""); } diff --git a/tests/scryer/main.rs b/tests/scryer/main.rs index 878f0d90..85f7df98 100644 --- a/tests/scryer/main.rs +++ b/tests/scryer/main.rs @@ -14,7 +14,10 @@ mod src_tests; /// then check that the changes are as expected e.g. by looking at the `git diff` #[test] #[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))] -#[cfg_attr(miri, ignore = "blocked on crossbeam UB")] +#[cfg_attr( + miri, + ignore = "miri isolation, unsupported operation: can't call foreign function" +)] fn cli_tests() { trycmd::TestCases::new() .default_bin_name("scryer-prolog") diff --git a/tests/scryer/src_tests.rs b/tests/scryer/src_tests.rs index 2434cb1f..f0edd5d8 100644 --- a/tests/scryer/src_tests.rs +++ b/tests/scryer/src_tests.rs @@ -3,35 +3,35 @@ use serial_test::serial; #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn builtins() { load_module_test("src/tests/builtins.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn call_with_inference_limit() { load_module_test("src/tests/call_with_inference_limit.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn facts() { load_module_test("src/tests/facts.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn hello_world() { load_module_test("src/tests/hello_world.pl", "Hello World!\n"); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn syntax_error() { load_module_test( "tests-pl/syntax_error.pl", @@ -41,21 +41,21 @@ fn syntax_error() { #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn predicates() { load_module_test("src/tests/predicates.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn rules() { load_module_test("src/tests/rules.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn setup_call_cleanup_load() { load_module_test( "src/tests/setup_call_cleanup.pl", @@ -65,14 +65,14 @@ fn setup_call_cleanup_load() { #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn clpz_load() { load_module_test("src/tests/clpz/test_clpz.pl", ""); } #[serial] #[test] -#[cfg_attr(miri, ignore = "blocked on helper.rs UB")] +#[cfg_attr(miri, ignore = "it takes too long to run")] fn iso_conformity_tests() { load_module_test("tests-pl/iso-conformity-tests.pl", "All tests passed"); } From 943dce566b6d6b43816f2eef0c39cb87abd9273c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 18:26:34 +0200 Subject: [PATCH 33/45] fix clippy --- src/machine/mod.rs | 2 +- src/machine/parsed_results.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index 6a9c74e6..d4a69c77 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -125,7 +125,7 @@ mod libraries { } pub(crate) fn get(name: &str) -> Option<&'static str> { - LIBRARIES.with(|libs| libs.get(name).map(|&lib| lib)) + LIBRARIES.with(|libs| libs.get(name).copied()) } #[cfg(test)] diff --git a/src/machine/parsed_results.rs b/src/machine/parsed_results.rs index 681b27e4..8ca6f8e8 100644 --- a/src/machine/parsed_results.rs +++ b/src/machine/parsed_results.rs @@ -38,7 +38,7 @@ pub fn write_prolog_value_as_json( ) } else { //return valid json string - writer.write_str(&s) + writer.write_str(s) } } Value::List(l) => { From c414f75329477d8ffb8747d7a7e228af8bbdf11b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 18:47:35 +0200 Subject: [PATCH 34/45] switch macos to latest as macos-11 has been removed See https://github.com/actions/runner-images/issues/9255 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0345cdaf..3fc66550 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,7 +39,7 @@ jobs: include: # operating systems - { os: windows-latest, rust-version: stable, target: 'x86_64-pc-windows-msvc', publish: true } - - { os: macos-11, rust-version: stable, target: 'x86_64-apple-darwin', publish: true } + - { os: macos-latest, rust-version: stable, target: 'x86_64-apple-darwin', publish: true } - { os: ubuntu-20.04, rust-version: stable, target: 'x86_64-unknown-linux-gnu', publish: true } # architectures - { os: ubuntu-22.04, rust-version: stable, target: 'x86_64-unknown-linux-gnu', publish: true } From 0284a2092dfae1f5fead17a1ad140bb10f9c7fff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 22:47:22 +0200 Subject: [PATCH 35/45] reduce span of unsafe block --- src/machine/machine_state.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/machine/machine_state.rs b/src/machine/machine_state.rs index 3763b2c0..e90b8644 100644 --- a/src/machine/machine_state.rs +++ b/src/machine/machine_state.rs @@ -679,15 +679,13 @@ impl MachineState { indices: &mut IndexStore, ) -> CallResult { if let Stream::Readline(ptr) = stream { - unsafe { - let readline = ptr.as_ptr().as_mut().unwrap(); - readline.set_atoms_for_completion(&self.atom_tbl); - return self.read_term( - stream, - indices, - MachineState::read_term_from_user_input_eof_handler, - ); - } + let readline = unsafe { ptr.as_ptr().as_mut() }.unwrap(); + readline.set_atoms_for_completion(&self.atom_tbl); + return self.read_term( + stream, + indices, + MachineState::read_term_from_user_input_eof_handler, + ); } if let Stream::Byte(_) = stream { From a2a50586aa3b77c18990cd21bba62125776b4c0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 22:48:04 +0200 Subject: [PATCH 36/45] replace static Once and two mut statics with one static OnceLock --- src/machine/mod.rs | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index d4a69c77..33199edd 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -60,6 +60,7 @@ use std::env; use std::io::Read; use std::path::PathBuf; use std::sync::atomic::AtomicBool; +use std::sync::OnceLock; use self::config::MachineConfig; use self::parsed_results::*; @@ -1261,33 +1262,25 @@ impl Machine { #[inline(always)] fn run_cleaners(&mut self) -> bool { - use std::sync::Once; + static CLEANER_INIT: OnceLock<(usize, usize)> = OnceLock::new(); - static CLEANER_INIT: Once = Once::new(); + let (r_c_w_h, r_c_wo_h) = *CLEANER_INIT.get_or_init(|| { + let r_c_w_h_atom = atom!("run_cleaners_with_handling"); + let r_c_wo_h_atom = atom!("run_cleaners_without_handling"); + let iso_ext = atom!("iso_ext"); - static mut RCWH: usize = 0; - static mut RCWOH: usize = 0; - - let (r_c_w_h, r_c_wo_h) = unsafe { - CLEANER_INIT.call_once(|| { - let r_c_w_h_atom = atom!("run_cleaners_with_handling"); - let r_c_wo_h_atom = atom!("run_cleaners_without_handling"); - let iso_ext = atom!("iso_ext"); - - RCWH = self - .indices - .get_predicate_code_index(r_c_w_h_atom, 0, iso_ext) - .and_then(|item| item.local()) - .unwrap(); - RCWOH = self - .indices - .get_predicate_code_index(r_c_wo_h_atom, 1, iso_ext) - .and_then(|item| item.local()) - .unwrap(); - }); - - (RCWH, RCWOH) - }; + let r_c_w_h = self + .indices + .get_predicate_code_index(r_c_w_h_atom, 0, iso_ext) + .and_then(|item| item.local()) + .unwrap(); + let r_c_wo_h = self + .indices + .get_predicate_code_index(r_c_wo_h_atom, 1, iso_ext) + .and_then(|item| item.local()) + .unwrap(); + (r_c_w_h, r_c_wo_h) + }); if let Some(&(_, b_cutoff, prev_block)) = self.machine_st.cont_pts.last() { if self.machine_st.b < b_cutoff { From 89c1ea4232e4c2b66758b32c00e6adadea1acd3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 23:08:21 +0200 Subject: [PATCH 37/45] fix leak of ArenaAllocated IndexPtr --- src/arena.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index fb42ac17..75ac69f0 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -627,6 +627,12 @@ impl ArenaAllocated for IndexPtr { arena.base = Some(NonNull::new(raw_box).unwrap()); allocated_ptr } + + /// # Safety + /// - ptr points to an allocated slab of the correct kind + unsafe fn dealloc(ptr: NonNull>) { + drop(unsafe { Box::from_raw(ptr.as_ptr().cast::()) }); + } } #[repr(C)] @@ -768,11 +774,15 @@ unsafe fn drop_slab_in_place(value: NonNull) { ArenaHeaderTag::StandardErrorStream => { drop_typed_slab_in_place!(StandardErrorStream, value); } - ArenaHeaderTag::NullStream - | ArenaHeaderTag::IndexPtrUndefined + ArenaHeaderTag::IndexPtrUndefined | ArenaHeaderTag::IndexPtrDynamicUndefined | ArenaHeaderTag::IndexPtrDynamicIndex - | ArenaHeaderTag::IndexPtrIndex => {} + | ArenaHeaderTag::IndexPtrIndex => { + drop_typed_slab_in_place!(IndexPtr, value); + } + ArenaHeaderTag::NullStream => { + unreachable!("NullStream is never arena allocated!"); + } } } From 496e4e9f56f1f8a956f12126fb8019e4c51a1077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 6 Jul 2024 23:18:35 +0200 Subject: [PATCH 38/45] fix clippy again --- src/machine/streams.rs | 3 +++ src/macros.rs | 1 + 2 files changed, 4 insertions(+) diff --git a/src/machine/streams.rs b/src/machine/streams.rs index ad09e858..61191205 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -323,6 +323,9 @@ impl Write for HttpWriteStream { #[cfg(feature = "http")] impl HttpWriteStream { + // TODO why is this suddenly dead code and should it be used somewhere? + // Should this be impl Drop for HttpWriteStream? + #[allow(dead_code)] fn drop(&mut self) { let headers = unsafe { std::mem::ManuallyDrop::take(&mut self.headers) }; let buffer = unsafe { std::mem::ManuallyDrop::take(&mut self.buffer) }; diff --git a/src/macros.rs b/src/macros.rs index 54fcaa63..ca556513 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -324,6 +324,7 @@ macro_rules! match_untyped_arena_ptr { ($ptr:expr, $( ($(ArenaHeaderTag::$tag:tt)|+, $n:ident) => $code:block $(,)?)+ $(_ => $misc_code:expr $(,)?)?) => ({ let ptr_id = $ptr; + #[allow(clippy::toplevel_ref_arg)] match ptr_id.get_tag() { $($(match_untyped_arena_ptr_pat!($tag) => { match_untyped_arena_ptr_pat_body!(ptr_id, $tag, $n, $code) From 39bd52054276d40b90dc3088ed488df1c5df6196 Mon Sep 17 00:00:00 2001 From: Skgland Date: Sun, 7 Jul 2024 11:06:11 +0200 Subject: [PATCH 39/45] ignore `pstr_iter_tests` test in miri as it takes too long --- src/machine/partial_string.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/machine/partial_string.rs b/src/machine/partial_string.rs index 1d6403d7..5f26eaed 100644 --- a/src/machine/partial_string.rs +++ b/src/machine/partial_string.rs @@ -801,6 +801,7 @@ mod test { use crate::machine::mock_wam::*; #[test] + #[cfg_attr(miri, ignore = "it takes too long to run")] fn pstr_iter_tests() { let mut wam = MockWAM::new(); From 74720d4d2ef07cf6848cd44470ee713c97938a5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 14:18:50 +0200 Subject: [PATCH 40/45] remove unsafe `impl From for CodeIndex` --- src/machine/heap.rs | 4 ++-- src/machine/machine_indices.rs | 7 ------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/machine/heap.rs b/src/machine/heap.rs index 7ba02da9..0f42b8c6 100644 --- a/src/machine/heap.rs +++ b/src/machine/heap.rs @@ -69,8 +69,8 @@ impl TryFrom for Literal { (ArenaHeaderTag::Rational, n) => { Ok(Literal::Rational(n)) } - (ArenaHeaderTag::IndexPtr, _ip) => { - Ok(Literal::CodeIndex(CodeIndex::from(cons_ptr))) + (ArenaHeaderTag::IndexPtr, ip) => { + Ok(Literal::CodeIndex(CodeIndex::from(ip))) } _ => { Err(()) diff --git a/src/machine/machine_indices.rs b/src/machine/machine_indices.rs index cecdef22..9dc8138e 100644 --- a/src/machine/machine_indices.rs +++ b/src/machine/machine_indices.rs @@ -159,13 +159,6 @@ impl From for UntypedArenaPtr { } } -impl From for CodeIndex { - #[inline(always)] - fn from(ptr: UntypedArenaPtr) -> CodeIndex { - CodeIndex(unsafe { ptr.as_typed_ptr() }) - } -} - impl From> for CodeIndex { #[inline(always)] fn from(ptr: TypedArenaPtr) -> CodeIndex { From 213ee5ca45d9bb93b73c28bf246d3026811fafc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 14:36:53 +0200 Subject: [PATCH 41/45] add `AllocateInArena` as a Pivot for `arena_alloc!` so that the value type passed to `arena_alloc!` can differ from the `ArenaAllocated::Payload` type --- src/arena.rs | 24 +++++++++++++++++++++++- src/parser/lexer.rs | 1 - 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 75ac69f0..bb346810 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -21,6 +21,7 @@ use std::fmt; use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::mem; +use std::mem::ManuallyDrop; use std::net::TcpListener; use std::ops::{Deref, DerefMut}; use std::ptr; @@ -32,7 +33,7 @@ use std::sync::RwLock; macro_rules! arena_alloc { ($e:expr, $arena:expr) => {{ let result = $e; - ArenaAllocated::alloc($arena, result) + $crate::arena::AllocateInArena::arena_allocate(result, $arena) }}; } @@ -355,6 +356,27 @@ where } } +pub trait AllocateInArena +where + AllocFor: ArenaAllocated, +{ + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr; +} + +impl> AllocateInArena for P { + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr { + T::alloc(arena, self) + } +} + +/* apparently this overlaps the planket impl above somehow +impl>> AllocateInArena for P { + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr { + T::alloc(arena, ManuallyDrop::new(self)) + } +} +*/ + pub trait ArenaAllocated { type Payload: ?Sized; diff --git a/src/parser/lexer.rs b/src/parser/lexer.rs index d783c588..03f472b9 100644 --- a/src/parser/lexer.rs +++ b/src/parser/lexer.rs @@ -1,6 +1,5 @@ use lexical::parse_lossy; -use crate::arena::ArenaAllocated; use crate::atom_table::*; pub use crate::machine::machine_state::*; use crate::parser::ast::*; From a0d790445d65f7d960656a7357b8f13b35540f58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 14:37:28 +0200 Subject: [PATCH 42/45] check that we don't attempt to un-drop while evacuating --- src/machine/loader.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/machine/loader.rs b/src/machine/loader.rs index 88666330..0dfdfad2 100644 --- a/src/machine/loader.rs +++ b/src/machine/loader.rs @@ -304,11 +304,15 @@ impl<'a> LoadState<'a> for LiveLoadAndMachineState<'a> { #[inline(always)] fn evacuate(mut loader: Loader<'a, Self>) -> Result { + if loader.payload.load_state.get_tag() != ArenaHeaderTag::Dropped { loader .payload .load_state .set_tag(ArenaHeaderTag::InactiveLoadState); Ok(loader.payload.load_state) + } else { + unreachable!("we never evacuate after dropping") + } } #[inline(always)] From 1b19ae81d5c3dff3806f9fa7404a0ce0a195fe04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 15:21:08 +0200 Subject: [PATCH 43/45] wrap Payloads that are dropped eraly in ManuallyDrop --- src/arena.rs | 33 +++++++++++++++++++++++++-- src/machine/loader.rs | 19 +++++++--------- src/machine/streams.rs | 45 ++++++++++++++++++------------------- src/machine/system_calls.rs | 6 +---- 4 files changed, 62 insertions(+), 41 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index bb346810..c532d24a 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -310,6 +310,13 @@ impl TypedArenaPtr { } } +impl>> TypedArenaPtr { + pub fn drop_payload(&mut self) { + self.set_tag(ArenaHeaderTag::Dropped); + unsafe { ManuallyDrop::drop(&mut *self.as_ptr()) } + } +} + impl TypedArenaPtr where T::Payload: Sized, @@ -582,16 +589,34 @@ impl ArenaAllocated for Rational { } } +impl AllocateInArena for LiveLoadState { + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr { + LiveLoadState::alloc(arena, ManuallyDrop::new(self)) + } +} + impl ArenaAllocated for LiveLoadState { - type Payload = Self; + type Payload = ManuallyDrop; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::LiveLoadState } + + unsafe fn dealloc(ptr: NonNull>) { + let mut slab = unsafe { Box::from_raw(ptr.as_ptr()) }; + unsafe { ManuallyDrop::drop(&mut slab.payload) }; + drop(slab); + } +} + +impl AllocateInArena for TcpListener { + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr { + TcpListener::alloc(arena, ManuallyDrop::new(self)) + } } impl ArenaAllocated for TcpListener { - type Payload = Self; + type Payload = ManuallyDrop; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::TcpListener @@ -701,6 +726,10 @@ pub struct TypedAllocSlab { } impl TypedAllocSlab { + pub fn payload(&mut self) -> &mut T::Payload { + &mut self.payload + } + /// # Safety /// - ptr points to a valid allocation of Self #[inline] diff --git a/src/machine/loader.rs b/src/machine/loader.rs index 0dfdfad2..07efb58a 100644 --- a/src/machine/loader.rs +++ b/src/machine/loader.rs @@ -305,11 +305,11 @@ impl<'a> LoadState<'a> for LiveLoadAndMachineState<'a> { #[inline(always)] fn evacuate(mut loader: Loader<'a, Self>) -> Result { if loader.payload.load_state.get_tag() != ArenaHeaderTag::Dropped { - loader - .payload - .load_state - .set_tag(ArenaHeaderTag::InactiveLoadState); - Ok(loader.payload.load_state) + loader + .payload + .load_state + .set_tag(ArenaHeaderTag::InactiveLoadState); + Ok(loader.payload.load_state) } else { unreachable!("we never evacuate after dropping") } @@ -323,7 +323,7 @@ impl<'a> LoadState<'a> for LiveLoadAndMachineState<'a> { #[inline(always)] fn reset_machine(loader: &mut Loader<'a, Self>) { if loader.payload.load_state.get_tag() != ArenaHeaderTag::Dropped { - loader.payload.load_state.set_tag(ArenaHeaderTag::Dropped); + loader.payload.load_state.drop_payload(); loader.reset_machine(); } } @@ -1788,11 +1788,8 @@ impl Machine { (HeapCellValueTag::Cons, cons_ptr) => { match_untyped_arena_ptr!(cons_ptr, (ArenaHeaderTag::LiveLoadState, payload) => { - unsafe { - std::ptr::drop_in_place( - payload.as_ptr() as *mut LiveLoadState, - ); - } + let mut payload = payload; + payload.drop_payload() } _ => {} ); diff --git a/src/machine/streams.rs b/src/machine/streams.rs index 61191205..b6281933 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -454,13 +454,25 @@ impl DerefMut for StreamLayout { macro_rules! arena_allocated_impl_for_stream { ($stream_type:ty, $stream_tag:ident) => { + impl $crate::arena::AllocateInArena<$stream_tag> for StreamLayout<$stream_type> { + fn arena_allocate(self, arena: &mut Arena) -> TypedArenaPtr<$stream_tag> { + $stream_tag::alloc(arena, core::mem::ManuallyDrop::new(self)) + } + } + impl ArenaAllocated for $stream_tag { - type Payload = StreamLayout<$stream_type>; + type Payload = core::mem::ManuallyDrop>; #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::$stream_tag } + + unsafe fn dealloc(ptr: std::ptr::NonNull>) { + let mut slab = unsafe { Box::from_raw(ptr.as_ptr()) }; + unsafe { std::mem::ManuallyDrop::drop(slab.payload()) }; + drop(slab); + } } }; } @@ -994,7 +1006,7 @@ impl Stream { past_end_of_stream, stream, .. - } = &mut **stream_layout; + } = &mut ***stream_layout; stream .get_mut() @@ -1068,7 +1080,7 @@ impl Stream { past_end_of_stream, stream, .. - } = &mut **stream_layout; + } = &mut ***stream_layout; let cursor_len = stream.get_ref().0.get_ref().len() as u64; cursor_position(past_end_of_stream, &stream.get_ref().0, cursor_len) @@ -1078,7 +1090,7 @@ impl Stream { past_end_of_stream, stream, .. - } = &mut **stream_layout; + } = &mut ***stream_layout; let cursor_len = stream.stream.get_ref().len() as u64; cursor_position(past_end_of_stream, &stream.stream, cursor_len) @@ -1090,7 +1102,7 @@ impl Stream { past_end_of_stream, stream, .. - } = &mut **stream_layout; + } = &mut ***stream_layout; match stream.get_ref().file.metadata() { Ok(metadata) => { @@ -1270,38 +1282,25 @@ impl Stream { Stream::NamedTls(ref mut tls_stream) => tls_stream.inner_mut().tls_stream.shutdown(), #[cfg(feature = "http")] Stream::HttpRead(ref mut http_stream) => { - unsafe { - http_stream.set_tag(ArenaHeaderTag::Dropped); - std::ptr::drop_in_place(&mut http_stream.inner_mut().body_reader as *mut _); - } + http_stream.drop_payload(); Ok(()) } #[cfg(feature = "http")] - Stream::HttpWrite(ref mut http_stream) => { - http_stream.inner_mut().drop(); - unsafe { - http_stream.set_tag(ArenaHeaderTag::Dropped); - std::ptr::drop_in_place(&mut http_stream.inner_mut().buffer as *mut _); - } + Stream::HttpWrite(mut http_stream) => { + http_stream.drop_payload(); Ok(()) } Stream::InputFile(mut file_stream) => { // close the stream by dropping the inner File. - unsafe { - file_stream.set_tag(ArenaHeaderTag::Dropped); - std::ptr::drop_in_place(&mut file_stream.inner_mut().file as *mut _); - } + file_stream.drop_payload(); Ok(()) } Stream::OutputFile(mut file_stream) => { // close the stream by dropping the inner File. - unsafe { - file_stream.set_tag(ArenaHeaderTag::Dropped); - std::ptr::drop_in_place(&mut file_stream.file as *mut _); - } + file_stream.drop_payload(); Ok(()) } diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index 17095a36..1145a450 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -6737,12 +6737,8 @@ impl Machine { (HeapCellValueTag::Cons, cons_ptr) => { match_untyped_arena_ptr!(cons_ptr, (ArenaHeaderTag::TcpListener, tcp_listener) => { - unsafe { - // dropping closes the instance. - std::ptr::drop_in_place(&mut tcp_listener as *mut _); - } + tcp_listener.drop_payload(); - tcp_listener.set_tag(ArenaHeaderTag::Dropped); return Ok(()); } _ => { From 689632b51ca32c0ec163c91ef1ab24301c77ee16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 15:47:50 +0200 Subject: [PATCH 44/45] cleanup the slab types - get rid of HeaderOrIdxPtr - add IndexPtrSlab - add UntypedArenaSlab - add to_untyped for converting a typed slab into an unsyped slab - remove TypedArenaPtr::new, as they shouldn't be created outside of this module --- src/arena.rs | 164 +++++++++++++++++++++++++++++---------------------- src/types.rs | 1 + 2 files changed, 95 insertions(+), 70 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index c532d24a..46d4d26a 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -296,14 +296,6 @@ where } impl TypedArenaPtr { - /// # 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 unsafe fn new(data: *mut T::Payload) -> Self { - unsafe { TypedArenaPtr(ptr::NonNull::new_unchecked(data)) } - } - #[inline] pub fn as_ptr(&self) -> *mut T::Payload { self.0.as_ptr() @@ -398,14 +390,14 @@ pub trait ArenaAllocated { /// # Safety /// - the caller must guarantee that the pointee type of UntypedArenaPtr is Self + /// - the pointer must be non-null unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr where Self::Payload: Sized, { - // safety: - // - allocated in an arena as from an UntypedArenaPtr - // - caller guarantees the type is correct - unsafe { TypedArenaPtr::new(ptr.payload_offset().cast_mut().cast::()) } + TypedArenaPtr(NonNull::new_unchecked( + ptr.payload_offset().cast_mut().cast::(), + )) } #[allow(clippy::missing_safety_doc)] @@ -417,20 +409,14 @@ pub trait ArenaAllocated { let slab = Box::new(TypedAllocSlab { slab: AllocSlab { next: arena.base.take(), - #[cfg(target_pointer_width = "32")] - _padding: 0, - header: HeaderOrIdxPtr { - header: ArenaHeader::build_with(size as u64, Self::tag()), - }, + header: ArenaHeader::build_with(size as u64, Self::tag()), }, payload: value, }); - let raw_box = Box::into_raw(slab); - // safety: Box::into_raw retuns a pointer to a valid allocation - let allocated_ptr = unsafe { TypedAllocSlab::to_typed_arena_ptr(raw_box) }; + let (allocated_ptr, untyped_slab) = slab.to_untyped(); - arena.base = Some(NonNull::new(raw_box.cast::()).unwrap()); + arena.base = Some(untyped_slab); allocated_ptr } @@ -655,94 +641,132 @@ impl ArenaAllocated for IndexPtr { /// # Safety /// - the caller must guarantee that the pointee type of UntypedArenaPtr is T + /// - the pointer must be non-null unsafe fn typed_ptr(ptr: UntypedArenaPtr) -> TypedArenaPtr { - unsafe { TypedArenaPtr::new(ptr.get_ptr().cast_mut().cast::()) } + TypedArenaPtr(NonNull::new_unchecked( + ptr.get_ptr().cast_mut().cast::(), + )) } #[inline] fn alloc(arena: &mut Arena, value: Self) -> TypedArenaPtr { - let slab = Box::new(AllocSlab { + let slab = Box::new(IndexPtrSlab { next: arena.base.take(), - #[cfg(target_pointer_width = "32")] - _padding: 0, - header: HeaderOrIdxPtr { idx_ptr: value }, + index_ptr: value, }); - let raw_box = Box::into_raw(slab); - let allocated_ptr = - unsafe { TypedArenaPtr::new(ptr::addr_of_mut!((*raw_box).header.idx_ptr)) }; - arena.base = Some(NonNull::new(raw_box).unwrap()); + let (allocated_ptr, untyped_slab) = slab.to_untyped(); + arena.base = Some(untyped_slab); allocated_ptr } /// # Safety /// - ptr points to an allocated slab of the correct kind unsafe fn dealloc(ptr: NonNull>) { - drop(unsafe { Box::from_raw(ptr.as_ptr().cast::()) }); + drop(unsafe { Box::from_raw(ptr.as_ptr().cast::()) }); } } #[repr(C)] -union HeaderOrIdxPtr { +#[derive(Debug)] +pub struct AllocSlab { + next: Option, header: ArenaHeader, - idx_ptr: IndexPtr, +} + +#[repr(C)] +#[derive(Debug)] +pub struct IndexPtrSlab { + next: Option, + index_ptr: IndexPtr, } const _: () = { - if std::mem::size_of::() != std::mem::size_of::() { - panic!("Size of ArenaHeader != IndexPtr") + if std::mem::align_of::() < std::mem::align_of::<*const ()>() { + panic!("alignment of AllocSlab is too low"); + } + + if std::mem::offset_of!(AllocSlab, header) % std::mem::align_of::<*const ()>() != 0 { + panic!("alignment of header not a multiple of pointers alignment"); + } + + if std::mem::offset_of!(AllocSlab, header) != std::mem::offset_of!(IndexPtrSlab, index_ptr) { + panic!("IndexPtrSlab.index_ptr and AllocSlab.header are at different offsets"); } }; -impl Debug for HeaderOrIdxPtr { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - unsafe { &self.header }.fmt(f) - } -} +impl IndexPtrSlab { + #[inline] + pub fn to_untyped(self: Box) -> (TypedArenaPtr, UntypedArenaSlab) { + let raw_box = Box::into_raw(self); -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) } + // safety: the pointer from Box::into_raw fullfills addr_of_mut's saftey requirements + let index_ptr_ptr = unsafe { ptr::addr_of_mut!((*raw_box).index_ptr) }; + let allocated_ptr = TypedArenaPtr( + // safety: the pointer points into a valid allocation so it is non null + unsafe { NonNull::new_unchecked(index_ptr_ptr) }, + ); + + let untyped_arena = UntypedArenaSlab { + // safety: pointer from Box::into_raw is never null + slab: unsafe { NonNull::new_unchecked(raw_box.cast::()) }, + }; + + (allocated_ptr, untyped_arena) } } #[repr(C)] -#[derive(Clone, Debug)] -pub struct AllocSlab { - next: Option>, - #[cfg(target_pointer_width = "32")] - _padding: u32, - header: HeaderOrIdxPtr, -} - -#[repr(C)] -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct TypedAllocSlab { slab: AllocSlab, payload: T::Payload, } impl TypedAllocSlab { + pub fn tag(&self) -> ArenaHeaderTag { + self.slab.header.tag() + } + pub fn payload(&mut self) -> &mut T::Payload { &mut self.payload } - /// # Safety - /// - ptr points to a valid allocation of Self #[inline] - pub unsafe fn to_typed_arena_ptr(ptr: *mut Self) -> TypedArenaPtr { - // safety: - // - this is the arena allocation of corresponding type - unsafe { TypedArenaPtr::new(addr_of_mut!((*ptr).payload)) } + pub fn to_untyped(self: Box) -> (TypedArenaPtr, UntypedArenaSlab) { + let raw_box = Box::into_raw(self); + + // safety: the pointer from Box::into_raw fullfills addr_of_mut's saftey requirements + let payload_ptr = unsafe { addr_of_mut!((*raw_box).payload) }; + + ( + TypedArenaPtr(unsafe { + // safety: the pointer points into a valid allocation so it is non null + ptr::NonNull::new_unchecked(payload_ptr) + }), + UntypedArenaSlab { + // safety: pointer from Box::into_raw is never null + slab: unsafe { NonNull::new_unchecked(raw_box.cast::()) }, + }, + ) + } +} + +#[derive(Debug)] +#[repr(transparent)] +pub struct UntypedArenaSlab { + slab: NonNull, +} + +impl Drop for UntypedArenaSlab { + fn drop(&mut self) { + unsafe { drop_slab_in_place(self.slab) }; } } #[derive(Debug)] pub struct Arena { - base: Option>, + base: Option, pub f64_tbl: Arc, } @@ -767,7 +791,7 @@ unsafe fn drop_slab_in_place(value: NonNull) { }; } - match (unsafe { value.as_ref() }).header.header.tag() { + match (unsafe { value.as_ref() }).header.tag() { ArenaHeaderTag::Integer => { drop_typed_slab_in_place!(Integer, value); } @@ -839,18 +863,18 @@ unsafe fn drop_slab_in_place(value: NonNull) { impl Drop for Arena { fn drop(&mut self) { + // we un-nest UntypedArenaSlab to prevent stackoverflow due to the recursive drop + let mut ptr = self.base.take(); - while let Some(slab) = ptr { - unsafe { - ptr = slab.as_ref().next; - drop_slab_in_place(slab); - } + while let Some(mut slab) = ptr { + ptr = unsafe { slab.slab.as_mut() }.next.take(); + drop(slab); } } } -const_assert!(mem::size_of::() == 16); +const_assert!(mem::size_of::() <= 24); const_assert!(mem::size_of::>() == 8); #[cfg(test)] diff --git a/src/types.rs b/src/types.rs index 07328a07..49ff0a6e 100644 --- a/src/types.rs +++ b/src/types.rs @@ -710,6 +710,7 @@ impl UntypedArenaPtr { /// # Safety /// - this UntypedArenaPtr actuall pointee type is T + /// - the pointer must be non-null #[inline] pub unsafe fn as_typed_ptr(self) -> TypedArenaPtr where From 5d03be4b092e113d5bc420aa83704bd7d6be075e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bennet=20Ble=C3=9Fmann?= Date: Sun, 7 Jul 2024 15:51:22 +0200 Subject: [PATCH 45/45] fix leak of slaps with dropped payload --- src/arena.rs | 22 +++++++++++++++++----- src/machine/streams.rs | 11 ++++++++++- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 46d4d26a..a3449d44 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -197,6 +197,7 @@ pub enum ArenaHeaderTag { } #[bitfield] +#[repr(align(8))] #[derive(Copy, Clone, Debug)] pub struct ArenaHeader { #[allow(dead_code)] @@ -590,7 +591,16 @@ impl ArenaAllocated for LiveLoadState { unsafe fn dealloc(ptr: NonNull>) { let mut slab = unsafe { Box::from_raw(ptr.as_ptr()) }; - unsafe { ManuallyDrop::drop(&mut slab.payload) }; + + match slab.tag() { + ArenaHeaderTag::LiveLoadState | ArenaHeaderTag::InactiveLoadState => { + unsafe { ManuallyDrop::drop(&mut slab.payload) }; + } + ArenaHeaderTag::Dropped => {} + _ => { + unreachable!() + } + } drop(slab); } } @@ -710,6 +720,7 @@ impl IndexPtrSlab { let untyped_arena = UntypedArenaSlab { // safety: pointer from Box::into_raw is never null slab: unsafe { NonNull::new_unchecked(raw_box.cast::()) }, + tag: ::tag(), }; (allocated_ptr, untyped_arena) @@ -747,20 +758,21 @@ impl TypedAllocSlab { UntypedArenaSlab { // safety: pointer from Box::into_raw is never null slab: unsafe { NonNull::new_unchecked(raw_box.cast::()) }, + tag: T::tag(), }, ) } } #[derive(Debug)] -#[repr(transparent)] pub struct UntypedArenaSlab { slab: NonNull, + tag: ArenaHeaderTag, } impl Drop for UntypedArenaSlab { fn drop(&mut self) { - unsafe { drop_slab_in_place(self.slab) }; + unsafe { drop_slab_in_place(self.slab, self.tag) }; } } @@ -784,14 +796,14 @@ impl Arena { } } -unsafe fn drop_slab_in_place(value: NonNull) { +unsafe fn drop_slab_in_place(value: NonNull, tag: ArenaHeaderTag) { macro_rules! drop_typed_slab_in_place { ($payload: ty, $value: expr) => { <$payload as ArenaAllocated>::dealloc($value.cast::>()) }; } - match (unsafe { value.as_ref() }).header.tag() { + match tag { ArenaHeaderTag::Integer => { drop_typed_slab_in_place!(Integer, value); } diff --git a/src/machine/streams.rs b/src/machine/streams.rs index b6281933..d7df644d 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -470,7 +470,16 @@ macro_rules! arena_allocated_impl_for_stream { unsafe fn dealloc(ptr: std::ptr::NonNull>) { let mut slab = unsafe { Box::from_raw(ptr.as_ptr()) }; - unsafe { std::mem::ManuallyDrop::drop(slab.payload()) }; + + match slab.tag() { + ArenaHeaderTag::$stream_tag => { + unsafe { std::mem::ManuallyDrop::drop(slab.payload()) }; + } + ArenaHeaderTag::Dropped => {} + _ => { + unreachable!() + } + } drop(slab); } }