From 8020991257ea5272a520bbb8a8920dce33afce5c Mon Sep 17 00:00:00 2001 From: snek Date: Mon, 6 Jan 2025 13:59:24 +0100 Subject: [PATCH] fix: let cppgc align to 16 bytes (#1677) --- examples/cppgc-object.rs | 2 +- examples/cppgc.rs | 2 +- src/binding.cc | 16 +- src/cppgc.rs | 13 +- src/support.h | 4 +- tests/test_cppgc.rs | 330 ++++++++++++++++++++++----------------- 6 files changed, 214 insertions(+), 153 deletions(-) diff --git a/examples/cppgc-object.rs b/examples/cppgc-object.rs index 4dc44feef6..8f09ea6910 100644 --- a/examples/cppgc-object.rs +++ b/examples/cppgc-object.rs @@ -27,7 +27,7 @@ fn main() { v8::V8::initialize_platform(platform.clone()); v8::V8::initialize(); - v8::cppgc::initalize_process(platform.clone()); + v8::cppgc::initialize_process(platform.clone()); { let heap = diff --git a/examples/cppgc.rs b/examples/cppgc.rs index c8ac4731cd..21e55310fa 100644 --- a/examples/cppgc.rs +++ b/examples/cppgc.rs @@ -46,7 +46,7 @@ fn main() { let platform = v8::new_default_platform(0, false).make_shared(); v8::V8::initialize_platform(platform.clone()); v8::V8::initialize(); - v8::cppgc::initalize_process(platform.clone()); + v8::cppgc::initialize_process(platform.clone()); { // Create a managed heap. diff --git a/src/binding.cc b/src/binding.cc index 15b7cced6c..a86535e0ec 100644 --- a/src/binding.cc +++ b/src/binding.cc @@ -3976,9 +3976,19 @@ void cppgc__heap__collect_garbage_for_testing( heap->CollectGarbageForTesting(stack_state); } -RustObj* cppgc__make_garbage_collectable(v8::CppHeap* heap, size_t size) { - return cppgc::MakeGarbageCollected(heap->GetAllocationHandle(), - cppgc::AdditionalBytes(size)); +class alignas(16) RustObjButAlign16 : public RustObj {}; + +RustObj* cppgc__make_garbage_collectable(v8::CppHeap* heap, size_t size, + size_t alignment) { + if (alignment <= 8) { + return cppgc::MakeGarbageCollected(heap->GetAllocationHandle(), + cppgc::AdditionalBytes(size)); + } + if (alignment <= 16) { + return cppgc::MakeGarbageCollected( + heap->GetAllocationHandle(), cppgc::AdditionalBytes(size)); + } + return nullptr; } void cppgc__Visitor__Trace__Member(cppgc::Visitor* visitor, diff --git a/src/cppgc.rs b/src/cppgc.rs index 215c7ba5ba..96f54d35f8 100644 --- a/src/cppgc.rs +++ b/src/cppgc.rs @@ -27,6 +27,7 @@ extern "C" { fn cppgc__make_garbage_collectable( heap: *mut Heap, size: usize, + alignment: usize, ) -> *mut RustObj; fn cppgc__heap__enable_detached_garbage_collections_for_testing( @@ -145,7 +146,7 @@ unsafe fn get_object_from_rust_obj( /// creating a Heap. /// /// Can be called multiple times when paired with `ShutdownProcess()`. -pub fn initalize_process(platform: SharedRef) { +pub fn initialize_process(platform: SharedRef) { unsafe { cppgc__initialize_process(&*platform as *const Platform as *mut _); } @@ -155,7 +156,7 @@ pub fn initalize_process(platform: SharedRef) { /// /// Must be called after destroying the last used heap. Some process-global /// metadata may not be returned and reused upon a subsequent -/// `initalize_process()` call. +/// `initialize_process()` call. pub unsafe fn shutdown_process() { cppgc__shutdown_process(); } @@ -343,6 +344,11 @@ pub unsafe fn make_garbage_collected( heap: &Heap, obj: T, ) -> Ptr { + const { + // max alignment in cppgc is 16 + assert!(std::mem::align_of::() <= 16); + } + let additional_bytes = (object_offset_for_rust_obj::() - std::mem::size_of::()) + std::mem::size_of::(); @@ -351,9 +357,12 @@ pub unsafe fn make_garbage_collected( cppgc__make_garbage_collectable( heap as *const Heap as *mut _, additional_bytes, + std::mem::align_of::(), ) }; + assert!(!pointer.is_null()); + unsafe { let inner = get_object_from_rust_obj::(pointer); inner.write(obj); diff --git a/src/support.h b/src/support.h index a530343cba..87cd632510 100644 --- a/src/support.h +++ b/src/support.h @@ -193,8 +193,8 @@ struct three_pointers_t { #endif // SUPPORT_H_ -class RustObj final : public cppgc::GarbageCollected, - public cppgc::NameProvider { +class RustObj : public cppgc::GarbageCollected, + public cppgc::NameProvider { public: ~RustObj(); void Trace(cppgc::Visitor* visitor) const; diff --git a/tests/test_cppgc.rs b/tests/test_cppgc.rs index 6a84622bfa..e9622f9072 100644 --- a/tests/test_cppgc.rs +++ b/tests/test_cppgc.rs @@ -2,177 +2,219 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use v8::cppgc::{GarbageCollected, Visitor}; -struct CppGCGuard { - pub platform: v8::SharedRef, -} - -fn initalize_test() -> CppGCGuard { - v8::V8::set_flags_from_string("--no_freeze_flags_after_init --expose-gc"); - let platform = v8::new_unprotected_default_platform(0, false).make_shared(); - v8::V8::initialize_platform(platform.clone()); - v8::V8::initialize(); - v8::cppgc::initalize_process(platform.clone()); - - CppGCGuard { platform } -} - -impl Drop for CppGCGuard { - fn drop(&mut self) { - unsafe { - v8::cppgc::shutdown_process(); - v8::V8::dispose(); - } - v8::V8::dispose_platform(); +mod setup { + use std::sync::Once; + use std::sync::RwLock; + // use std::sync::RwLockReadGuard; + use std::sync::RwLockWriteGuard; + + static PROCESS_LOCK: RwLock<()> = RwLock::new(()); + + /* + /// Set up global state for a test that can run in parallel with other tests. + pub(super) fn parallel_test() -> SetupGuard> { + initialize_once(); + SetupGuard::new(PROCESS_LOCK.read().unwrap()) } -} - -const TAG: u16 = 1; + */ -#[test] -fn cppgc_object_wrap() { - let guard = initalize_test(); + /// Set up global state for a test that must be the only test running. + pub(super) fn sequential_test() -> SetupGuard> { + initialize_once(); + SetupGuard::new(PROCESS_LOCK.write().unwrap()) + } - static TRACE_COUNT: AtomicUsize = AtomicUsize::new(0); - static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); + fn initialize_once() { + static START: Once = Once::new(); + START.call_once(|| { + assert!(v8::icu::set_common_data_74(align_data::include_aligned!( + align_data::Align16, + "../third_party/icu/common/icudtl.dat" + )) + .is_ok()); + v8::V8::set_flags_from_string( + "--no_freeze_flags_after_init --expose_gc --harmony-import-assertions --harmony-shadow-realm --allow_natives_syntax --turbo_fast_api_calls", + ); - struct Wrap { - value: v8::TracedReference, + let platform = v8::new_unprotected_default_platform(0, false).make_shared(); + v8::V8::initialize_platform(platform.clone()); + v8::V8::initialize(); + v8::cppgc::initialize_process(platform.clone()); + }); } - impl GarbageCollected for Wrap { - fn trace(&self, visitor: &Visitor) { - TRACE_COUNT.fetch_add(1, Ordering::SeqCst); - visitor.trace(&self.value); - } - - fn get_name(&self) -> Option<&'static std::ffi::CStr> { - Some(c"Eyecatcher") - } + #[must_use] + pub(super) struct SetupGuard { + _inner: G, } - impl Drop for Wrap { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); + impl SetupGuard { + fn new(inner: G) -> Self { + Self { _inner: inner } } } +} - fn op_wrap( - scope: &mut v8::HandleScope, - args: v8::FunctionCallbackArguments, - mut rv: v8::ReturnValue, - ) { - fn empty( - _scope: &mut v8::HandleScope, - _args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue, - ) { - } - let templ = v8::FunctionTemplate::new(scope, empty); - let func = templ.get_function(scope).unwrap(); - let obj = func.new_instance(scope, &[]).unwrap(); - assert!(obj.is_api_wrapper()); - - let wrap = Wrap { - value: v8::TracedReference::new(scope, args.get(0)), - }; - let member = unsafe { - v8::cppgc::make_garbage_collected(scope.get_cpp_heap().unwrap(), wrap) - }; - - unsafe { - v8::Object::wrap::(scope, obj, &member); - } +const TAG: u16 = 1; - rv.set(obj.into()); - } +macro_rules! test { + ( $( $decln:ident : $declt:ty )?, $( $initn:ident : $inite:expr )? ) => {{ + let _guard = setup::sequential_test(); - fn op_unwrap( - scope: &mut v8::HandleScope, - args: v8::FunctionCallbackArguments, - mut rv: v8::ReturnValue, - ) { - let obj = args.get(0).try_into().unwrap(); - let member = unsafe { v8::Object::unwrap::(scope, obj) }; - rv.set(member.unwrap().value.get(scope).unwrap()); - } + static TRACE_COUNT: AtomicUsize = AtomicUsize::new(0); + static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); - { - // Create a managed heap. - let mut heap = v8::cppgc::Heap::create( - guard.platform.clone(), - v8::cppgc::HeapCreateParams::default(), - ); - let isolate = &mut v8::Isolate::new(v8::CreateParams::default()); - isolate.attach_cpp_heap(&mut heap); - - { - let handle_scope = &mut v8::HandleScope::new(isolate); - let context = v8::Context::new(handle_scope, Default::default()); - let scope = &mut v8::ContextScope::new(handle_scope, context); - let global = context.global(scope); - { - let func = v8::Function::new(scope, op_wrap).unwrap(); - let name = v8::String::new(scope, "wrap").unwrap(); - global.set(scope, name.into(), func.into()).unwrap(); - } - { - let func = v8::Function::new(scope, op_unwrap).unwrap(); - let name = v8::String::new(scope, "unwrap").unwrap(); - global.set(scope, name.into(), func.into()).unwrap(); + struct Wrap { + $( #[allow(unused)] $decln: $declt , )? + value: v8::TracedReference, } - execute_script( - scope, - r#" - { - const x = {}; - const y = unwrap(wrap(x)); // collected - if (x !== y) { - throw new Error('mismatch'); + impl GarbageCollected for Wrap { + fn trace(&self, visitor: &Visitor) { + TRACE_COUNT.fetch_add(1, Ordering::SeqCst); + visitor.trace(&self.value); + } + + fn get_name(&self) -> Option<&'static std::ffi::CStr> { + Some(c"Eyecatcher") } } - globalThis.wrapped = wrap(wrap({})); // not collected - "#, - ); + impl Drop for Wrap { + fn drop(&mut self) { + DROP_COUNT.fetch_add(1, Ordering::SeqCst); + } + } - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); + fn op_wrap( + scope: &mut v8::HandleScope, + args: v8::FunctionCallbackArguments, + mut rv: v8::ReturnValue, + ) { + fn empty( + _scope: &mut v8::HandleScope, + _args: v8::FunctionCallbackArguments, + _rv: v8::ReturnValue, + ) { + } + let templ = v8::FunctionTemplate::new(scope, empty); + let func = templ.get_function(scope).unwrap(); + let obj = func.new_instance(scope, &[]).unwrap(); + assert!(obj.is_api_wrapper()); + + let wrap = Wrap { + $( $initn: $inite , )? + value: v8::TracedReference::new(scope, args.get(0)), + }; + let member = unsafe { + v8::cppgc::make_garbage_collected(scope.get_cpp_heap().unwrap(), wrap) + }; + + unsafe { + v8::Object::wrap::(scope, obj, &member); + } - { - let mut vec = Vec::::new(); - scope.take_heap_snapshot(|chunk| { - vec.extend_from_slice(chunk); - true - }); - let s = std::str::from_utf8(&vec).unwrap(); - assert!(s.contains("Eyecatcher")); + rv.set(obj.into()); } - scope.request_garbage_collection_for_testing( - v8::GarbageCollectionType::Full, - ); - - assert!(TRACE_COUNT.load(Ordering::SeqCst) > 0); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 1); + fn op_unwrap( + scope: &mut v8::HandleScope, + args: v8::FunctionCallbackArguments, + mut rv: v8::ReturnValue, + ) { + let obj = args.get(0).try_into().unwrap(); + let member = unsafe { v8::Object::unwrap::(scope, obj) }; + rv.set(member.unwrap().value.get(scope).unwrap()); + } - execute_script( - scope, - r#" - globalThis.wrapped = undefined; - "#, - ); + { + // Create a managed heap. + let mut heap = v8::cppgc::Heap::create( + v8::V8::get_current_platform(), + v8::cppgc::HeapCreateParams::default(), + ); + let isolate = &mut v8::Isolate::new(v8::CreateParams::default()); + isolate.attach_cpp_heap(&mut heap); + + { + let handle_scope = &mut v8::HandleScope::new(isolate); + let context = v8::Context::new(handle_scope, Default::default()); + let scope = &mut v8::ContextScope::new(handle_scope, context); + let global = context.global(scope); + { + let func = v8::Function::new(scope, op_wrap).unwrap(); + let name = v8::String::new(scope, "wrap").unwrap(); + global.set(scope, name.into(), func.into()).unwrap(); + } + { + let func = v8::Function::new(scope, op_unwrap).unwrap(); + let name = v8::String::new(scope, "unwrap").unwrap(); + global.set(scope, name.into(), func.into()).unwrap(); + } + + execute_script( + scope, + r#" + { + const x = {}; + const y = unwrap(wrap(x)); // collected + if (x !== y) { + throw new Error('mismatch'); + } + } + + globalThis.wrapped = wrap(wrap({})); // not collected + "#, + ); + + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); + + { + let mut vec = Vec::::new(); + scope.take_heap_snapshot(|chunk| { + vec.extend_from_slice(chunk); + true + }); + let s = std::str::from_utf8(&vec).unwrap(); + assert!(s.contains("Eyecatcher")); + } + + scope.request_garbage_collection_for_testing( + v8::GarbageCollectionType::Full, + ); + + assert!(TRACE_COUNT.load(Ordering::SeqCst) > 0); + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 1); + + execute_script( + scope, + r#" + globalThis.wrapped = undefined; + "#, + ); + + scope.request_garbage_collection_for_testing( + v8::GarbageCollectionType::Full, + ); + + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 3); + } - scope.request_garbage_collection_for_testing( - v8::GarbageCollectionType::Full, - ); + isolate.detach_cpp_heap(); + heap.terminate(); + drop(heap); + } + }} +} - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 3); - } +#[test] +fn cppgc_object_wrap8() { + test!(,); +} - isolate.detach_cpp_heap(); - heap.terminate(); - drop(heap); - } +#[test] +fn cppgc_object_wrap16() { + test!(big: u128, big: 0); } fn execute_script(