From 6aa3c7d5d6ee84db5bbe63c201ed744e2452a170 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bennet=20Ble=C3=9Fmann?= Date: Sat, 29 Jul 2023 11:08:32 +0200 Subject: [PATCH] handle atom table resize * bumping serial_test dev-dependency due to broken should_panic handling in old version --- Cargo.lock | 36 ++++++++++++---- Cargo.toml | 2 +- src/atom_table.rs | 93 +++++++++++++++++++++++++----------------- tests/scryer/issues.rs | 20 ++++++++- 4 files changed, 103 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 967181ad..d681658f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -349,6 +349,19 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "dashmap" +version = "5.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6943ae99c34386c84a470c499d3414f66502a41340aa895406e0d2e4a207b91d" +dependencies = [ + "cfg-if", + "hashbrown 0.14.0", + "lock_api", + "once_cell", + "parking_lot_core 0.9.8", +] + [[package]] name = "dashu" version = "0.3.1" @@ -794,6 +807,12 @@ version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" +[[package]] +name = "hashbrown" +version = "0.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2c6201b9ff9fd90a5a3bac2e56a830d0caa509576f0e503818ee82c181b3437a" + [[package]] name = "heck" version = "0.3.3" @@ -999,7 +1018,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99" dependencies = [ "autocfg", - "hashbrown", + "hashbrown 0.12.3", ] [[package]] @@ -2050,24 +2069,27 @@ dependencies = [ [[package]] name = "serial_test" -version = "0.5.1" +version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e0bccbcf40c8938196944a3da0e133e031a33f4d6b72db3bda3cc556e361905d" +checksum = "0e56dd856803e253c8f298af3f4d7eb0ae5e23a737252cd90bb4f3b435033b2d" dependencies = [ + "dashmap", + "futures", "lazy_static", - "parking_lot 0.11.2", + "log", + "parking_lot 0.12.1", "serial_test_derive", ] [[package]] name = "serial_test_derive" -version = "0.5.1" +version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b2acd6defeddb41eb60bb468f8825d0cfd0c2a76bc03bfd235b6a1dc4f6a1ad5" +checksum = "91d129178576168c589c9ec973feedf7d3126c01ac2bf08795109aa35b69fb8f" dependencies = [ "proc-macro2", "quote", - "syn 1.0.109", + "syn 2.0.22", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 194d4482..b5f330f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,7 +74,7 @@ rand = "0.8.5" [dev-dependencies] assert_cmd = "1.0.3" predicates-core = "1.0.2" -serial_test = "0.5.1" +serial_test = "2.0.0" [patch.crates-io] modular-bitfield = { git = "https://github.com/mthom/modular-bitfield" } diff --git a/src/atom_table.rs b/src/atom_table.rs index 9f480139..df3e1e44 100644 --- a/src/atom_table.rs +++ b/src/atom_table.rs @@ -37,59 +37,61 @@ impl From for Atom { } } -#[cfg(test)] -use std::cell::RefCell; - const ATOM_TABLE_INIT_SIZE: usize = 1 << 16; const ATOM_TABLE_ALIGN: usize = 8; #[cfg(test)] thread_local! { - static ATOM_TABLE_BUF_BASE: RefCell<*const u8> = RefCell::new(ptr::null_mut()); + static ATOM_TABLE_BUF_BASE: std::cell::RefCell<*const u8> = std::cell::RefCell::new(ptr::null_mut()); } #[cfg(not(test))] -static mut ATOM_TABLE_BUF_BASE: *const u8 = ptr::null_mut(); +static ATOM_TABLE_BUF_BASE: std::sync::atomic::AtomicPtr = + std::sync::atomic::AtomicPtr::new(ptr::null_mut()); +fn set_atom_tbl_buf_base(old_ptr: *const u8, new_ptr: *const u8) -> Result<(), *const u8> { #[cfg(test)] -fn set_atom_tbl_buf_base(ptr: *const u8) { + { ATOM_TABLE_BUF_BASE.with(|atom_table_buf_base| { let mut borrow = atom_table_buf_base.borrow_mut(); - assert!( - borrow.is_null() || ptr.is_null(), - "Overwriting atom table base pointer!" - ); - *borrow = ptr; - }); + if *borrow != old_ptr { + Err(*borrow) + } else { + *borrow = new_ptr; + Ok(()) + } + })?; + }; + #[cfg(not(test))] + { + ATOM_TABLE_BUF_BASE + .compare_exchange( + old_ptr.cast_mut(), + new_ptr.cast_mut(), + std::sync::atomic::Ordering::Relaxed, + std::sync::atomic::Ordering::Relaxed, + ) + .map_err(|ptr| ptr.cast_const()) + }?; + Ok(()) } -#[cfg(test)] pub(crate) fn get_atom_tbl_buf_base() -> *const u8 { + #[cfg(test)] + { ATOM_TABLE_BUF_BASE.with(|atom_table_buf_base| *atom_table_buf_base.borrow()) } - #[cfg(not(test))] -fn set_atom_tbl_buf_base(ptr: *const u8) { - unsafe { - // FIXME: to prevent a toctou race-condition an atomic compare_exchange or a global lock should be used - assert!( - ATOM_TABLE_BUF_BASE.is_null() || ptr.is_null(), - "Overwriting atom table base pointer!" - ); - ATOM_TABLE_BUF_BASE = ptr; + { + ATOM_TABLE_BUF_BASE.load(std::sync::atomic::Ordering::Relaxed) } } -#[cfg(not(test))] -pub(crate) fn get_atom_tbl_buf_base() -> *const u8 { - unsafe { ATOM_TABLE_BUF_BASE } -} - #[test] -#[should_panic(expected = "Overwriting atom table base pointer!")] +#[should_panic(expected = "Overwriting atom table base pointer")] fn atomtable_is_not_concurrency_safe() { - let table_a = AtomTable::new(); - let table_b = AtomTable::new(); + let _table_a = AtomTable::new(); + let _table_b = AtomTable::new(); } impl RawBlockTraits for AtomTable { @@ -256,9 +258,17 @@ pub struct AtomTable { pub table: IndexSet, } +#[cold] +fn atom_table_base_pointer_missmatch(expected: *const u8, got: *const u8) -> ! { + assert_eq!(expected, got, "Overwriting atom table base pointer, expected old value to be {expected:p}, but found {got:p}"); + unreachable!("This should only be called in a case of a missmatch as such the assert_eq should have failed!") +} + impl Drop for AtomTable { fn drop(&mut self) { - set_atom_tbl_buf_base(ptr::null()); + if let Err(got) = set_atom_tbl_buf_base(self.block.base, ptr::null()) { + atom_table_base_pointer_missmatch(self.block.base, got); + } self.block.deallocate(); } } @@ -266,13 +276,17 @@ impl Drop for AtomTable { impl AtomTable { #[inline] pub fn new() -> Self { - let table = Self { - block: RawBlock::new(), - table: IndexSet::new(), - }; + let mut block = RawBlock::new(); - set_atom_tbl_buf_base(table.block.base); - table + if let Err(got) = set_atom_tbl_buf_base(ptr::null(), block.base) { + block.deallocate(); + atom_table_base_pointer_missmatch(ptr::null(), got); + } + + Self { + block, + table: IndexSet::new(), + } } #[inline] @@ -307,8 +321,11 @@ impl AtomTable { ptr = self.block.alloc(size); if ptr.is_null() { + let old_base = self.block.base; self.block.grow(); - set_atom_tbl_buf_base(self.block.base); + if let Err(got) = set_atom_tbl_buf_base(old_base, self.block.base) { + atom_table_base_pointer_missmatch(old_base, got); + } } else { break; } diff --git a/tests/scryer/issues.rs b/tests/scryer/issues.rs index f1735e78..ffa39148 100644 --- a/tests/scryer/issues.rs +++ b/tests/scryer/issues.rs @@ -1,4 +1,5 @@ use crate::helper::{load_module_test, run_top_level_test_no_args, run_top_level_test_with_args}; +use scryer_prolog::machine::Machine; use serial_test::serial; // issue #857 @@ -128,10 +129,12 @@ fn compound_goal() { // issue #815 #[test] fn no_stutter() { - run_top_level_test_no_args("write(a), write(b), false.\n\ + run_top_level_test_no_args( + "write(a), write(b), false.\n\ halt.\n\ ", - "ab false.\n") + "ab false.\n", + ) } /* @@ -168,3 +171,16 @@ fn call_0() { " error(existence_error(procedure,call/0),call/0).\n", ); } + +// issue #1206 +#[serial] +#[test] +#[should_panic(expected = "Overwriting atom table base pointer")] +fn atomtable_is_not_concurrency_safe() { + // this is basically the same test as scryer_prolog::atom_table::atomtable_is_not_concurrency_safe + // but for this integration test scryer_prolog is compiled with cfg!(not(test)) while for the unit test it is colpiled with cfg!(test) + // as the atom table implementation differ between cfg!(test) and cfg!(not(test)) both test serve a pourpose + // Note: this integration test itself is compiled with cfg!(test) independent of scryer_prolog itself + let _machine_a = Machine::with_test_streams(); + let _machine_b = Machine::with_test_streams(); +} -- 2.54.0