From fea59e4f5cc6176093da1d8efa7d7b3b4b8d0fa7 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Tue, 30 Apr 2024 09:58:59 +0800 Subject: [PATCH] Fix write barrier parameter type (#1130) Change the target type of write barrier functions to `Option`. This allows VM bindings to use the write barrier API when storing NULL references or non-references values to slots. Fixes: https://github.com/mmtk/mmtk-core/issues/1129 --- src/memory_manager.rs | 30 ++++++++++++++++--- src/plan/barriers.rs | 16 +++++----- src/plan/generational/barrier.rs | 2 +- .../mock_test_barrier_slow_path_assertion.rs | 11 +++---- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/memory_manager.rs b/src/memory_manager.rs index 2c20fcffd3..0b9f7cd3c3 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -229,6 +229,24 @@ pub fn post_alloc( /// * `src`: The modified source object. /// * `slot`: The location of the field to be modified. /// * `target`: The target for the write operation. +/// +/// # Deprecated +/// +/// This function needs to be redesigned. Its current form has multiple issues. +/// +/// - It is only able to write non-null object references into the slot. But dynamic language +/// VMs may write non-reference values, such as tagged small integers, special values such as +/// `null`, `undefined`, `true`, `false`, etc. into a field that previous contains an object +/// reference. +/// - It relies on `slot.store` to write `target` into the slot, but `slot.store` is designed for +/// forwarding references when an object is moved by GC, and is supposed to preserve tagged +/// type information, the offset (if it is an interior pointer), etc. A write barrier is +/// associated to an assignment operation, which usually updates such information instead. +/// +/// We will redesign a more general subsuming write barrier to address those problems and replace +/// the current `object_reference_write`. Before that happens, VM bindings should use +/// `object_reference_write_pre` and `object_reference_write_post` instead. +#[deprecated = "Use `object_reference_write_pre` and `object_reference_write_post` instead, until this function is redesigned"] pub fn object_reference_write( mutator: &mut Mutator, src: ObjectReference, @@ -252,12 +270,14 @@ pub fn object_reference_write( /// * `mutator`: The mutator for the current thread. /// * `src`: The modified source object. /// * `slot`: The location of the field to be modified. -/// * `target`: The target for the write operation. +/// * `target`: The target for the write operation. `None` if the slot did not hold an object +/// reference before the write operation. For example, the slot may be holding a `null` +/// reference, a small integer, or special values such as `true`, `false`, `undefined`, etc. pub fn object_reference_write_pre( mutator: &mut Mutator, src: ObjectReference, slot: VM::VMEdge, - target: ObjectReference, + target: Option, ) { mutator .barrier() @@ -278,12 +298,14 @@ pub fn object_reference_write_pre( /// * `mutator`: The mutator for the current thread. /// * `src`: The modified source object. /// * `slot`: The location of the field to be modified. -/// * `target`: The target for the write operation. +/// * `target`: The target for the write operation. `None` if the slot no longer hold an object +/// reference after the write operation. This may happen when writing a `null` reference, a small +/// integers, or a special value such as`true`, `false`, `undefined`, etc., into the slot. pub fn object_reference_write_post( mutator: &mut Mutator, src: ObjectReference, slot: VM::VMEdge, - target: ObjectReference, + target: Option, ) { mutator .barrier() diff --git a/src/plan/barriers.rs b/src/plan/barriers.rs index caf3372251..ef1cb6c4bb 100644 --- a/src/plan/barriers.rs +++ b/src/plan/barriers.rs @@ -52,9 +52,9 @@ pub trait Barrier: 'static + Send + Downcast { slot: VM::VMEdge, target: ObjectReference, ) { - self.object_reference_write_pre(src, slot, target); + self.object_reference_write_pre(src, slot, Some(target)); slot.store(target); - self.object_reference_write_post(src, slot, target); + self.object_reference_write_post(src, slot, Some(target)); } /// Full pre-barrier for object reference write @@ -62,7 +62,7 @@ pub trait Barrier: 'static + Send + Downcast { &mut self, _src: ObjectReference, _slot: VM::VMEdge, - _target: ObjectReference, + _target: Option, ) { } @@ -71,7 +71,7 @@ pub trait Barrier: 'static + Send + Downcast { &mut self, _src: ObjectReference, _slot: VM::VMEdge, - _target: ObjectReference, + _target: Option, ) { } @@ -81,7 +81,7 @@ pub trait Barrier: 'static + Send + Downcast { &mut self, _src: ObjectReference, _slot: VM::VMEdge, - _target: ObjectReference, + _target: Option, ) { } @@ -147,7 +147,7 @@ pub trait BarrierSemantics: 'static + Send { &mut self, src: ObjectReference, slot: ::VMEdge, - target: ObjectReference, + target: Option, ); /// Slow-path call for mempry slice copy operations. For example, array-copy operations. @@ -217,7 +217,7 @@ impl Barrier for ObjectBarrier { &mut self, src: ObjectReference, slot: ::VMEdge, - target: ObjectReference, + target: Option, ) { if self.object_is_unlogged(src) { self.object_reference_write_slow(src, slot, target); @@ -228,7 +228,7 @@ impl Barrier for ObjectBarrier { &mut self, src: ObjectReference, slot: ::VMEdge, - target: ObjectReference, + target: Option, ) { if self.log_object(src) { self.semantics diff --git a/src/plan/generational/barrier.rs b/src/plan/generational/barrier.rs index 9402d2470d..3c7281ba93 100644 --- a/src/plan/generational/barrier.rs +++ b/src/plan/generational/barrier.rs @@ -75,7 +75,7 @@ impl + PlanTraceObject> BarrierSem &mut self, src: ObjectReference, _slot: VM::VMEdge, - _target: ObjectReference, + _target: Option, ) { // enqueue the object self.modbuf.push(src); diff --git a/src/vm/tests/mock_tests/mock_test_barrier_slow_path_assertion.rs b/src/vm/tests/mock_tests/mock_test_barrier_slow_path_assertion.rs index e244a596d7..2d22d06aea 100644 --- a/src/vm/tests/mock_tests/mock_test_barrier_slow_path_assertion.rs +++ b/src/vm/tests/mock_tests/mock_test_barrier_slow_path_assertion.rs @@ -30,7 +30,7 @@ fn test_assertion_barrier_invalid_ref() { fixture.mutator_mut().barrier.object_reference_write_slow( invalid_objref, edge, - objref, + Some(objref), ); }); }, @@ -51,10 +51,11 @@ fn test_assertion_barrier_valid_ref() { let edge = Address::from_ref(&slot); // Invoke barrier slowpath with the valid object ref - fixture - .mutator_mut() - .barrier - .object_reference_write_slow(objref, edge, objref); + fixture.mutator_mut().barrier.object_reference_write_slow( + objref, + edge, + Some(objref), + ); }); }, no_cleanup,