diff --git a/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs b/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs index 4394bbd369..bc7e2eda41 100644 --- a/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs +++ b/docs/userguide/src/tutorial/code/mygc_semispace/gc_work.rs @@ -56,7 +56,6 @@ impl ProcessEdgesWork for MyGCProcessEdges { } fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - debug_assert!(!object.is_null()); let worker = self.worker(); let queue = &mut self.base.nodes; if self.plan.tospace().in_space(object) { diff --git a/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md b/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md index cb751aa082..0474a89cd1 100644 --- a/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md +++ b/docs/userguide/src/tutorial/mygc/ss/exercise_solution.md @@ -149,8 +149,6 @@ In `gc_work.rs`: the tospace and fromspace: ```rust fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - debug_assert!(!object.is_null()); - // Add this! else if self.plan().youngspace().in_space(object) { self.plan().youngspace.trace_object::>( diff --git a/src/memory_manager.rs b/src/memory_manager.rs index 7cab5f932c..2c20fcffd3 100644 --- a/src/memory_manager.rs +++ b/src/memory_manager.rs @@ -638,9 +638,6 @@ pub fn is_mmtk_object(addr: Address) -> bool { /// * `object`: The object reference to query. pub fn is_in_mmtk_spaces(object: ObjectReference) -> bool { use crate::mmtk::SFT_MAP; - if object.is_null() { - return false; - } SFT_MAP .get_checked(object.to_address::()) .is_in_space(object) diff --git a/src/plan/generational/gc_work.rs b/src/plan/generational/gc_work.rs index 5862e4a00e..afa0ebf1fb 100644 --- a/src/plan/generational/gc_work.rs +++ b/src/plan/generational/gc_work.rs @@ -43,8 +43,6 @@ impl + PlanTraceObject, const KIND } fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - debug_assert!(!object.is_null()); - // We cannot borrow `self` twice in a call, so we extract `worker` as a local variable. let worker = self.worker(); self.plan.trace_object_nursery::( @@ -55,10 +53,10 @@ impl + PlanTraceObject, const KIND } fn process_edge(&mut self, slot: EdgeOf) { - let object = slot.load(); - if object.is_null() { + let Some(object) = slot.load() else { + // Skip slots that are not holding an object reference. return; - } + }; let new_object = self.trace_object(object); debug_assert!(!self.plan.is_object_in_nursery(new_object)); // Note: If `object` is a mature object, `trace_object` will not call `space.trace_object`, diff --git a/src/plan/global.rs b/src/plan/global.rs index 5b28c4b720..1e47665581 100644 --- a/src/plan/global.rs +++ b/src/plan/global.rs @@ -691,7 +691,7 @@ pub trait PlanTraceObject { /// /// Arguments: /// * `trace`: the current transitive closure - /// * `object`: the object to trace. This is a non-nullable object reference. + /// * `object`: the object to trace. /// * `worker`: the GC worker that is tracing this object. fn trace_object( &self, diff --git a/src/plan/tracing.rs b/src/plan/tracing.rs index adc2ea9462..27e021051f 100644 --- a/src/plan/tracing.rs +++ b/src/plan/tracing.rs @@ -117,7 +117,7 @@ impl<'a, E: ProcessEdgesWork> EdgeVisitor> for ObjectsClosure<'a, E> { { use crate::vm::edge_shape::Edge; trace!( - "(ObjectsClosure) Visit edge {:?} (pointing to {})", + "(ObjectsClosure) Visit edge {:?} (pointing to {:?})", slot, slot.load() ); diff --git a/src/policy/copyspace.rs b/src/policy/copyspace.rs index 82a4ee94a6..3fd7a21854 100644 --- a/src/policy/copyspace.rs +++ b/src/policy/copyspace.rs @@ -204,7 +204,6 @@ impl CopySpace { worker: &mut GCWorker, ) -> ObjectReference { trace!("copyspace.trace_object(, {:?}, {:?})", object, semantics,); - debug_assert!(!object.is_null()); // If this is not from space, we do not need to trace it (the object has been copied to the tosapce) if !self.is_from_space() { diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index 049e18f699..1f9d436ab6 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -184,7 +184,6 @@ impl crate::policy::gc_work::PolicyTraceObject for ImmixSpace copy: Option, worker: &mut GCWorker, ) -> ObjectReference { - debug_assert!(!object.is_null()); if KIND == TRACE_KIND_TRANSITIVE_PIN { self.trace_object_without_moving(queue, object) } else if KIND == TRACE_KIND_DEFRAG { diff --git a/src/policy/immortalspace.rs b/src/policy/immortalspace.rs index 1e4d19eefa..5eeebd58c9 100644 --- a/src/policy/immortalspace.rs +++ b/src/policy/immortalspace.rs @@ -187,7 +187,6 @@ impl ImmortalSpace { queue: &mut Q, object: ObjectReference, ) -> ObjectReference { - debug_assert!(!object.is_null()); #[cfg(feature = "vo_bit")] debug_assert!( crate::util::metadata::vo_bit::is_vo_bit_set::(object), diff --git a/src/policy/largeobjectspace.rs b/src/policy/largeobjectspace.rs index 64a13c8f37..ec6b2f7506 100644 --- a/src/policy/largeobjectspace.rs +++ b/src/policy/largeobjectspace.rs @@ -189,7 +189,6 @@ impl LargeObjectSpace { queue: &mut Q, object: ObjectReference, ) -> ObjectReference { - debug_assert!(!object.is_null()); #[cfg(feature = "vo_bit")] debug_assert!( crate::util::metadata::vo_bit::is_vo_bit_set::(object), diff --git a/src/policy/markcompactspace.rs b/src/policy/markcompactspace.rs index ce125e8288..8a8e2dca8a 100644 --- a/src/policy/markcompactspace.rs +++ b/src/policy/markcompactspace.rs @@ -37,12 +37,7 @@ impl SFT for MarkCompactSpace { } fn get_forwarded_object(&self, object: ObjectReference) -> Option { - let forwarding_pointer = Self::get_header_forwarding_pointer(object); - if forwarding_pointer.is_null() { - None - } else { - Some(forwarding_pointer) - } + Self::get_header_forwarding_pointer(object) } fn is_live(&self, object: ObjectReference) -> bool { @@ -130,7 +125,6 @@ impl crate::policy::gc_work::PolicyTraceObject for MarkCompac _copy: Option, _worker: &mut GCWorker, ) -> ObjectReference { - debug_assert!(!object.is_null()); debug_assert!( KIND != TRACE_KIND_TRANSITIVE_PIN, "MarkCompact does not support transitive pin trace." @@ -177,8 +171,9 @@ impl MarkCompactSpace { } /// Get header forwarding pointer for an object - fn get_header_forwarding_pointer(object: ObjectReference) -> ObjectReference { - unsafe { Self::header_forwarding_pointer_address(object).load::() } + fn get_header_forwarding_pointer(object: ObjectReference) -> Option { + let addr = unsafe { Self::header_forwarding_pointer_address(object).load::
() }; + ObjectReference::from_raw_address(addr) } /// Store header forwarding pointer for an object @@ -251,12 +246,8 @@ impl MarkCompactSpace { queue.enqueue(object); } - let result = Self::get_header_forwarding_pointer(object); - debug_assert!( - !result.is_null(), - "Object {object} does not have a forwarding pointer" - ); - result + Self::get_header_forwarding_pointer(object) + .unwrap_or_else(|| panic!("Object {object} does not have a forwarding pointer")) } pub fn test_and_mark(object: ObjectReference) -> bool { @@ -393,10 +384,9 @@ impl MarkCompactSpace { // clear the VO bit vo_bit::unset_vo_bit::(obj); - let forwarding_pointer = Self::get_header_forwarding_pointer(obj); - - trace!("Compact {} to {}", obj, forwarding_pointer); - if !forwarding_pointer.is_null() { + let maybe_forwarding_pointer = Self::get_header_forwarding_pointer(obj); + if let Some(forwarding_pointer) = maybe_forwarding_pointer { + trace!("Compact {} to {}", obj, forwarding_pointer); let new_object = forwarding_pointer; Self::clear_header_forwarding_pointer(new_object); @@ -408,6 +398,8 @@ impl MarkCompactSpace { vo_bit::set_vo_bit::(new_object); to = new_object.to_object_start::() + copied_size; debug_assert_eq!(end_of_new_object, to); + } else { + trace!("Skipping dead object {}", obj); } } } diff --git a/src/policy/marksweepspace/malloc_ms/global.rs b/src/policy/marksweepspace/malloc_ms/global.rs index 91d9b8a698..93f76e6cdb 100644 --- a/src/policy/marksweepspace/malloc_ms/global.rs +++ b/src/policy/marksweepspace/malloc_ms/global.rs @@ -400,8 +400,6 @@ impl MallocSpace { queue: &mut Q, object: ObjectReference, ) -> ObjectReference { - debug_assert!(!object.is_null()); - assert!( self.in_space(object), "Cannot mark an object {} that was not alloced by malloc.", diff --git a/src/policy/marksweepspace/native_ms/block.rs b/src/policy/marksweepspace/native_ms/block.rs index e9af1c8272..d607f71b4e 100644 --- a/src/policy/marksweepspace/native_ms/block.rs +++ b/src/policy/marksweepspace/native_ms/block.rs @@ -280,7 +280,8 @@ impl Block { while cell + cell_size <= self.start() + Block::BYTES { // The invariants we checked earlier ensures that we can use cell and object reference interchangably // We may not really have an object in this cell, but if we do, this object reference is correct. - let potential_object = ObjectReference::from_raw_address(cell); + // About unsafe: We know `cell` is non-zero here. + let potential_object = unsafe { ObjectReference::from_raw_address_unchecked(cell) }; if !VM::VMObjectModel::LOCAL_MARK_BIT_SPEC .is_marked::(potential_object, Ordering::SeqCst) @@ -320,9 +321,12 @@ impl Block { while cell + cell_size <= self.end() { // possible object ref - let potential_object_ref = ObjectReference::from_raw_address( - cursor + VM::VMObjectModel::OBJECT_REF_OFFSET_LOWER_BOUND, - ); + let potential_object_ref = unsafe { + // We know cursor plus an offset cannot be 0. + ObjectReference::from_raw_address_unchecked( + cursor + VM::VMObjectModel::OBJECT_REF_OFFSET_LOWER_BOUND, + ) + }; trace!( "{:?}: cell = {}, last cell in free list = {}, cursor = {}, potential object = {}", self, diff --git a/src/policy/marksweepspace/native_ms/global.rs b/src/policy/marksweepspace/native_ms/global.rs index 5db093574e..e16d38626f 100644 --- a/src/policy/marksweepspace/native_ms/global.rs +++ b/src/policy/marksweepspace/native_ms/global.rs @@ -291,7 +291,6 @@ impl MarkSweepSpace { queue: &mut Q, object: ObjectReference, ) -> ObjectReference { - debug_assert!(!object.is_null()); debug_assert!( self.in_space(object), "Cannot mark an object {} that was not alloced by free list allocator.", diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs index 45cfd13715..3b9e5c4724 100644 --- a/src/scheduler/gc_work.rs +++ b/src/scheduler/gc_work.rs @@ -223,7 +223,6 @@ impl ObjectTracer for ProcessEdgesWorkTracer { /// Forward the `trace_object` call to the underlying `ProcessEdgesWork`, /// and flush as soon as the underlying buffer of `process_edges_work` is full. fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - debug_assert!(!object.is_null()); let result = self.process_edges_work.trace_object(object); self.flush_if_full(); result @@ -604,12 +603,11 @@ pub trait ProcessEdgesWork: /// Process an edge, including loading the object reference from the memory slot, /// trace the object and store back the new object reference if necessary. fn process_edge(&mut self, slot: EdgeOf) { - let object = slot.load(); - if object.is_null() { + let Some(object) = slot.load() else { + // Skip slots that are not holding an object reference. return; - } + }; let new_object = self.trace_object(object); - debug_assert!(!new_object.is_null()); if Self::OVERWRITE_REFERENCE && new_object != object { slot.store(new_object); } @@ -667,8 +665,6 @@ impl ProcessEdgesWork for SFTProcessEdges { fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { use crate::policy::sft::GCWorkerMutRef; - debug_assert!(!object.is_null()); - // Erase type parameter let worker = GCWorkerMutRef::new(self.worker()); @@ -925,7 +921,6 @@ impl + Plan, const KIND: TraceKin } fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - debug_assert!(!object.is_null()); // We cannot borrow `self` twice in a call, so we extract `worker` as a local variable. let worker = self.worker(); self.plan @@ -933,12 +928,11 @@ impl + Plan, const KIND: TraceKin } fn process_edge(&mut self, slot: EdgeOf) { - let object = slot.load(); - if object.is_null() { + let Some(object) = slot.load() else { + // Skip slots that are not holding an object reference. return; - } + }; let new_object = self.trace_object(object); - debug_assert!(!new_object.is_null()); if P::may_move_objects::() && new_object != object { slot.store(new_object); } diff --git a/src/util/address.rs b/src/util/address.rs index 913c3ea198..2bd9c613a9 100644 --- a/src/util/address.rs +++ b/src/util/address.rs @@ -3,6 +3,7 @@ use bytemuck::NoUninit; use std::fmt; use std::mem; +use std::num::NonZeroUsize; use std::ops::*; use std::sync::atomic::Ordering; @@ -477,30 +478,52 @@ use crate::vm::VMBinding; /// their layout. We now only allow a binding to define their semantics through a set of /// methods in [`crate::vm::ObjectModel`]. Major refactoring is needed in MMTk to allow /// the opaque `ObjectReference` type, and we haven't seen a use case for now. +/// +/// Note that [`ObjectReference`] cannot be null. For the cases where a non-null object reference +/// may or may not exist, (such as the result of [`crate::vm::edge_shape::Edge::load`]) +/// `Option` should be used. [`ObjectReference`] is backed by `NonZeroUsize` +/// which cannot be zero, and it has the `#[repr(transparent)]` attribute. Thanks to [null pointer +/// optimization (NPO)][NPO], `Option` has the same size as `NonZeroUsize` and +/// `usize`. For the convenience of passing `Option` to and from native (C/C++) +/// programs, mmtk-core provides [`crate::util::api_util::NullableObjectReference`]. +/// +/// [NPO]: https://doc.rust-lang.org/std/option/index.html#representation #[repr(transparent)] #[derive(Copy, Clone, Eq, Hash, PartialOrd, Ord, PartialEq, NoUninit)] -pub struct ObjectReference(usize); +pub struct ObjectReference(NonZeroUsize); impl ObjectReference { - /// The null object reference, represented as zero. - pub const NULL: ObjectReference = ObjectReference(0); - /// Cast the object reference to its raw address. This method is mostly for the convinience of a binding. /// /// MMTk should not make any assumption on the actual location of the address with the object reference. /// MMTk should not assume the address returned by this method is in our allocation. For the purposes of /// setting object metadata, MMTk should use [`crate::vm::ObjectModel::ref_to_address()`] or [`crate::vm::ObjectModel::ref_to_header()`]. pub fn to_raw_address(self) -> Address { - Address(self.0) + Address(self.0.get()) } /// Cast a raw address to an object reference. This method is mostly for the convinience of a binding. /// This is how a binding creates `ObjectReference` instances. /// + /// If `addr` is 0, the result is `None`. + /// /// MMTk should not assume an arbitrary address can be turned into an object reference. MMTk can use [`crate::vm::ObjectModel::address_to_ref()`] /// to turn addresses that are from [`crate::vm::ObjectModel::ref_to_address()`] back to object. - pub fn from_raw_address(addr: Address) -> ObjectReference { - ObjectReference(addr.0) + pub fn from_raw_address(addr: Address) -> Option { + NonZeroUsize::new(addr.0).map(ObjectReference) + } + + /// Like `from_raw_address`, but assume `addr` is not zero. This can be used to elide a check + /// against zero for performance-critical code. + /// + /// # Safety + /// + /// This method assumes `addr` is not zero. It should only be used in cases where we know at + /// compile time that the input cannot be zero. For example, if we compute the address by + /// adding a positive offset to a non-zero address, we know the result must not be zero. + pub unsafe fn from_raw_address_unchecked(addr: Address) -> ObjectReference { + debug_assert!(!addr.is_zero()); + ObjectReference(NonZeroUsize::new_unchecked(addr.0)) } /// Get the in-heap address from an object reference. This method is used by MMTk to get an in-heap address @@ -541,28 +564,15 @@ impl ObjectReference { obj } - /// is this object reference null reference? - pub fn is_null(self) -> bool { - self.0 == 0 - } - /// Is the object reachable, determined by the policy? /// Note: Objects in ImmortalSpace may have `is_live = true` but are actually unreachable. pub fn is_reachable(self) -> bool { - if self.is_null() { - false - } else { - unsafe { SFT_MAP.get_unchecked(self.to_address::()) }.is_reachable(self) - } + unsafe { SFT_MAP.get_unchecked(self.to_address::()) }.is_reachable(self) } /// Is the object live, determined by the policy? pub fn is_live(self) -> bool { - if self.0 == 0 { - false - } else { - unsafe { SFT_MAP.get_unchecked(self.to_address::()) }.is_live(self) - } + unsafe { SFT_MAP.get_unchecked(self.to_address::()) }.is_live(self) } /// Can the object be moved? diff --git a/src/util/alloc/free_list_allocator.rs b/src/util/alloc/free_list_allocator.rs index 27546fc337..2bbd10a64d 100644 --- a/src/util/alloc/free_list_allocator.rs +++ b/src/util/alloc/free_list_allocator.rs @@ -370,6 +370,8 @@ impl FreeListAllocator { #[cfg(feature = "malloc_native_mimalloc")] fn free(&self, addr: Address) { + assert!(!addr.is_zero(), "Attempted to free zero address."); + use crate::util::ObjectReference; let block = Block::from_unaligned_address(addr); let block_tls = block.load_tls(); @@ -402,11 +404,11 @@ impl FreeListAllocator { } // unset allocation bit - unsafe { - crate::util::metadata::vo_bit::unset_vo_bit_unsafe::( - ObjectReference::from_raw_address(addr), - ) - }; + // Note: We cannot use `unset_vo_bit_unsafe` because two threads may attempt to free + // objects at adjacent addresses, and they may share the same byte in the VO bit metadata. + crate::util::metadata::vo_bit::unset_vo_bit::(unsafe { + ObjectReference::from_raw_address_unchecked(addr) + }) } fn store_block_tls(&self, block: Block) { diff --git a/src/util/api_util.rs b/src/util/api_util.rs new file mode 100644 index 0000000000..a923dec720 --- /dev/null +++ b/src/util/api_util.rs @@ -0,0 +1,33 @@ +//! This module contain helpers for the convenience of exposing the MMTk API to native (usually +//! C/C++) programs. + +use super::{Address, ObjectReference}; + +/// An `Option` encoded as a `usize` (which is guaranteed to have the size of a +/// native pointer). It guarantees that `None` is encoded as 0, and `Some(objref)` is encoded as +/// the underlying `usize` value of the `ObjectReference` itself. +/// +/// Note: The Rust ABI currently doesn't guarantee the encoding of `None` even if the `T` in +/// `Option` is eligible for null pointer optimization. Transmuting a `None` value of +/// `Option` to `usize` still has undefined behavior. +/// See: +/// +/// It is intended for passing an `Option` values to and from native programs +/// (usually C or C++) that have null pointers. +#[repr(transparent)] +pub struct NullableObjectReference(usize); + +impl From for Option { + fn from(value: NullableObjectReference) -> Self { + ObjectReference::from_raw_address(unsafe { Address::from_usize(value.0) }) + } +} + +impl From> for NullableObjectReference { + fn from(value: Option) -> Self { + let encoded = value + .map(|obj| obj.to_raw_address().as_usize()) + .unwrap_or(0); + Self(encoded) + } +} diff --git a/src/util/metadata/vo_bit/mod.rs b/src/util/metadata/vo_bit/mod.rs index 0382d933d0..7f1648de8d 100644 --- a/src/util/metadata/vo_bit/mod.rs +++ b/src/util/metadata/vo_bit/mod.rs @@ -100,7 +100,8 @@ pub fn is_vo_bit_set(object: ObjectReference) -> bool { /// Check if an address can be turned directly into an object reference using the VO bit. /// If so, return `Some(object)`. Otherwise return `None`. pub fn is_vo_bit_set_for_addr(address: Address) -> Option { - let potential_object = ObjectReference::from_raw_address(address); + let potential_object = ObjectReference::from_raw_address(address)?; + let addr = potential_object.to_address::(); // If we haven't mapped VO bit for the address, it cannot be an object @@ -123,7 +124,8 @@ pub fn is_vo_bit_set_for_addr(address: Address) -> Option(address: Address) -> Option { - let potential_object = ObjectReference::from_raw_address(address); + let potential_object = ObjectReference::from_raw_address(address)?; + let addr = potential_object.to_address::(); // If we haven't mapped VO bit for the address, it cannot be an object diff --git a/src/util/mod.rs b/src/util/mod.rs index 79922a8b06..393a24d539 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -11,6 +11,8 @@ pub mod address; /// Allocators // This module is made public so the binding could implement allocator slowpaths if they would like to. pub mod alloc; +/// Helpers for making native APIs. +pub mod api_util; /// Constants used in MMTk pub mod constants; /// Calculation, conversion and rounding for memory related numbers. diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs index 01b3fec447..85c2aaaad5 100644 --- a/src/util/object_forwarding.rs +++ b/src/util/object_forwarding.rs @@ -150,7 +150,9 @@ pub fn read_forwarding_pointer(object: ObjectReference) -> Object // We write the forwarding poiner. We know it is an object reference. unsafe { - ObjectReference::from_raw_address(crate::util::Address::from_usize( + // We use "unchecked" convertion becasue we guarantee the forwarding pointer we stored + // previously is from a valid `ObjectReference` which is never zero. + ObjectReference::from_raw_address_unchecked(crate::util::Address::from_usize( VM::VMObjectModel::LOCAL_FORWARDING_POINTER_SPEC.load_atomic::( object, Some(FORWARDING_POINTER_MASK), diff --git a/src/util/reference_processor.rs b/src/util/reference_processor.rs index f7a4a17241..4f4331849c 100644 --- a/src/util/reference_processor.rs +++ b/src/util/reference_processor.rs @@ -210,7 +210,6 @@ impl ReferenceProcessor { /// `ObjectReference` otherwise. The referent must be live when calling this function. fn get_forwarded_referent(referent: ObjectReference) -> ObjectReference { debug_assert!(referent.is_live::()); - debug_assert!(!referent.is_null()); referent.get_forwarded_object::().unwrap_or(referent) } @@ -219,7 +218,6 @@ impl ReferenceProcessor { /// function. fn get_forwarded_reference(object: ObjectReference) -> ObjectReference { debug_assert!(object.is_live::()); - debug_assert!(!object.is_null()); object.get_forwarded_object::().unwrap_or(object) } @@ -238,19 +236,17 @@ impl ReferenceProcessor { e: &mut E, referent: ObjectReference, ) -> ObjectReference { - debug_assert!(!referent.is_null()); e.trace_object(referent) } /// This function is called when forwarding the references and referents (for MarkCompact). It /// - adds the reference or the referent to the tracing queue if not yet reached, so that - /// the children of the reference or referent will be kept alive, too, and + /// the children of the reference or referent will be visited and forwarded, too, and /// - gets the forwarded object reference of the object. fn trace_forward_object( e: &mut E, referent: ObjectReference, ) -> ObjectReference { - debug_assert!(!referent.is_null()); e.trace_object(referent) } @@ -261,12 +257,10 @@ impl ReferenceProcessor { // This is the end of a GC. We do some assertions here to make sure our reference tables are correct. #[cfg(debug_assertions)] { - // For references in the table, the reference needs to be valid, and if the referent is not null, it should be valid as well + // For references in the table, the reference needs to be valid, and if the referent is not cleared, it should be valid as well sync.references.iter().for_each(|reff| { - debug_assert!(!reff.is_null()); debug_assert!(reff.is_in_any_space::()); - let referent = VM::VMReferenceGlue::get_referent(*reff); - if !VM::VMReferenceGlue::is_referent_cleared(referent) { + if let Some(referent) = VM::VMReferenceGlue::get_referent(*reff) { debug_assert!( referent.is_in_any_space::(), "Referent {:?} (of reference {:?}) is not in any space", @@ -275,12 +269,11 @@ impl ReferenceProcessor { ); } }); - // For references that will be enqueue'd, the referent needs to be valid, and the referent needs to be null. + // For references that will be enqueue'd, the reference needs to be valid, and the referent needs to be cleared. sync.enqueued_references.iter().for_each(|reff| { - debug_assert!(!reff.is_null()); debug_assert!(reff.is_in_any_space::()); - let referent = VM::VMReferenceGlue::get_referent(*reff); - debug_assert!(VM::VMReferenceGlue::is_referent_cleared(referent)); + let maybe_referent = VM::VMReferenceGlue::get_referent(*reff); + debug_assert!(maybe_referent.is_none()); }); } @@ -305,7 +298,6 @@ impl ReferenceProcessor { trace: &mut E, reference: ObjectReference, ) -> ObjectReference { - let old_referent = ::VMReferenceGlue::get_referent(reference); { use crate::vm::ObjectModel; trace!( @@ -315,7 +307,9 @@ impl ReferenceProcessor { ); } - if !::VMReferenceGlue::is_referent_cleared(old_referent) { + if let Some(old_referent) = + ::VMReferenceGlue::get_referent(reference) + { let new_referent = ReferenceProcessor::trace_forward_object(trace, old_referent); ::VMReferenceGlue::set_referent(reference, new_referent); @@ -329,11 +323,6 @@ impl ReferenceProcessor { let new_reference = ReferenceProcessor::trace_forward_object(trace, reference); trace!(" reference: forwarded to {}", new_reference); - debug_assert!( - !new_reference.is_null(), - "reference {:?}'s forwarding pointer is NULL", - reference - ); new_reference } @@ -412,8 +401,6 @@ impl ReferenceProcessor { ); for reference in sync.references.iter() { - debug_assert!(!reference.is_null()); - trace!("Processing reference: {:?}", reference); if !reference.is_live::() { @@ -421,13 +408,12 @@ impl ReferenceProcessor { // following trace. We postpone the decision. continue; } - // Reference is definitely reachable. Retain the referent. - let referent = ::VMReferenceGlue::get_referent(*reference); - if !::VMReferenceGlue::is_referent_cleared(referent) { + if let Some(referent) = ::VMReferenceGlue::get_referent(*reference) + { Self::keep_referent_alive(trace, referent); + trace!(" ~> {:?} (retained)", referent); } - trace!(" ~> {:?} (retained)", referent); } debug!("Ending ReferenceProcessor.retain({:?})", self.semantics); @@ -435,7 +421,7 @@ impl ReferenceProcessor { /// Process a reference. /// * If both the reference and the referent is alive, return the updated reference and update its referent properly. - /// * If the reference is alive, and the referent is not null but not alive, return None and the reference (with cleared referent) is enqueued. + /// * If the reference is alive, and the referent is not cleared but not alive, return None and the reference (with cleared referent) is enqueued. /// * For other cases, return None. /// /// If a None value is returned, the reference can be removed from the reference table. Otherwise, the updated reference should be kept @@ -445,8 +431,6 @@ impl ReferenceProcessor { reference: ObjectReference, enqueued_references: &mut Vec, ) -> Option { - debug_assert!(!reference.is_null()); - trace!("Process reference: {}", reference); // If the reference is dead, we're done with it. Let it (and @@ -454,32 +438,32 @@ impl ReferenceProcessor { if !reference.is_live::() { VM::VMReferenceGlue::clear_referent(reference); trace!(" UNREACHABLE reference: {}", reference); - trace!(" (unreachable)"); return None; } - // The reference object is live + // The reference object is live. let new_reference = Self::get_forwarded_reference::(reference); - let old_referent = VM::VMReferenceGlue::get_referent(reference); - trace!(" ~> {}", old_referent); + trace!(" forwarded to: {}", new_reference); + + // Get the old referent. + let maybe_old_referent = VM::VMReferenceGlue::get_referent(reference); + trace!(" referent: {:?}", maybe_old_referent); // If the application has cleared the referent the Java spec says // this does not cause the Reference object to be enqueued. We // simply allow the Reference object to fall out of our // waiting list. - if VM::VMReferenceGlue::is_referent_cleared(old_referent) { - trace!(" (cleared referent) "); + let Some(old_referent) = maybe_old_referent else { + trace!(" (cleared referent) "); return None; - } - - trace!(" => {}", new_reference); + }; if old_referent.is_live::() { // Referent is still reachable in a way that is as strong as // or stronger than the current reference level. let new_referent = Self::get_forwarded_referent::(old_referent); debug_assert!(new_referent.is_live::()); - trace!(" ~> {}", new_referent); + trace!(" forwarded referent to: {}", new_referent); // The reference object stays on the waiting list, and the // referent is untouched. The only thing we must do is @@ -492,7 +476,7 @@ impl ReferenceProcessor { Some(new_reference) } else { // Referent is unreachable. Clear the referent and enqueue the reference object. - trace!(" UNREACHABLE referent: {}", old_referent); + trace!(" UNREACHABLE referent: {}", old_referent); VM::VMReferenceGlue::clear_referent(new_reference); enqueued_references.push(new_reference); diff --git a/src/util/sanity/sanity_checker.rs b/src/util/sanity/sanity_checker.rs index 1fc7836613..d6ef849e35 100644 --- a/src/util/sanity/sanity_checker.rs +++ b/src/util/sanity/sanity_checker.rs @@ -189,9 +189,6 @@ impl ProcessEdgesWork for SanityGCProcessEdges { } fn trace_object(&mut self, object: ObjectReference) -> ObjectReference { - if object.is_null() { - return object; - } let mut sanity_checker = self.mmtk().sanity_checker.lock().unwrap(); if !sanity_checker.refs.contains(&object) { // FIXME steveb consider VM-specific integrity check on reference. diff --git a/src/util/test_util/mock_vm.rs b/src/util/test_util/mock_vm.rs index 842b37f8e6..30fae0f651 100644 --- a/src/util/test_util/mock_vm.rs +++ b/src/util/test_util/mock_vm.rs @@ -232,8 +232,7 @@ pub struct MockVM { // reference glue pub weakref_clear_referent: MockMethod, pub weakref_set_referent: MockMethod<(ObjectReference, ObjectReference), ()>, - pub weakref_get_referent: MockMethod, - pub weakref_is_referent_cleared: MockMethod, + pub weakref_get_referent: MockMethod>, pub weakref_enqueue_references: MockMethod<(&'static [ObjectReference], VMWorkerThread), ()>, // scanning pub support_edge_enqueuing: MockMethod<(VMWorkerThread, ObjectReference), bool>, @@ -304,14 +303,13 @@ impl Default for MockVM { object.to_raw_address().sub(DEFAULT_OBJECT_REF_OFFSET) })), address_to_ref: MockMethod::new_fixed(Box::new(|addr| { - ObjectReference::from_raw_address(addr.add(DEFAULT_OBJECT_REF_OFFSET)) + ObjectReference::from_raw_address(addr.add(DEFAULT_OBJECT_REF_OFFSET)).unwrap() })), dump_object: MockMethod::new_unimplemented(), weakref_clear_referent: MockMethod::new_unimplemented(), weakref_get_referent: MockMethod::new_unimplemented(), weakref_set_referent: MockMethod::new_unimplemented(), - weakref_is_referent_cleared: MockMethod::new_fixed(Box::new(|r| r.is_null())), weakref_enqueue_references: MockMethod::new_unimplemented(), support_edge_enqueuing: MockMethod::new_fixed(Box::new(|_| true)), @@ -551,12 +549,9 @@ impl crate::vm::ReferenceGlue for MockVM { fn set_referent(reference: ObjectReference, referent: ObjectReference) { mock!(weakref_set_referent(reference, referent)) } - fn get_referent(object: ObjectReference) -> ObjectReference { + fn get_referent(object: ObjectReference) -> Option { mock!(weakref_get_referent(object)) } - fn is_referent_cleared(referent: ObjectReference) -> bool { - mock!(weakref_is_referent_cleared(referent)) - } fn enqueue_references(references: &[ObjectReference], tls: VMWorkerThread) { mock!(weakref_enqueue_references(lifetime!(references), tls)) } diff --git a/src/vm/edge_shape.rs b/src/vm/edge_shape.rs index 45e7141c25..ffd24c0c0a 100644 --- a/src/vm/edge_shape.rs +++ b/src/vm/edge_shape.rs @@ -49,11 +49,11 @@ pub trait Edge: Copy + Send + Debug + PartialEq + Eq + Hash { /// /// If the slot is not holding an object reference (For example, if it is holding NULL or a /// tagged non-reference value. See trait-level doc comment.), this method should return - /// `ObjectReference::NULL`. + /// `None`. /// /// If the slot holds an object reference with tag bits, the returned value shall be the object /// reference with the tag bits removed. - fn load(&self) -> ObjectReference; + fn load(&self) -> Option; /// Store the object reference `object` into the slot. /// @@ -83,12 +83,14 @@ pub trait Edge: Copy + Send + Debug + PartialEq + Eq + Hash { } } -/// A simple edge implementation that represents a word-sized slot where an ObjectReference value -/// is stored as is. It is the default edge type, and should be suitable for most VMs. +/// A simple edge implementation that represents a word-sized slot which holds the raw address of +/// an `ObjectReference`, or 0 if it is holding a null reference. +/// +/// It is the default edge type, and should be suitable for most VMs. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] #[repr(transparent)] pub struct SimpleEdge { - slot_addr: *mut Atomic, + slot_addr: *mut Atomic
, } impl SimpleEdge { @@ -113,12 +115,13 @@ impl SimpleEdge { unsafe impl Send for SimpleEdge {} impl Edge for SimpleEdge { - fn load(&self) -> ObjectReference { - unsafe { (*self.slot_addr).load(atomic::Ordering::Relaxed) } + fn load(&self) -> Option { + let addr = unsafe { (*self.slot_addr).load(atomic::Ordering::Relaxed) }; + ObjectReference::from_raw_address(addr) } fn store(&self, object: ObjectReference) { - unsafe { (*self.slot_addr).store(object, atomic::Ordering::Relaxed) } + unsafe { (*self.slot_addr).store(object.to_raw_address(), atomic::Ordering::Relaxed) } } } @@ -133,8 +136,9 @@ impl Edge for SimpleEdge { /// simply as an `ObjectReference`. The intention and the semantics are clearer with /// `SimpleEdge`. impl Edge for Address { - fn load(&self) -> ObjectReference { - unsafe { Address::load(*self) } + fn load(&self) -> Option { + let addr = unsafe { Address::load(*self) }; + ObjectReference::from_raw_address(addr) } fn store(&self, object: ObjectReference) { diff --git a/src/vm/object_model.rs b/src/vm/object_model.rs index a185268e8c..9449be2564 100644 --- a/src/vm/object_model.rs +++ b/src/vm/object_model.rs @@ -424,7 +424,7 @@ pub trait ObjectModel { /// the address that a binding gets by an allocation call ([`crate::memory_manager::alloc`]). /// /// Arguments: - /// * `object`: The object to be queried. It should not be null. + /// * `object`: The object to be queried. fn ref_to_object_start(object: ObjectReference) -> Address; /// Return the header base address from an object reference. Any object header metadata @@ -433,7 +433,7 @@ pub trait ObjectModel { /// will not be called, and the binding can simply use `unreachable!()` for the method. /// /// Arguments: - /// * `object`: The object to be queried. It should not be null. + /// * `object`: The object to be queried. fn ref_to_header(object: ObjectReference) -> Address; /// Return an address guaranteed to be inside the storage associated @@ -449,7 +449,7 @@ pub trait ObjectModel { /// MMTk uses this method more frequently than [`crate::vm::ObjectModel::ref_to_object_start`]. /// /// Arguments: - /// * `object`: The object to be queried. It should not be null. + /// * `object`: The object to be queried. fn ref_to_address(object: ObjectReference) -> Address; /// Return an object for a given address returned by `ref_to_address()`. diff --git a/src/vm/reference_glue.rs b/src/vm/reference_glue.rs index 3b1ef98852..1b27b97ade 100644 --- a/src/vm/reference_glue.rs +++ b/src/vm/reference_glue.rs @@ -23,15 +23,14 @@ pub trait ReferenceGlue { /// /// Arguments: /// * `new_reference`: The reference whose referent is to be cleared. - fn clear_referent(new_reference: ObjectReference) { - Self::set_referent(new_reference, ObjectReference::NULL); - } + fn clear_referent(new_reference: ObjectReference); /// Get the referent from a weak reference object. /// /// Arguments: - /// * `object`: The object reference. - fn get_referent(object: ObjectReference) -> ObjectReference; + /// * `object`: Reference to the referent. `None`` if the object currently does not point to a + /// referent. This may happen if the reference has been cleared. + fn get_referent(object: ObjectReference) -> Option; /// Set the referent in a weak reference object. /// @@ -40,14 +39,6 @@ pub trait ReferenceGlue { /// * `referent`: The referent object reference. fn set_referent(reff: ObjectReference, referent: ObjectReference); - /// Check if the referent has been cleared. - /// - /// Arguments: - /// * `referent`: The referent object reference. - fn is_referent_cleared(referent: ObjectReference) -> bool { - referent.is_null() - } - /// For weak reference types, if the referent is cleared during GC, the reference /// will be added to a queue, and MMTk will call this method to inform /// the VM about the changes for those references. This method is used diff --git a/src/vm/scanning.rs b/src/vm/scanning.rs index df6a0c6607..9d3951d0d9 100644 --- a/src/vm/scanning.rs +++ b/src/vm/scanning.rs @@ -16,7 +16,7 @@ impl EdgeVisitor for F { fn visit_edge(&mut self, edge: ES) { #[cfg(debug_assertions)] trace!( - "(FunctionClosure) Visit edge {:?} (pointing to {})", + "(FunctionClosure) Visit edge {:?} (pointing to {:?})", edge, edge.load() ); @@ -27,7 +27,6 @@ impl EdgeVisitor for F { /// Callback trait of scanning functions that directly trace through edges. pub trait ObjectTracer { /// Call this function to trace through an object graph edge which points to `object`. - /// `object` must point to a valid object, and cannot be `ObjectReference::NULL`. /// /// The return value is the new object reference for `object` if it is moved, or `object` if /// not moved. If moved, the caller should update the slot that holds the reference to 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 bdbabec5fb..e244a596d7 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 @@ -26,7 +26,7 @@ fn test_assertion_barrier_invalid_ref() { // Create an invalid object reference (offset 8 bytes on the original object ref), and invoke barrier slowpath with it // The invalid object ref has no VO bit, and the assertion should fail. let invalid_objref = - ObjectReference::from_raw_address(objref.to_raw_address() + 8usize); + ObjectReference::from_raw_address(objref.to_raw_address() + 8usize).unwrap(); fixture.mutator_mut().barrier.object_reference_write_slow( invalid_objref, edge, diff --git a/src/vm/tests/mock_tests/mock_test_edges.rs b/src/vm/tests/mock_tests/mock_test_edges.rs index b443ca3a69..d068b40e4a 100644 --- a/src/vm/tests/mock_tests/mock_test_edges.rs +++ b/src/vm/tests/mock_tests/mock_test_edges.rs @@ -27,7 +27,7 @@ mod simple_edges { let edge = SimpleEdge::from_address(Address::from_ref(&slot)); let objref = edge.load(); - assert_eq!(objref, fixture.objref1); + assert_eq!(objref, Some(fixture.objref1)); }); }, no_cleanup, @@ -47,7 +47,7 @@ mod simple_edges { assert_eq!(slot.load(Ordering::SeqCst), fixture.objref2); let objref = edge.load(); - assert_eq!(objref, fixture.objref2); + assert_eq!(objref, Some(fixture.objref2)); }); }, no_cleanup, @@ -81,7 +81,7 @@ mod compressed_oop { } impl Edge for CompressedOopEdge { - fn load(&self) -> ObjectReference { + fn load(&self) -> Option { let compressed = unsafe { (*self.slot_addr).load(atomic::Ordering::Relaxed) }; let expanded = (compressed as usize) << 3; ObjectReference::from_raw_address(unsafe { Address::from_usize(expanded) }) @@ -121,7 +121,8 @@ mod compressed_oop { let compressed1 = (COMPRESSABLE_ADDR1 >> 3) as u32; let compressed2 = (COMPRESSABLE_ADDR2 >> 3) as u32; let objref2 = - ObjectReference::from_raw_address(unsafe { Address::from_usize(COMPRESSABLE_ADDR2) }); + ObjectReference::from_raw_address(unsafe { Address::from_usize(COMPRESSABLE_ADDR2) }) + .unwrap(); let mut slot: Atomic = Atomic::new(compressed1); @@ -130,7 +131,7 @@ mod compressed_oop { assert_eq!(slot.load(Ordering::SeqCst), compressed2); let objref = edge.load(); - assert_eq!(objref, objref2); + assert_eq!(objref, Some(objref2)); } } @@ -173,7 +174,7 @@ mod offset_edge { } impl Edge for OffsetEdge { - fn load(&self) -> ObjectReference { + fn load(&self) -> Option { let middle = unsafe { (*self.slot_addr).load(atomic::Ordering::Relaxed) }; let begin = middle - self.offset; ObjectReference::from_raw_address(begin) @@ -200,7 +201,7 @@ mod offset_edge { let edge = OffsetEdge::new_with_offset(Address::from_ref(&slot), OFFSET); let objref = edge.load(); - assert_eq!(objref, fixture.objref1); + assert_eq!(objref, Some(fixture.objref1)); }); }, no_cleanup, @@ -222,7 +223,7 @@ mod offset_edge { assert_eq!(slot.load(Ordering::SeqCst), addr2 + OFFSET); let objref = edge.load(); - assert_eq!(objref, fixture.objref2); + assert_eq!(objref, Some(fixture.objref2)); }); }, no_cleanup, @@ -254,7 +255,7 @@ mod tagged_edge { } impl Edge for TaggedEdge { - fn load(&self) -> ObjectReference { + fn load(&self) -> Option { let tagged = unsafe { (*self.slot_addr).load(atomic::Ordering::Relaxed) }; let untagged = tagged & !Self::TAG_BITS_MASK; ObjectReference::from_raw_address(unsafe { Address::from_usize(untagged) }) @@ -288,8 +289,8 @@ mod tagged_edge { let objref2 = edge2.load(); // Tags should not affect loaded values. - assert_eq!(objref1, fixture.objref1); - assert_eq!(objref2, fixture.objref1); + assert_eq!(objref1, Some(fixture.objref1)); + assert_eq!(objref2, Some(fixture.objref1)); }); }, no_cleanup, @@ -326,8 +327,8 @@ mod tagged_edge { let objref2 = edge2.load(); // Tags should not affect loaded values. - assert_eq!(objref1, fixture.objref2); - assert_eq!(objref2, fixture.objref2); + assert_eq!(objref1, Some(fixture.objref2)); + assert_eq!(objref2, Some(fixture.objref2)); }); }, no_cleanup, @@ -358,7 +359,7 @@ mod mixed { unsafe impl Send for DummyVMEdge {} impl Edge for DummyVMEdge { - fn load(&self) -> ObjectReference { + fn load(&self) -> Option { match self { DummyVMEdge::Simple(e) => e.load(), #[cfg(target_pointer_width = "64")] @@ -405,7 +406,12 @@ mod mixed { let edges = [de1, de3, de4]; for (i, edge) in edges.iter().enumerate() { let objref = edge.load(); - assert_eq!(objref, fixture.objref1, "Edge {} is not properly loaded", i); + assert_eq!( + objref, + Some(fixture.objref1), + "Edge {} is not properly loaded", + i + ); } let mutable_edges = [de1, de3, de4]; @@ -413,7 +419,8 @@ mod mixed { edge.store(fixture.objref2); let objref = edge.load(); assert_eq!( - objref, fixture.objref2, + objref, + Some(fixture.objref2), "Edge {} is not properly loaded after store", i ); diff --git a/src/vm/tests/mock_tests/mock_test_is_in_mmtk_spaces.rs b/src/vm/tests/mock_tests/mock_test_is_in_mmtk_spaces.rs index 45cf7ce535..97a620f2ea 100644 --- a/src/vm/tests/mock_tests/mock_test_is_in_mmtk_spaces.rs +++ b/src/vm/tests/mock_tests/mock_test_is_in_mmtk_spaces.rs @@ -9,14 +9,20 @@ lazy_static! { } #[test] -pub fn null() { +pub fn near_zero() { with_mockvm( default_setup, || { SINGLE_OBJECT.with_fixture(|_| { + // FIXME: `is_in_mmtk_space` will crash if we pass it an address lower than + // DEFAULT_OBJECT_REF_OFFSET. We need to clarify its requirement on the argument, + // and decide if we need to test calling `is_in_mmtk_space` with 0 as an argument. + let addr = unsafe { Address::from_usize(DEFAULT_OBJECT_REF_OFFSET) }; assert!( - !memory_manager::is_in_mmtk_spaces::(ObjectReference::NULL), - "NULL pointer should not be in any MMTk spaces." + !memory_manager::is_in_mmtk_spaces::( + ObjectReference::from_raw_address(addr).unwrap() + ), + "A very low address {addr} should not be in any MMTk spaces." ); }); }, @@ -32,7 +38,7 @@ pub fn max() { SINGLE_OBJECT.with_fixture(|_fixture| { assert!( !memory_manager::is_in_mmtk_spaces::( - ObjectReference::from_raw_address(Address::MAX) + ObjectReference::from_raw_address(Address::MAX).unwrap() ), "Address::MAX should not be in any MMTk spaces." ); @@ -78,7 +84,7 @@ pub fn large_offsets_aligned() { // It's just a smoke test. It is hard to predict if the addr is still in any space, // but it must not crash. let _ = memory_manager::is_in_mmtk_spaces::( - ObjectReference::from_raw_address(addr), + ObjectReference::from_raw_address(addr).unwrap(), ); } }); @@ -107,7 +113,7 @@ pub fn negative_offsets() { // It's just a smoke test. It is hard to predict if the addr is still in any space, // but it must not crash. let _ = memory_manager::is_in_mmtk_spaces::( - ObjectReference::from_raw_address(addr), + ObjectReference::from_raw_address(addr).unwrap(), ); } });