From a6157a926dbf9d470ac656013b1b94162602dec3 Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Wed, 24 Apr 2024 20:28:06 -0600 Subject: [PATCH 1/5] improve use of unsafe Rust in arena.rs (#2391) --- src/arena.rs | 309 ++++++++++++++++------------------------- src/machine/streams.rs | 17 +-- 2 files changed, 119 insertions(+), 207 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index 172e6e9d5..d24acfe99 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -11,7 +11,6 @@ use crate::read::*; use crate::parser::dashu::{Integer, Rational}; use ordered_float::OrderedFloat; -use std::alloc; use std::cell::UnsafeCell; use std::fmt; use std::hash::{Hash, Hasher}; @@ -43,6 +42,29 @@ macro_rules! float_alloc { }}; } +pub fn header_offset_from_payload() -> usize { + let payload_offset = mem::offset_of!(TypedAllocSlab, payload); + let slab_offset = mem::offset_of!(TypedAllocSlab, slab); + let header_offset = slab_offset + mem::offset_of!(AllocSlab, header); + + debug_assert!(payload_offset > header_offset); + payload_offset - header_offset +} + +pub fn ptr_to_allocated(slab: &mut AllocSlab) -> TypedArenaPtr { + let typed_slab: &mut TypedAllocSlab = unsafe { mem::transmute(slab) }; + typed_slab.to_typed_arena_ptr() +} + +#[macro_export] +macro_rules! gen_ptr_to_allocated { + ($payload: ty) => { + fn ptr_to_allocated(slab: &mut AllocSlab) -> TypedArenaPtr<$payload> { + ptr_to_allocated::<$payload>(slab) + } + }; +} + use std::sync::Arc; use std::sync::Mutex; use std::sync::Weak; @@ -196,6 +218,7 @@ pub enum ArenaHeaderTag { #[bitfield] #[derive(Copy, Clone, Debug)] pub struct ArenaHeader { + #[allow(dead_code)] size: B56, m: bool, tag: ArenaHeaderTag, @@ -291,16 +314,12 @@ impl TypedArenaPtr { #[inline] pub fn header_ptr(&self) -> *const ArenaHeader { - let mut ptr = self.as_ptr() as *const u8 as usize; - ptr -= T::header_offset_from_payload(); // mem::size_of::<*const ArenaHeader>(); - ptr as *const ArenaHeader + unsafe { self.as_ptr().byte_sub(T::header_offset_from_payload()) as *const _ } } #[inline] fn header_ptr_mut(&mut self) -> *mut ArenaHeader { - let mut ptr = self.as_ptr() as *const u8 as usize; - ptr -= T::header_offset_from_payload(); // mem::size_of::<*const ArenaHeader>(); - ptr as *mut ArenaHeader + unsafe { self.as_ptr().byte_sub(T::header_offset_from_payload()) as *mut _ } } #[inline] @@ -339,35 +358,31 @@ pub trait ArenaAllocated: Sized { type PtrToAllocated; fn tag() -> ArenaHeaderTag; - fn size(&self) -> usize; - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated; + fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated; fn header_offset_from_payload() -> usize { - mem::size_of::() + header_offset_from_payload::() } #[allow(clippy::missing_safety_doc)] - unsafe fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { - let size = value.size() + mem::size_of::(); - - #[cfg(target_pointer_width = "32")] - let align = mem::align_of::() * 2; - - #[cfg(target_pointer_width = "64")] - let align = mem::align_of::(); - let layout = alloc::Layout::from_size_align_unchecked(size, align); - - let slab = alloc::alloc(layout) as *mut AllocSlab; + fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + let size = mem::size_of::>(); + let slab = Box::new(TypedAllocSlab { + slab: AllocSlab { + next: arena.base.take(), + #[cfg(target_pointer_width = "32")] + _padding: 0, + header: ArenaHeader::build_with(size as u64, Self::tag()), + }, + payload: value, + }); - (*slab).next = arena.base; - (*slab).header = ArenaHeader::build_with(value.size() as u64, Self::tag()); + let mut untyped_slab = unsafe { Box::from_raw(Box::into_raw(slab) as *mut AllocSlab) }; + let allocated_ptr = Self::ptr_to_allocated(untyped_slab.as_mut()); - let offset = (*slab).payload_offset(); - let result = value.copy_to_arena(offset); + arena.base = Some(untyped_slab); - arena.base = slab; - - result + allocated_ptr } } @@ -505,141 +520,69 @@ impl fmt::Display for F64Offset { impl ArenaAllocated for Integer { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(Integer); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Integer } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for Rational { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(Rational); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::Rational } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for LiveLoadState { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(LiveLoadState); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::LiveLoadState } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for TcpListener { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(TcpListener); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::TcpListener } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } #[cfg(feature = "http")] impl ArenaAllocated for HttpListener { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(HttpListener); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpListener } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } #[cfg(feature = "http")] impl ArenaAllocated for HttpResponse { type PtrToAllocated = TypedArenaPtr; + gen_ptr_to_allocated!(HttpResponse); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::HttpResponse } - - #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } - } } impl ArenaAllocated for IndexPtr { @@ -651,17 +594,8 @@ impl ArenaAllocated for IndexPtr { } #[inline] - fn size(&self) -> usize { - mem::size_of::() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - ptr::write(dst, self); - TypedArenaPtr::new(dst) - } + fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated { + TypedArenaPtr::new(unsafe { mem::transmute(slab.header) }) } #[inline] @@ -669,38 +603,48 @@ impl ArenaAllocated for IndexPtr { 0 } - unsafe fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { - let size = mem::size_of::(); - - let align = mem::align_of::(); - let layout = alloc::Layout::from_size_align_unchecked(size, align); - - let slab = alloc::alloc(layout) as *mut AllocSlab; - - (*slab).next = arena.base; - - let result = value.copy_to_arena( - &(*slab).header as *const crate::arena::ArenaHeader - as *mut crate::machine::machine_indices::IndexPtr, - ); - arena.base = slab; - - result + #[inline] + fn alloc(arena: &mut Arena, value: Self) -> Self::PtrToAllocated { + let mut slab = Box::new(AllocSlab { + next: arena.base.take(), + #[cfg(target_pointer_width = "32")] + padding: 0, + header: unsafe { mem::transmute(value) }, + }); + + let allocated_ptr = + TypedArenaPtr::new(unsafe { mem::transmute(ptr::addr_of_mut!(slab.header)) }); + arena.base = Some(slab); + allocated_ptr } } #[repr(C)] -#[derive(Clone, Copy, Debug)] -struct AllocSlab { - next: *mut AllocSlab, +#[derive(Clone, Debug)] +pub struct AllocSlab { + next: Option>, #[cfg(target_pointer_width = "32")] _padding: u32, header: ArenaHeader, } +#[repr(C)] +#[derive(Clone, Debug)] +pub struct TypedAllocSlab { + slab: AllocSlab, + payload: Payload, +} + +impl TypedAllocSlab { + #[inline] + pub fn to_typed_arena_ptr(&mut self) -> TypedArenaPtr { + TypedArenaPtr::new(&mut self.payload as *mut _) + } +} + #[derive(Debug)] pub struct Arena { - base: *mut AllocSlab, + base: Option>, pub f64_tbl: Arc, } @@ -712,72 +656,77 @@ impl Arena { #[inline] pub fn new() -> Self { Arena { - base: ptr::null_mut(), + base: None, f64_tbl: F64Table::new(), } } } unsafe fn drop_slab_in_place(value: &mut AllocSlab) { - use crate::parser::char_reader::CharReader; + macro_rules! drop_typed_slab_in_place { + ($payload: ty, $value: expr) => { + let slab: &mut TypedAllocSlab<$payload> = mem::transmute($value); + ptr::drop_in_place(ptr::addr_of_mut!(slab.payload)); + }; + } match value.header.tag() { ArenaHeaderTag::Integer => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(Integer, value); } ArenaHeaderTag::Rational => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(Rational, value); } ArenaHeaderTag::InputFileStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(InputFileStream, value); } ArenaHeaderTag::OutputFileStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(OutputFileStream, value); } ArenaHeaderTag::NamedTcpStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(NamedTcpStream, value); } ArenaHeaderTag::NamedTlsStream => { #[cfg(feature = "tls")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(NamedTlsStream, value); } ArenaHeaderTag::HttpReadStream => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(HttpReadStream, value); } ArenaHeaderTag::HttpWriteStream => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(HttpWriteStream, value); } ArenaHeaderTag::ReadlineStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(ReadlineStream, value); } ArenaHeaderTag::StaticStringStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StaticStringStream, value); } ArenaHeaderTag::ByteStream => { - ptr::drop_in_place(value.payload_offset::>>()); + drop_typed_slab_in_place!(ByteStream, value); } ArenaHeaderTag::LiveLoadState | ArenaHeaderTag::InactiveLoadState => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(LiveLoadState, value); } ArenaHeaderTag::Dropped => {} ArenaHeaderTag::TcpListener => { - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(TcpListener, value); } ArenaHeaderTag::HttpListener => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(HttpListener, value); } ArenaHeaderTag::HttpResponse => { #[cfg(feature = "http")] - ptr::drop_in_place(value.payload_offset::()); + drop_typed_slab_in_place!(HttpResponse, value); } ArenaHeaderTag::StandardOutputStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StandardOutputStream, value); } ArenaHeaderTag::StandardErrorStream => { - ptr::drop_in_place(value.payload_offset::>()); + drop_typed_slab_in_place!(StandardErrorStream, value); } ArenaHeaderTag::NullStream | ArenaHeaderTag::IndexPtrUndefined @@ -789,44 +738,20 @@ unsafe fn drop_slab_in_place(value: &mut AllocSlab) { impl Drop for Arena { fn drop(&mut self) { - let mut ptr = self.base; + let mut ptr = self.base.take(); - while !ptr.is_null() { + while let Some(mut slab) = ptr { unsafe { - let ptr_r = &*ptr; - - let layout = alloc::Layout::from_size_align_unchecked( - ptr_r.slab_size(), - mem::align_of::(), - ); - - drop_slab_in_place(&mut *ptr); - - let next_ptr = ptr_r.next; - alloc::dealloc(ptr as *mut u8, layout); - ptr = next_ptr; + drop_slab_in_place(&mut slab); + ptr = slab.next; } } - self.base = ptr::null_mut(); + self.base = None; } } const_assert!(mem::size_of::() == 16); - -impl AllocSlab { - #[inline] - fn slab_size(&self) -> usize { - self.header.size() as usize + mem::size_of::() - } - - fn payload_offset(&self) -> *mut T { - // This looks really scary, should this method be marked as unsafe? - // Also, this seems to cause UB. - unsafe { (self as *const AllocSlab).add(1) as *mut T } - } -} - const_assert!(mem::size_of::>() == 8); #[cfg(test)] diff --git a/src/machine/streams.rs b/src/machine/streams.rs index ba3659dd2..c2adb47e3 100644 --- a/src/machine/streams.rs +++ b/src/machine/streams.rs @@ -455,25 +455,12 @@ macro_rules! arena_allocated_impl_for_stream { impl ArenaAllocated for StreamLayout<$stream_type> { type PtrToAllocated = TypedArenaPtr>; + gen_ptr_to_allocated!(StreamLayout<$stream_type>); + #[inline] fn tag() -> ArenaHeaderTag { ArenaHeaderTag::$stream_tag } - - #[inline] - fn size(&self) -> usize { - mem::size_of::>() - } - - #[allow(clippy::not_unsafe_ptr_arg_deref)] - #[inline] - fn copy_to_arena(self, dst: *mut Self) -> Self::PtrToAllocated { - unsafe { - // Miri seems to hit this a lot - ptr::write(dst, self); - TypedArenaPtr::new(dst as *mut Self) - } - } } }; } From 8fe1c206d33db6477f044caad25bd67b6f8c80ac Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Wed, 24 Apr 2024 20:33:42 -0600 Subject: [PATCH 2/5] bump required rust version for use of newly introduced macros --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 2af52e257..6753ee013 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,7 @@ license = "BSD-3-Clause" keywords = ["prolog", "prolog-interpreter", "prolog-system"] categories = ["command-line-utilities"] build = "build/main.rs" -rust-version = "1.70" +rust-version = "1.77" [lib] crate-type = ["cdylib", "rlib"] From be23c74b17481a5f19c3564e67dcea1b8da0ae79 Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Wed, 24 Apr 2024 21:21:25 -0600 Subject: [PATCH 3/5] fix wasm tests in unsafe_improvements branch --- src/arena.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index d24acfe99..bd4d294c8 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -595,7 +595,7 @@ impl ArenaAllocated for IndexPtr { #[inline] fn ptr_to_allocated(slab: &mut AllocSlab) -> Self::PtrToAllocated { - TypedArenaPtr::new(unsafe { mem::transmute(slab.header) }) + TypedArenaPtr::new(ptr::addr_of_mut!(slab.header) as *mut _) } #[inline] @@ -608,7 +608,7 @@ impl ArenaAllocated for IndexPtr { let mut slab = Box::new(AllocSlab { next: arena.base.take(), #[cfg(target_pointer_width = "32")] - padding: 0, + _padding: 0, header: unsafe { mem::transmute(value) }, }); @@ -666,7 +666,7 @@ unsafe fn drop_slab_in_place(value: &mut AllocSlab) { macro_rules! drop_typed_slab_in_place { ($payload: ty, $value: expr) => { let slab: &mut TypedAllocSlab<$payload> = mem::transmute($value); - ptr::drop_in_place(ptr::addr_of_mut!(slab.payload)); + ptr::drop_in_place(&mut slab.payload); }; } From 5e1effc69ad590be7ed459ab739fe1ea2ba51ffd Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Thu, 25 Apr 2024 20:05:22 -0600 Subject: [PATCH 4/5] fix failing clippy checks, remove unnecessary code --- src/arena.rs | 12 ++---------- src/machine/machine_indices.rs | 8 +++++--- src/machine/system_calls.rs | 14 ++++++-------- src/parser/char_reader.rs | 7 +++---- 4 files changed, 16 insertions(+), 25 deletions(-) diff --git a/src/arena.rs b/src/arena.rs index bd4d294c8..7bae991cf 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -24,10 +24,7 @@ use std::sync::RwLock; macro_rules! arena_alloc { ($e:expr, $arena:expr) => {{ let result = $e; - #[allow(unused_unsafe)] - unsafe { - ArenaAllocated::alloc($arena, result) - } + ArenaAllocated::alloc($arena, result) }}; } @@ -35,10 +32,7 @@ macro_rules! arena_alloc { macro_rules! float_alloc { ($e:expr, $arena:expr) => {{ let result = $e; - #[allow(unused_unsafe)] - unsafe { - $arena.f64_tbl.build_with(result).as_ptr() - } + unsafe { $arena.f64_tbl.build_with(result).as_ptr() } }}; } @@ -746,8 +740,6 @@ impl Drop for Arena { ptr = slab.next; } } - - self.base = None; } } diff --git a/src/machine/machine_indices.rs b/src/machine/machine_indices.rs index 1ae2527b7..830cc91f2 100644 --- a/src/machine/machine_indices.rs +++ b/src/machine/machine_indices.rs @@ -298,9 +298,11 @@ impl IndexStore { _ => self .get_meta_predicate_spec(key.0, key.1, &compilation_target) .map(|meta_specs| { - meta_specs.iter().find(|meta_spec| match meta_spec { - MetaSpec::Colon | MetaSpec::RequiresExpansionWithArgument(_) => true, - _ => false, + meta_specs.iter().find(|meta_spec| { + matches!( + meta_spec, + MetaSpec::Colon | MetaSpec::RequiresExpansionWithArgument(_) + ) }) }) .map(|meta_spec_opt| meta_spec_opt.is_some()) diff --git a/src/machine/system_calls.rs b/src/machine/system_calls.rs index 8dafa43e5..80b579e79 100644 --- a/src/machine/system_calls.rs +++ b/src/machine/system_calls.rs @@ -428,14 +428,12 @@ impl BrentAlgState { pstr_chars = pstr.as_str_from(n).chars().count() - 1; - if heap[h].get_tag() == HeapCellValueTag::PStrOffset { - if heap[h_offset].get_tag() == HeapCellValueTag::CStr { - return if pstr_chars < max_steps { - CycleSearchResult::ProperList(pstr_chars + 1) - } else { - let offset = max_steps as usize + n; - CycleSearchResult::PStrLocation(max_steps, h_offset, offset) - } + if heap[h].get_tag() == HeapCellValueTag::PStrOffset && heap[h_offset].get_tag() == HeapCellValueTag::CStr { + return if pstr_chars < max_steps { + CycleSearchResult::ProperList(pstr_chars + 1) + } else { + let offset = max_steps + n; + CycleSearchResult::PStrLocation(max_steps, h_offset, offset) } } diff --git a/src/parser/char_reader.rs b/src/parser/char_reader.rs index a6dd75db4..bc9c16010 100644 --- a/src/parser/char_reader.rs +++ b/src/parser/char_reader.rs @@ -327,12 +327,11 @@ impl Read for CharReader { return self.inner.read_vectored(bufs); } - let nread = { - self.refresh_buffer()?; - (&self.buf[self.pos..]).read_vectored(bufs)? - }; + self.refresh_buffer()?; + let nread = (&self.buf[self.pos..]).read_vectored(bufs)?; self.consume(nread); + Ok(nread) } } From 79bc2d9c68da2257a94eb22ef8717a304173e344 Mon Sep 17 00:00:00 2001 From: Mark Thom Date: Mon, 29 Apr 2024 15:40:05 -0600 Subject: [PATCH 5/5] remove 1.70 version CI job --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e1d8177a..9bbf50b6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,7 +47,6 @@ jobs: # FIXME(issue #2138): run wasm tests, failing to run since https://github.com/mthom/scryer-prolog/pull/2137 removed wasm-pack - { os: ubuntu-22.04, rust-version: nightly, target: 'wasm32-unknown-unknown', publish: true, args: '--no-default-features' , test-args: '--no-run --no-default-features' } # rust versions - - { os: ubuntu-22.04, rust-version: "1.70", target: 'x86_64-unknown-linux-gnu'} - { 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'} defaults: