From 3be73b8048df17f1f2bd019d0b109488a2c5e313 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Mon, 17 Jun 2024 16:42:43 +0800 Subject: [PATCH] Fix clippy warnings for Rust 1.79 (#1151) This PR fixes new warnings generated by Clippy after upgrading Rust to 1.79. - Only define `ImmixSpaceArg::mixed_age` when the "vo_bit" feature is enabled. This eliminates a dead code warning. - Use the associated constant `::MAX` (and `MIN`, too) instead of the deprecated `max_value()` method and the `::std::::MAX` constant. - Annotate the destination type of some `transmute` calls. - Use the safe `std::ptr::NonNull::from()` converter instead of `transmute`. - Initialize array elements of type `Atomic` from constant. - Remove `MutatorContext::barrier_impl`. It is not used by any bindings right now, and it depends on the layout of `&dyn Trait`. Since `Barrier` has `Downcast` as a supertrait, if a binding needs a reference to a concrete `Barrier` implementation, it should just use downcast. The warning against the `transmute` call in `mock_vm.rs` is suppressed because it is almost impossible to annotate. Also fixes compilation problems: - Made benchmarks related to the "mock_test" feature conditionally compiled so that we don't get compilation error when running `cargo clippy --benches` or `cargo clippy --all-targets`. --- benches/main.rs | 17 ++++++++++++----- benches/{ => mock_bench}/alloc.rs | 0 benches/mock_bench/mod.rs | 2 ++ benches/{ => mock_bench}/sft.rs | 0 src/plan/generational/immix/global.rs | 1 + src/plan/immix/global.rs | 1 + src/plan/mutator_context.rs | 12 ------------ src/plan/sticky/immix/global.rs | 1 + src/policy/immix/immixspace.rs | 2 ++ src/policy/sft_map.rs | 8 ++++++-- src/util/address.rs | 3 +-- src/util/constants.rs | 4 ++-- src/util/erase_vm.rs | 3 ++- src/util/heap/layout/byte_map_mmapper.rs | 18 ++++++++++++++---- src/util/heap/layout/fragmented_mapper.rs | 17 ++++++++++++++--- src/util/heap/space_descriptor.rs | 2 +- src/util/int_array_freelist.rs | 5 +++-- src/util/options.rs | 2 +- src/util/test_util/mock_vm.rs | 7 ++++++- 19 files changed, 69 insertions(+), 36 deletions(-) rename benches/{ => mock_bench}/alloc.rs (100%) create mode 100644 benches/mock_bench/mod.rs rename benches/{ => mock_bench}/sft.rs (100%) diff --git a/benches/main.rs b/benches/main.rs index 92b836a40c..b759bebeaf 100644 --- a/benches/main.rs +++ b/benches/main.rs @@ -19,18 +19,25 @@ use criterion::Criterion; // However, I will just keep these benchmarks here. If we find it not useful, and we do not plan to improve MockVM, we can delete // them. -mod alloc; -mod sft; +#[cfg(feature = "mock_test")] +mod mock_bench; -fn bench_main(c: &mut Criterion) { +pub fn bench_main(_c: &mut Criterion) { + #[cfg(feature = "mock_test")] match std::env::var("MMTK_BENCH") { Ok(bench) => match bench.as_str() { - "alloc" => alloc::bench(c), - "sft" => sft::bench(c), + "alloc" => mock_bench::alloc::bench(_c), + "sft" => mock_bench::sft::bench(_c), _ => panic!("Unknown benchmark {:?}", bench), }, Err(_) => panic!("Need to name a benchmark by the env var MMTK_BENCH"), } + + #[cfg(not(feature = "mock_test"))] + { + eprintln!("ERROR: Currently there are no benchmarks when the \"mock_test\" feature is not enabled."); + std::process::exit(1); + } } criterion_group!(benches, bench_main); diff --git a/benches/alloc.rs b/benches/mock_bench/alloc.rs similarity index 100% rename from benches/alloc.rs rename to benches/mock_bench/alloc.rs diff --git a/benches/mock_bench/mod.rs b/benches/mock_bench/mod.rs new file mode 100644 index 0000000000..532ed23009 --- /dev/null +++ b/benches/mock_bench/mod.rs @@ -0,0 +1,2 @@ +pub mod alloc; +pub mod sft; diff --git a/benches/sft.rs b/benches/mock_bench/sft.rs similarity index 100% rename from benches/sft.rs rename to benches/mock_bench/sft.rs diff --git a/src/plan/generational/immix/global.rs b/src/plan/generational/immix/global.rs index ab0ccbf58d..85a89b3f3e 100644 --- a/src/plan/generational/immix/global.rs +++ b/src/plan/generational/immix/global.rs @@ -251,6 +251,7 @@ impl GenImmix { // Any object is moved into the mature space, or is copied inside the mature space. We will unlog it. unlog_object_when_traced: false, // In GenImmix, young objects are not allocated in ImmixSpace directly. + #[cfg(feature = "vo_bit")] mixed_age: false, }, ); diff --git a/src/plan/immix/global.rs b/src/plan/immix/global.rs index 6292b01aa2..d651183a69 100644 --- a/src/plan/immix/global.rs +++ b/src/plan/immix/global.rs @@ -138,6 +138,7 @@ impl Immix { ImmixSpaceArgs { reset_log_bit_in_major_gc: false, unlog_object_when_traced: false, + #[cfg(feature = "vo_bit")] mixed_age: false, }, ) diff --git a/src/plan/mutator_context.rs b/src/plan/mutator_context.rs index 5b2ce60703..8bf266f430 100644 --- a/src/plan/mutator_context.rs +++ b/src/plan/mutator_context.rs @@ -153,13 +153,6 @@ impl MutatorContext for Mutator { self.mutator_tls } - /// Used by specialized barrier slow-path calls to avoid dynamic dispatches. - unsafe fn barrier_impl>(&mut self) -> &mut B { - debug_assert!(self.barrier().is::()); - let (payload, _vptr) = std::mem::transmute::<_, (*mut B, *mut ())>(self.barrier()); - &mut *payload - } - fn barrier(&mut self) -> &mut dyn Barrier { &mut *self.barrier } @@ -316,11 +309,6 @@ pub trait MutatorContext: Send + 'static { fn get_tls(&self) -> VMMutatorThread; /// Get active barrier trait object fn barrier(&mut self) -> &mut dyn Barrier; - /// Force cast the barrier trait object to a concrete implementation. - /// - /// # Safety - /// The safety of this function is ensured by a down-cast check. - unsafe fn barrier_impl>(&mut self) -> &mut B; } /// This is used for plans to indicate the number of allocators reserved for the plan. diff --git a/src/plan/sticky/immix/global.rs b/src/plan/sticky/immix/global.rs index 4a0d9ab14c..c9557ac981 100644 --- a/src/plan/sticky/immix/global.rs +++ b/src/plan/sticky/immix/global.rs @@ -329,6 +329,7 @@ impl StickyImmix { // Along with the option above, we unlog them again during tracing. reset_log_bit_in_major_gc: true, // In StickyImmix, both young and old objects are allocated in the ImmixSpace. + #[cfg(feature = "vo_bit")] mixed_age: true, }, ); diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index 8fb0c5fac3..3e94ee9c55 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -77,6 +77,8 @@ pub struct ImmixSpaceArgs { /// instance contain young objects, their VO bits need to be updated during this GC. Currently /// only StickyImmix is affected. GenImmix allocates young objects in a separete CopySpace /// nursery and its VO bits can be cleared in bulk. + // Currently only used when "vo_bit" is enabled. Using #[cfg(...)] to eliminate dead code warning. + #[cfg(feature = "vo_bit")] pub mixed_age: bool, } diff --git a/src/policy/sft_map.rs b/src/policy/sft_map.rs index c0e859842b..8fccf9cd5f 100644 --- a/src/policy/sft_map.rs +++ b/src/policy/sft_map.rs @@ -103,8 +103,12 @@ pub(crate) type SFTRawPointer = *const (dyn SFT + Sync + 'static); /// see if 128 bits atomic instructions are available. #[cfg(target_pointer_width = "64")] type AtomicDoubleWord = portable_atomic::AtomicU128; +#[cfg(target_pointer_width = "64")] +type DoubleWord = u128; #[cfg(target_pointer_width = "32")] type AtomicDoubleWord = portable_atomic::AtomicU64; +#[cfg(target_pointer_width = "32")] +type DoubleWord = u64; /// The type we store SFT raw pointer as. It basically just double word sized atomic integer. /// This type provides an abstraction so we can access SFT easily. @@ -128,7 +132,7 @@ impl SFTRefStorage { } pub fn new(sft: SFTRawPointer) -> Self { - let val = unsafe { std::mem::transmute(sft) }; + let val: DoubleWord = unsafe { std::mem::transmute(sft) }; Self(AtomicDoubleWord::new(val)) } @@ -140,7 +144,7 @@ impl SFTRefStorage { // Store a raw SFT pointer with the release ordering. pub fn store(&self, sft: SFTRawPointer) { - let val = unsafe { std::mem::transmute(sft) }; + let val: DoubleWord = unsafe { std::mem::transmute(sft) }; self.0.store(val, Ordering::Release) } } diff --git a/src/util/address.rs b/src/util/address.rs index b23ef587c7..c38274a0e1 100644 --- a/src/util/address.rs +++ b/src/util/address.rs @@ -134,7 +134,7 @@ impl Address { /// The lowest possible address. pub const ZERO: Self = Address(0); /// The highest possible address. - pub const MAX: Self = Address(usize::max_value()); + pub const MAX: Self = Address(usize::MAX); /// creates Address from a pointer pub fn from_ptr(ptr: *const T) -> Address { @@ -164,7 +164,6 @@ impl Address { /// It is unsafe and the user needs to be aware that they are creating an invalid address. /// The max address should only be used as unininitialized or sentinel values in performance critical code (where you dont want to use `Option
`). pub unsafe fn max() -> Address { - use std::usize; Address(usize::MAX) } diff --git a/src/util/constants.rs b/src/util/constants.rs index b82dce94f1..df119778f7 100644 --- a/src/util/constants.rs +++ b/src/util/constants.rs @@ -78,8 +78,8 @@ mod java_specific_constants { pub const LOG_BITS_IN_LONG: u8 = LOG_BITS_IN_BYTE + LOG_BYTES_IN_LONG; pub const BITS_IN_LONG: usize = 1 << LOG_BITS_IN_LONG; - pub const MAX_INT: usize = i32::max_value() as usize; // 0x7fff_ffff - pub const MIN_INT: usize = i32::min_value() as u32 as usize; // 0x8000_0000 + pub const MAX_INT: usize = i32::MAX as usize; // 0x7fff_ffff + pub const MIN_INT: usize = i32::MIN as u32 as usize; // 0x8000_0000 } pub(crate) use java_specific_constants::*; diff --git a/src/util/erase_vm.rs b/src/util/erase_vm.rs index 0087a7b923..352c0e856f 100644 --- a/src/util/erase_vm.rs +++ b/src/util/erase_vm.rs @@ -17,7 +17,8 @@ macro_rules! define_erased_vm_mut_ref { pub struct $new_type<'a>(usize, PhantomData<&'a ()>); impl<'a> $new_type<'a> { pub fn new(r: &'a mut $orig_type) -> Self { - Self(unsafe { std::mem::transmute(r) }, PhantomData) + let worker_as_usize: usize = unsafe { std::mem::transmute(r) }; + Self(worker_as_usize, PhantomData) } pub fn into_mut(self) -> &'a mut $orig_type { unsafe { std::mem::transmute(self.0) } diff --git a/src/util/heap/layout/byte_map_mmapper.rs b/src/util/heap/layout/byte_map_mmapper.rs index cbf29acb78..ed96aaa89f 100644 --- a/src/util/heap/layout/byte_map_mmapper.rs +++ b/src/util/heap/layout/byte_map_mmapper.rs @@ -12,7 +12,6 @@ use std::sync::Mutex; use atomic::Atomic; use std::io::Result; -use std::mem::transmute; const MMAP_NUM_CHUNKS: usize = if LOG_BYTES_IN_ADDRESS_SPACE == 32 { 1 << (LOG_BYTES_IN_ADDRESS_SPACE as usize - LOG_MMAP_CHUNK_BYTES) @@ -134,11 +133,22 @@ impl Mmapper for ByteMapMmapper { impl ByteMapMmapper { pub fn new() -> Self { - // Hacky because AtomicU8 does not implement Copy - // Should be fiiine because AtomicXXX has the same bit representation as XXX + // Because AtomicU8 does not implement Copy, it is a compilation error to usen the + // expression `[Atomic::new(MapState::Unmapped); MMAP_NUM_CHUNKS]` because that involves + // copying. We must define a constant for it. + // + // TODO: Use the inline const expression `const { Atomic::new(MapState::Unmapped) }` after + // we bump MSRV to 1.79. + + // If we declare a const Atomic, Clippy will warn about const items being interior mutable. + // Using inline const expression will eliminate this warning, but that is experimental until + // 1.79. Fix it after we bump MSRV. + #[allow(clippy::declare_interior_mutable_const)] + const INITIAL_ENTRY: Atomic = Atomic::new(MapState::Unmapped); + ByteMapMmapper { lock: Mutex::new(()), - mapped: unsafe { transmute([MapState::Unmapped; MMAP_NUM_CHUNKS]) }, + mapped: [INITIAL_ENTRY; MMAP_NUM_CHUNKS], strategy: Atomic::new(MmapStrategy::Normal), } } diff --git a/src/util/heap/layout/fragmented_mapper.rs b/src/util/heap/layout/fragmented_mapper.rs index 1223eb74de..93bd626a25 100644 --- a/src/util/heap/layout/fragmented_mapper.rs +++ b/src/util/heap/layout/fragmented_mapper.rs @@ -9,7 +9,6 @@ use atomic::{Atomic, Ordering}; use std::cell::UnsafeCell; use std::fmt; use std::io::Result; -use std::mem::transmute; use std::sync::Mutex; const MMAP_NUM_CHUNKS: usize = 1 << (33 - LOG_MMAP_CHUNK_BYTES); @@ -242,8 +241,20 @@ impl FragmentedMapper { } fn new_slab() -> Box { - let mapped: Box = - Box::new(unsafe { transmute([MapState::Unmapped; MMAP_NUM_CHUNKS]) }); + // Because AtomicU8 does not implement Copy, it is a compilation error to usen the + // expression `[Atomic::new(MapState::Unmapped); MMAP_NUM_CHUNKS]` because that involves + // copying. We must define a constant for it. + // + // TODO: Use the inline const expression `const { Atomic::new(MapState::Unmapped) }` after + // we bump MSRV to 1.79. + + // If we declare a const Atomic, Clippy will warn about const items being interior mutable. + // Using inline const expression will eliminate this warning, but that is experimental until + // 1.79. Fix it after we bump MSRV. + #[allow(clippy::declare_interior_mutable_const)] + const INITIAL_ENTRY: Atomic = Atomic::new(MapState::Unmapped); + + let mapped: Box = Box::new([INITIAL_ENTRY; MMAP_NUM_CHUNKS]); mapped } diff --git a/src/util/heap/space_descriptor.rs b/src/util/heap/space_descriptor.rs index 4303eb9041..ce885a17da 100644 --- a/src/util/heap/space_descriptor.rs +++ b/src/util/heap/space_descriptor.rs @@ -35,7 +35,7 @@ impl SpaceDescriptor { let top = end == vm_layout().heap_end; if vm_layout().force_use_contiguous_spaces { let space_index = if start > vm_layout().heap_end { - ::std::usize::MAX + usize::MAX } else { start >> vm_layout().space_shift_64() }; diff --git a/src/util/int_array_freelist.rs b/src/util/int_array_freelist.rs index e8a6e5b7cf..e59f96807d 100644 --- a/src/util/int_array_freelist.rs +++ b/src/util/int_array_freelist.rs @@ -1,5 +1,5 @@ use super::freelist::*; -use std::{mem, ptr::NonNull}; +use std::ptr::NonNull; #[derive(Debug)] pub struct IntArrayFreeList { @@ -42,11 +42,12 @@ impl IntArrayFreeList { iafl } pub fn from_parent(parent: &IntArrayFreeList, ordinal: i32) -> Self { + let parent_ptr = std::ptr::NonNull::from(parent); let iafl = IntArrayFreeList { head: -(1 + ordinal), heads: parent.heads, table: None, - parent: Some(unsafe { mem::transmute(parent) }), + parent: Some(parent_ptr), }; debug_assert!(-iafl.head <= iafl.heads); iafl diff --git a/src/util/options.rs b/src/util/options.rs index 3d1e70a965..b8f8cebe9d 100644 --- a/src/util/options.rs +++ b/src/util/options.rs @@ -8,7 +8,7 @@ use strum_macros::EnumString; /// The default stress factor. This is set to the max usize, /// which means we will never trigger a stress GC for the default value. -pub const DEFAULT_STRESS_FACTOR: usize = usize::max_value(); +pub const DEFAULT_STRESS_FACTOR: usize = usize::MAX; /// The zeroing approach to use for new object allocations. /// Affects each plan differently. diff --git a/src/util/test_util/mock_vm.rs b/src/util/test_util/mock_vm.rs index ab0cac2d1a..1183639a53 100644 --- a/src/util/test_util/mock_vm.rs +++ b/src/util/test_util/mock_vm.rs @@ -44,7 +44,12 @@ lazy_static! { // we do not store them for access after the mock method returns. macro_rules! lifetime { ($e: expr) => { - unsafe { std::mem::transmute($e) } + unsafe { + // The dynamic nature of this lifetime-removal macro makes it impossible to reason + // about the source and destination types of the `transmute`. + #[allow(clippy::missing_transmute_annotations)] + std::mem::transmute($e) + } }; }