Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unaligned edge access #232

Merged
merged 2 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions mmtk/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ pub extern "C" fn mmtk_object_reference_write_pre(
) {
mutator
.barrier()
.object_reference_write_pre(src, slot, target);
.object_reference_write_pre(src, slot.into(), target);
}

/// Full post barrier
Expand All @@ -356,7 +356,7 @@ pub extern "C" fn mmtk_object_reference_write_post(
) {
mutator
.barrier()
.object_reference_write_post(src, slot, target);
.object_reference_write_post(src, slot.into(), target);
}

/// Barrier slow-path call
Expand All @@ -369,7 +369,7 @@ pub extern "C" fn mmtk_object_reference_write_slow(
) {
mutator
.barrier()
.object_reference_write_slow(src, slot, target);
.object_reference_write_slow(src, slot.into(), target);
}

/// Array-copy pre-barrier
Expand All @@ -383,7 +383,7 @@ pub extern "C" fn mmtk_array_copy_pre(
let bytes = count << LOG_BYTES_IN_ADDRESS;
mutator
.barrier()
.memory_region_copy_pre(src..src + bytes, dst..dst + bytes);
.memory_region_copy_pre((src..src + bytes).into(), (dst..dst + bytes).into());
}

/// Array-copy post-barrier
Expand All @@ -397,7 +397,7 @@ pub extern "C" fn mmtk_array_copy_post(
let bytes = count << LOG_BYTES_IN_ADDRESS;
mutator
.barrier()
.memory_region_copy_post(src..src + bytes, dst..dst + bytes);
.memory_region_copy_post((src..src + bytes).into(), (dst..dst + bytes).into());
}

/// C2 Slowpath allocation barrier
Expand Down
103 changes: 103 additions & 0 deletions mmtk/src/edges.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
use std::ops::Range;

use mmtk::{
util::{Address, ObjectReference},
vm::edge_shape::{AddressRangeIterator, Edge, MemorySlice},
};

/// The type of edges in OpenJDK.
/// Currently it has the same layout as `Address`, but we override its load and store methods.
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
#[repr(transparent)]
pub struct OpenJDKEdge {
pub addr: Address,
}

impl From<Address> for OpenJDKEdge {
fn from(value: Address) -> Self {
Self { addr: value }
}
}

impl Edge for OpenJDKEdge {
fn load(&self) -> ObjectReference {
if cfg!(any(target_arch = "x86", target_arch = "x86_64")) {
// Workaround: On x86 (including x86_64), machine instructions may contain pointers as
// immediates, and they may be unaligned. It is an undefined behavior in Rust to
// dereference unaligned pointers. We have to explicitly use unaligned memory access
// methods. On x86, ordinary MOV instructions can load and store memory at unaligned
// addresses, so we expect `ptr.read_unaligned()` to have no performance penalty over
// `ptr.read()` if `ptr` is actually aligned.
unsafe {
let ptr = self.addr.to_ptr::<ObjectReference>();
ptr.read_unaligned()
}
} else {
unsafe { self.addr.load() }
}
}

fn store(&self, object: ObjectReference) {
if cfg!(any(target_arch = "x86", target_arch = "x86_64")) {
unsafe {
let ptr = self.addr.to_mut_ptr::<ObjectReference>();
ptr.write_unaligned(object)
}
} else {
unsafe { self.addr.store(object) }
}
}
}

/// A range of OpenJDKEdge, usually used for arrays.
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
pub struct OpenJDKEdgeRange {
range: Range<Address>,
}

impl From<Range<Address>> for OpenJDKEdgeRange {
fn from(value: Range<Address>) -> Self {
Self { range: value }
}
}

pub struct OpenJDKEdgeRangeIterator {
inner: AddressRangeIterator,
}

impl Iterator for OpenJDKEdgeRangeIterator {
type Item = OpenJDKEdge;

fn next(&mut self) -> Option<Self::Item> {
self.inner.next().map(|a| a.into())
}
}

// Note that we cannot implement MemorySlice for `Range<OpenJDKEdgeRange>` because neither
// `MemorySlice` nor `Range<T>` are defined in the `mmtk-openjdk` crate. ("orphan rule")
impl MemorySlice for OpenJDKEdgeRange {
type Edge = OpenJDKEdge;
type EdgeIterator = OpenJDKEdgeRangeIterator;

fn iter_edges(&self) -> Self::EdgeIterator {
OpenJDKEdgeRangeIterator {
inner: self.range.iter_edges(),
}
}

fn object(&self) -> Option<ObjectReference> {
self.range.object()
}

fn start(&self) -> Address {
self.range.start()
}

fn bytes(&self) -> usize {
self.range.bytes()
}

fn copy(src: &Self, tgt: &Self) {
MemorySlice::copy(&src.range, &tgt.range)
}
}
2 changes: 1 addition & 1 deletion mmtk/src/gc_work.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl<F: RootsWorkFactory<OpenJDKEdge>> GCWork<OpenJDK> for ScanCodeCacheRoots<F>
let mut edges = Vec::with_capacity(crate::CODE_CACHE_ROOTS_SIZE.load(Ordering::Relaxed));
for roots in (*crate::CODE_CACHE_ROOTS.lock().unwrap()).values() {
for r in roots {
edges.push(*r)
edges.push((*r).into())
}
}
// Create work packet
Expand Down
12 changes: 3 additions & 9 deletions mmtk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
extern crate lazy_static;

