Skip to content

Commit 66e8cb8

Browse files
playXEqinsoon
playXE
and
qinsoon
authored
Remove cast ref to mut everywhere (#893)
- MMTK type now stores `Plan` inside `UnsafeCell` to allow safe mutable access when necessary - Various types that previously had `mut_self()` method now are separated into `Type` and `TypeInner`, second one is stored inside `Type` behind `UnsafeCell` which allows to obtain mutable access when necessary without involving UB. - `&'static` references that were previously stored in some types are now converted into `NonNull<>` --------- Co-authored-by: Yi Lin <[email protected]>
1 parent 0532037 commit 66e8cb8

28 files changed

+478
-315
lines changed

‎.github/scripts/ci-style.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
. $(dirname "$0")/ci-common.sh
22

3-
export RUSTFLAGS="-D warnings"
3+
export RUSTFLAGS="-D warnings -A unknown-lints"
44

55
# --- Check main crate ---
66

‎src/memory_manager.rs

+23-14
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,12 @@ pub fn mmtk_init<VM: VMBinding>(builder: &MMTKBuilder) -> Box<MMTK<VM>> {
8888

8989
#[cfg(feature = "vm_space")]
9090
pub fn lazy_init_vm_space<VM: VMBinding>(mmtk: &'static mut MMTK<VM>, start: Address, size: usize) {
91-
mmtk.plan.base_mut().vm_space.lazy_initialize(start, size);
91+
unsafe {
92+
mmtk.get_plan_mut()
93+
.base_mut()
94+
.vm_space
95+
.lazy_initialize(start, size);
96+
}
9297
}
9398

9499
/// Request MMTk to create a mutator for the given thread. The ownership
@@ -345,7 +350,7 @@ pub fn get_allocator_mapping<VM: VMBinding>(
345350
mmtk: &MMTK<VM>,
346351
semantics: AllocationSemantics,
347352
) -> AllocatorSelector {
348-
mmtk.plan.get_allocator_mapping()[semantics]
353+
mmtk.get_plan().get_allocator_mapping()[semantics]
349354
}
350355

351356
/// The standard malloc. MMTk either uses its own allocator, or forward the call to a
@@ -467,11 +472,14 @@ pub fn start_worker<VM: VMBinding>(
467472
/// Collection::spawn_gc_thread() so that the VM knows the context.
468473
pub fn initialize_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>, tls: VMThread) {
469474
assert!(
470-
!mmtk.plan.is_initialized(),
475+
!mmtk.get_plan().is_initialized(),
471476
"MMTk collection has been initialized (was initialize_collection() already called before?)"
472477
);
473478
mmtk.scheduler.spawn_gc_threads(mmtk, tls);
474-
mmtk.plan.base().initialized.store(true, Ordering::SeqCst);
479+
mmtk.get_plan()
480+
.base()
481+
.initialized
482+
.store(true, Ordering::SeqCst);
475483
probe!(mmtk, collection_initialized);
476484
}
477485

@@ -483,10 +491,10 @@ pub fn initialize_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>, tls: VMThre
483491
/// * `mmtk`: A reference to an MMTk instance.
484492
pub fn enable_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>) {
485493
debug_assert!(
486-
!mmtk.plan.should_trigger_gc_when_heap_is_full(),
494+
!mmtk.get_plan().should_trigger_gc_when_heap_is_full(),
487495
"enable_collection() is called when GC is already enabled."
488496
);
489-
mmtk.plan
497+
mmtk.get_plan()
490498
.base()
491499
.trigger_gc_when_heap_is_full
492500
.store(true, Ordering::SeqCst);
@@ -504,10 +512,10 @@ pub fn enable_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>) {
504512
/// * `mmtk`: A reference to an MMTk instance.
505513
pub fn disable_collection<VM: VMBinding>(mmtk: &'static MMTK<VM>) {
506514
debug_assert!(
507-
mmtk.plan.should_trigger_gc_when_heap_is_full(),
515+
mmtk.get_plan().should_trigger_gc_when_heap_is_full(),
508516
"disable_collection() is called when GC is not enabled."
509517
);
510-
mmtk.plan
518+
mmtk.get_plan()
511519
.base()
512520
.trigger_gc_when_heap_is_full
513521
.store(false, Ordering::SeqCst);
@@ -538,7 +546,7 @@ pub fn process_bulk(builder: &mut MMTKBuilder, options: &str) -> bool {
538546
/// Arguments:
539547
/// * `mmtk`: A reference to an MMTk instance.
540548
pub fn used_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
541-
mmtk.plan.get_used_pages() << LOG_BYTES_IN_PAGE
549+
mmtk.get_plan().get_used_pages() << LOG_BYTES_IN_PAGE
542550
}
543551

544552
/// Return free memory in bytes. MMTk accounts for memory in pages, thus this method always returns a value in
@@ -547,7 +555,7 @@ pub fn used_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
547555
/// Arguments:
548556
/// * `mmtk`: A reference to an MMTk instance.
549557
pub fn free_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
550-
mmtk.plan.get_free_pages() << LOG_BYTES_IN_PAGE
558+
mmtk.get_plan().get_free_pages() << LOG_BYTES_IN_PAGE
551559
}
552560

553561
/// Return the size of all the live objects in bytes in the last GC. MMTk usually accounts for memory in pages.
@@ -558,7 +566,7 @@ pub fn free_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
558566
/// to call this method is at the end of a GC (e.g. when the runtime is about to resume threads).
559567
#[cfg(feature = "count_live_bytes_in_gc")]
560568
pub fn live_bytes_in_last_gc<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
561-
mmtk.plan
569+
mmtk.get_plan()
562570
.base()
563571
.live_bytes_in_last_gc
564572
.load(Ordering::SeqCst)
@@ -581,7 +589,7 @@ pub fn last_heap_address() -> Address {
581589
/// Arguments:
582590
/// * `mmtk`: A reference to an MMTk instance.
583591
pub fn total_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
584-
mmtk.plan.get_total_pages() << LOG_BYTES_IN_PAGE
592+
mmtk.get_plan().get_total_pages() << LOG_BYTES_IN_PAGE
585593
}
586594

587595
/// Trigger a garbage collection as requested by the user.
@@ -590,7 +598,8 @@ pub fn total_bytes<VM: VMBinding>(mmtk: &MMTK<VM>) -> usize {
590598
/// * `mmtk`: A reference to an MMTk instance.
591599
/// * `tls`: The thread that triggers this collection request.
592600
pub fn handle_user_collection_request<VM: VMBinding>(mmtk: &MMTK<VM>, tls: VMMutatorThread) {
593-
mmtk.plan.handle_user_collection_request(tls, false, false);
601+
mmtk.get_plan()
602+
.handle_user_collection_request(tls, false, false);
594603
}
595604

596605
/// Is the object alive?
@@ -709,7 +718,7 @@ pub fn is_mapped_address(address: Address) -> bool {
709718
/// * `mmtk`: A reference to an MMTk instance.
710719
/// * `object`: The object to check.
711720
pub fn modify_check<VM: VMBinding>(mmtk: &MMTK<VM>, object: ObjectReference) {
712-
mmtk.plan.modify_check(object);
721+
mmtk.get_plan().modify_check(object);
713722
}
714723

715724
/// Add a reference to the list of weak references. A binding may

‎src/mmtk.rs

+23-8
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use crate::util::reference_processor::ReferenceProcessors;
1515
use crate::util::sanity::sanity_checker::SanityChecker;
1616
use crate::vm::ReferenceGlue;
1717
use crate::vm::VMBinding;
18+
use std::cell::UnsafeCell;
1819
use std::default::Default;
1920
use std::sync::atomic::{AtomicBool, Ordering};
2021
use std::sync::Arc;
@@ -30,10 +31,10 @@ lazy_static! {
3031
// TODO: We should refactor this when we know more about how multiple MMTK instances work.
3132

3233
/// A global VMMap that manages the mapping of spaces to virtual memory ranges.
33-
pub static ref VM_MAP: Box<dyn VMMap> = layout::create_vm_map();
34+
pub static ref VM_MAP: Box<dyn VMMap + Send + Sync> = layout::create_vm_map();
3435

3536
/// A global Mmapper for mmaping and protection of virtual memory.
36-
pub static ref MMAPPER: Box<dyn Mmapper> = layout::create_mmapper();
37+
pub static ref MMAPPER: Box<dyn Mmapper + Send + Sync> = layout::create_mmapper();
3738
}
3839

3940
use crate::util::rust_util::InitializeOnce;
@@ -88,7 +89,7 @@ impl Default for MMTKBuilder {
8889
/// *Note that multi-instances is not fully supported yet*
8990
pub struct MMTK<VM: VMBinding> {
9091
pub(crate) options: Arc<Options>,
91-
pub(crate) plan: Box<dyn Plan<VM = VM>>,
92+
pub(crate) plan: UnsafeCell<Box<dyn Plan<VM = VM>>>,
9293
pub(crate) reference_processors: ReferenceProcessors,
9394
pub(crate) finalizable_processor:
9495
Mutex<FinalizableProcessor<<VM::VMReferenceGlue as ReferenceGlue<VM>>::FinalizableType>>,
@@ -100,6 +101,9 @@ pub struct MMTK<VM: VMBinding> {
100101
inside_harness: AtomicBool,
101102
}
102103

104+
unsafe impl<VM: VMBinding> Sync for MMTK<VM> {}
105+
unsafe impl<VM: VMBinding> Send for MMTK<VM> {}
106+
103107
impl<VM: VMBinding> MMTK<VM> {
104108
pub fn new(options: Arc<Options>) -> Self {
105109
// Initialize SFT first in case we need to use this in the constructor.
@@ -136,7 +140,7 @@ impl<VM: VMBinding> MMTK<VM> {
136140

137141
MMTK {
138142
options,
139-
plan,
143+
plan: UnsafeCell::new(plan),
140144
reference_processors: ReferenceProcessors::new(),
141145
finalizable_processor: Mutex::new(FinalizableProcessor::<
142146
<VM::VMReferenceGlue as ReferenceGlue<VM>>::FinalizableType,
@@ -152,20 +156,31 @@ impl<VM: VMBinding> MMTK<VM> {
152156

153157
pub fn harness_begin(&self, tls: VMMutatorThread) {
154158
probe!(mmtk, harness_begin);
155-
self.plan.handle_user_collection_request(tls, true, true);
159+
self.get_plan()
160+
.handle_user_collection_request(tls, true, true);
156161
self.inside_harness.store(true, Ordering::SeqCst);
157-
self.plan.base().stats.start_all();
162+
self.get_plan().base().stats.start_all();
158163
self.scheduler.enable_stat();
159164
}
160165

161166
pub fn harness_end(&'static self) {
162-
self.plan.base().stats.stop_all(self);
167+
self.get_plan().base().stats.stop_all(self);
163168
self.inside_harness.store(false, Ordering::SeqCst);
164169
probe!(mmtk, harness_end);
165170
}
166171

167172
pub fn get_plan(&self) -> &dyn Plan<VM = VM> {
168-
self.plan.as_ref()
173+
unsafe { &**(self.plan.get()) }
174+
}
175+
176+
/// Get the plan as mutable reference.
177+
///
178+
/// # Safety
179+
///
180+
/// This is unsafe because the caller must ensure that the plan is not used by other threads.
181+
#[allow(clippy::mut_from_ref)]
182+
pub unsafe fn get_plan_mut(&self) -> &mut dyn Plan<VM = VM> {
183+
&mut **(self.plan.get())
169184
}
170185

171186
pub fn get_options(&self) -> &Options {

‎src/plan/generational/copying/mutator.rs

+6-3
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,19 @@ pub fn create_gencopy_mutator<VM: VMBinding>(
3232
mutator_tls: VMMutatorThread,
3333
mmtk: &'static MMTK<VM>,
3434
) -> Mutator<VM> {
35-
let gencopy = mmtk.plan.downcast_ref::<GenCopy<VM>>().unwrap();
35+
let gencopy = mmtk.get_plan().downcast_ref::<GenCopy<VM>>().unwrap();
3636
let config = MutatorConfig {
3737
allocator_mapping: &ALLOCATOR_MAPPING,
38-
space_mapping: Box::new(create_gen_space_mapping(&*mmtk.plan, &gencopy.gen.nursery)),
38+
space_mapping: Box::new(create_gen_space_mapping(
39+
mmtk.get_plan(),
40+
&gencopy.gen.nursery,
41+
)),
3942
prepare_func: &gencopy_mutator_prepare,
4043
release_func: &gencopy_mutator_release,
4144
};
4245

4346
Mutator {
44-
allocators: Allocators::<VM>::new(mutator_tls, &*mmtk.plan, &config.space_mapping),
47+
allocators: Allocators::<VM>::new(mutator_tls, mmtk.get_plan(), &config.space_mapping),
4548
barrier: Box::new(ObjectBarrier::new(GenObjectBarrierSemantics::new(
4649
mmtk, gencopy,
4750
))),

‎src/plan/generational/gc_work.rs

+12-2
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,12 @@ impl<E: ProcessEdgesWork> GCWork<E::VM> for ProcessModBuf<E> {
102102
);
103103
}
104104
// scan modbuf only if the current GC is a nursery GC
105-
if mmtk.plan.generational().unwrap().is_current_gc_nursery() {
105+
if mmtk
106+
.get_plan()
107+
.generational()
108+
.unwrap()
109+
.is_current_gc_nursery()
110+
{
106111
// Scan objects in the modbuf and forward pointers
107112
let modbuf = std::mem::take(&mut self.modbuf);
108113
GCWork::do_work(
@@ -135,7 +140,12 @@ impl<E: ProcessEdgesWork> ProcessRegionModBuf<E> {
135140
impl<E: ProcessEdgesWork> GCWork<E::VM> for ProcessRegionModBuf<E> {
136141
fn do_work(&mut self, worker: &mut GCWorker<E::VM>, mmtk: &'static MMTK<E::VM>) {
137142
// Scan modbuf only if the current GC is a nursery GC
138-
if mmtk.plan.generational().unwrap().is_current_gc_nursery() {
143+
if mmtk
144+
.get_plan()
145+
.generational()
146+
.unwrap()
147+
.is_current_gc_nursery()
148+
{
139149
// Collect all the entries in all the slices
140150
let mut edges = vec![];
141151
for slice in &self.modbuf {

‎src/plan/generational/immix/mutator.rs

+6-3
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,19 @@ pub fn create_genimmix_mutator<VM: VMBinding>(
3030
mutator_tls: VMMutatorThread,
3131
mmtk: &'static MMTK<VM>,
3232
) -> Mutator<VM> {
33-
let genimmix = mmtk.plan.downcast_ref::<GenImmix<VM>>().unwrap();
33+
let genimmix = mmtk.get_plan().downcast_ref::<GenImmix<VM>>().unwrap();
3434
let config = MutatorConfig {
3535
allocator_mapping: &ALLOCATOR_MAPPING,
36-
space_mapping: Box::new(create_gen_space_mapping(&*mmtk.plan, &genimmix.gen.nursery)),
36+
space_mapping: Box::new(create_gen_space_mapping(
37+
mmtk.get_plan(),
38+
&genimmix.gen.nursery,
39+
)),
3740
prepare_func: &genimmix_mutator_prepare,
3841
release_func: &genimmix_mutator_release,
3942
};
4043

4144
Mutator {
42-
allocators: Allocators::<VM>::new(mutator_tls, &*mmtk.plan, &config.space_mapping),
45+
allocators: Allocators::<VM>::new(mutator_tls, mmtk.get_plan(), &config.space_mapping),
4346
barrier: Box::new(ObjectBarrier::new(GenObjectBarrierSemantics::new(
4447
mmtk, genimmix,
4548
))),

‎src/plan/global.rs

+10-7
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,9 @@ pub fn create_mutator<VM: VMBinding>(
4040
mmtk: &'static MMTK<VM>,
4141
) -> Box<Mutator<VM>> {
4242
Box::new(match *mmtk.options.plan {
43-
PlanSelector::NoGC => crate::plan::nogc::mutator::create_nogc_mutator(tls, &*mmtk.plan),
43+
PlanSelector::NoGC => crate::plan::nogc::mutator::create_nogc_mutator(tls, mmtk.get_plan()),
4444
PlanSelector::SemiSpace => {
45-
crate::plan::semispace::mutator::create_ss_mutator(tls, &*mmtk.plan)
45+
crate::plan::semispace::mutator::create_ss_mutator(tls, mmtk.get_plan())
4646
}
4747
PlanSelector::GenCopy => {
4848
crate::plan::generational::copying::mutator::create_gencopy_mutator(tls, mmtk)
@@ -51,14 +51,16 @@ pub fn create_mutator<VM: VMBinding>(
5151
crate::plan::generational::immix::mutator::create_genimmix_mutator(tls, mmtk)
5252
}
5353
PlanSelector::MarkSweep => {
54-
crate::plan::marksweep::mutator::create_ms_mutator(tls, &*mmtk.plan)
54+
crate::plan::marksweep::mutator::create_ms_mutator(tls, mmtk.get_plan())
55+
}
56+
PlanSelector::Immix => {
57+
crate::plan::immix::mutator::create_immix_mutator(tls, mmtk.get_plan())
5558
}
56-
PlanSelector::Immix => crate::plan::immix::mutator::create_immix_mutator(tls, &*mmtk.plan),
5759
PlanSelector::PageProtect => {
58-
crate::plan::pageprotect::mutator::create_pp_mutator(tls, &*mmtk.plan)
60+
crate::plan::pageprotect::mutator::create_pp_mutator(tls, mmtk.get_plan())
5961
}
6062
PlanSelector::MarkCompact => {
61-
crate::plan::markcompact::mutator::create_markcompact_mutator(tls, &*mmtk.plan)
63+
crate::plan::markcompact::mutator::create_markcompact_mutator(tls, mmtk.get_plan())
6264
}
6365
PlanSelector::StickyImmix => {
6466
crate::plan::sticky::immix::mutator::create_stickyimmix_mutator(tls, mmtk)
@@ -137,7 +139,7 @@ pub fn create_gc_worker_context<VM: VMBinding>(
137139
tls: VMWorkerThread,
138140
mmtk: &'static MMTK<VM>,
139141
) -> GCWorkerCopyContext<VM> {
140-
GCWorkerCopyContext::<VM>::new(tls, &*mmtk.plan, mmtk.plan.create_copy_config())
142+
GCWorkerCopyContext::<VM>::new(tls, mmtk.get_plan(), mmtk.get_plan().create_copy_config())
141143
}
142144

143145
/// A plan describes the global core functionality for all memory management schemes.
@@ -857,6 +859,7 @@ impl<VM: VMBinding> BasePlan<VM> {
857859
}
858860

859861
#[allow(unused_variables)] // depending on the enabled features, base may not be used.
862+
#[allow(clippy::needless_pass_by_ref_mut)] // depending on the enabled features, base may not be used.
860863
pub(crate) fn verify_side_metadata_sanity(
861864
&self,
862865
side_metadata_sanity_checker: &mut SideMetadataSanity,

‎src/plan/markcompact/gc_work.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ impl<VM: VMBinding> GCWork<VM> for UpdateReferences<VM> {
3939
fn do_work(&mut self, _worker: &mut GCWorker<VM>, mmtk: &'static MMTK<VM>) {
4040
// The following needs to be done right before the second round of root scanning
4141
VM::VMScanning::prepare_for_roots_re_scanning();
42-
mmtk.plan.base().prepare_for_stack_scanning();
42+
mmtk.get_plan().base().prepare_for_stack_scanning();
4343
#[cfg(feature = "extreme_assertions")]
4444
mmtk.edge_logger.reset();
4545

‎src/plan/sticky/immix/mutator.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,12 @@ pub fn create_stickyimmix_mutator<VM: VMBinding>(
2424
mutator_tls: VMMutatorThread,
2525
mmtk: &'static MMTK<VM>,
2626
) -> Mutator<VM> {
27-
let stickyimmix = mmtk.plan.downcast_ref::<StickyImmix<VM>>().unwrap();
27+
let stickyimmix = mmtk.get_plan().downcast_ref::<StickyImmix<VM>>().unwrap();
2828
let config = MutatorConfig {
2929
allocator_mapping: &ALLOCATOR_MAPPING,
3030
space_mapping: Box::new({
3131
let mut vec =
32-
create_space_mapping(immix::mutator::RESERVED_ALLOCATORS, true, &*mmtk.plan);
32+
create_space_mapping(immix::mutator::RESERVED_ALLOCATORS, true, mmtk.get_plan());
3333
vec.push((AllocatorSelector::Immix(0), stickyimmix.get_immix_space()));
3434
vec
3535
}),
@@ -38,13 +38,13 @@ pub fn create_stickyimmix_mutator<VM: VMBinding>(
3838
};
3939

4040
Mutator {
41-
allocators: Allocators::<VM>::new(mutator_tls, &*mmtk.plan, &config.space_mapping),
41+
allocators: Allocators::<VM>::new(mutator_tls, mmtk.get_plan(), &config.space_mapping),
4242
barrier: Box::new(ObjectBarrier::new(GenObjectBarrierSemantics::new(
4343
mmtk,
4444
stickyimmix,
4545
))),
4646
mutator_tls,
4747
config,
48-
plan: &*mmtk.plan,
48+
plan: mmtk.get_plan(),
4949
}
5050
}

‎src/policy/immix/immixspace.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ impl<VM: VMBinding> crate::policy::gc_work::PolicyTraceObject<VM> for ImmixSpace
195195
if Block::containing::<VM>(object).is_defrag_source() {
196196
debug_assert!(self.in_defrag());
197197
debug_assert!(
198-
!crate::plan::is_nursery_gc(&*worker.mmtk.plan),
198+
!crate::plan::is_nursery_gc(worker.mmtk.get_plan()),
199199
"Calling PolicyTraceObject on Immix in nursery GC"
200200
);
201201
self.trace_object_with_opportunistic_copy(

0 commit comments

Comments
 (0)