use std::collections::HashMap;
use std::ops::Range;
use std::ptr::null_mut;
use std::sync::atomic::AtomicUsize;
use std::sync::Mutex;

use edges::{OpenJDKEdge, OpenJDKEdgeRange};
use libc::{c_char, c_void, uintptr_t};
use mmtk::util::alloc::AllocationError;
use mmtk::util::opaque_pointer::*;
Expand All @@ -19,6 +19,7 @@ pub mod active_plan;
pub mod api;
mod build_info;
pub mod collection;
mod edges;
mod gc_work;
pub mod object_model;
mod object_scanning;
Expand Down Expand Up @@ -135,13 +136,6 @@ pub static FREE_LIST_ALLOCATOR_SIZE: uintptr_t =
#[derive(Default)]
pub struct OpenJDK;

/// The type of edges in OpenJDK.
///
/// TODO: We currently make it an alias to Address to make the change minimal.
/// If we support CompressedOOPs, we should define an enum type to support both
/// compressed and uncompressed OOPs.
pub type OpenJDKEdge = Address;

impl VMBinding for OpenJDK {
type VMObjectModel = object_model::VMObjectModel;
type VMScanning = scanning::VMScanning;
Expand All @@ -150,7 +144,7 @@ impl VMBinding for OpenJDK {
type VMReferenceGlue = reference_glue::VMReferenceGlue;

type VMEdge = OpenJDKEdge;
type VMMemorySlice = Range<Address>;
type VMMemorySlice = OpenJDKEdgeRange;

const MIN_ALIGNMENT: usize = 8;
const MAX_ALIGNMENT: usize = 8;
Expand Down
10 changes: 5 additions & 5 deletions mmtk/src/object_scanning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ impl OopIterate for OopMapBlock {
fn oop_iterate(&self, oop: Oop, closure: &mut impl EdgeVisitor<OpenJDKEdge>) {
let start = oop.get_field_address(self.offset);
for i in 0..self.count as usize {
let edge = start + (i << LOG_BYTES_IN_ADDRESS);
let edge = (start + (i << LOG_BYTES_IN_ADDRESS)).into();
closure.visit_edge(edge);
}
}
Expand Down Expand Up @@ -66,7 +66,7 @@ impl OopIterate for InstanceMirrorKlass {
let len = Self::static_oop_field_count(oop);
let slice = unsafe { slice::from_raw_parts(start, len as _) };
for oop in slice {
closure.visit_edge(Address::from_ref(oop as &Oop));
closure.visit_edge(Address::from_ref(oop as &Oop).into());
}
}
}
Expand All @@ -88,7 +88,7 @@ impl OopIterate for ObjArrayKlass {
fn oop_iterate(&self, oop: Oop, closure: &mut impl EdgeVisitor<OpenJDKEdge>) {
let array = unsafe { oop.as_array_oop() };
for oop in unsafe { array.data::<Oop>(BasicType::T_OBJECT) } {
closure.visit_edge(Address::from_ref(oop as &Oop));
closure.visit_edge(Address::from_ref(oop as &Oop).into());
}
}
}
Expand Down Expand Up @@ -133,9 +133,9 @@ impl InstanceRefKlass {
}
fn process_ref_as_strong(oop: Oop, closure: &mut impl EdgeVisitor<OpenJDKEdge>) {
let referent_addr = Self::referent_address(oop);
closure.visit_edge(referent_addr);
closure.visit_edge(referent_addr.into());
let discovered_addr = Self::discovered_address(oop);
closure.visit_edge(discovered_addr);
closure.visit_edge(discovered_addr.into());
}
}

Expand Down
4 changes: 3 additions & 1 deletion mmtk/src/scanning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ extern "C" fn report_edges_and_renew_buffer<F: RootsWorkFactory<OpenJDKEdge>>(
factory_ptr: *mut libc::c_void,
) -> NewBuffer {
if !ptr.is_null() {
let buf = unsafe { Vec::<Address>::from_raw_parts(ptr, length, capacity) };
// Note: Currently OpenJDKEdge has the same layout as Address. If the layout changes, we
// should fix the Rust-to-C interface.
let buf = unsafe { Vec::<OpenJDKEdge>::from_raw_parts(ptr as _, length, capacity) };
let factory: &mut F = unsafe { &mut *(factory_ptr as *mut F) };
factory.create_process_edge_roots_work(buf);
}
Expand Down
Loading