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

Add replay frames #29

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Add replay frames #29

wants to merge 9 commits into from

Conversation

uzervlad
Copy link
Contributor

@uzervlad uzervlad commented Nov 2, 2023

Pretty sure this could be optimized even more

Draft until a legitimate use case is present

Copy link
Contributor

@MaxOhn MaxOhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't checked read_struct_ptr_array yet but got some feedback for the other two methods.

Comment on lines 114 to 127
let mut buff = vec![0u8; size_of::<T>()];

let byte_buff = unsafe {
std::slice::from_raw_parts_mut(
buff.as_mut_ptr() as *mut u8,
buff.len()
)
};

self.read(addr, byte_buff.len(), byte_buff)?;

let s: T = unsafe { std::ptr::read(buff.as_ptr() as *const _) };

Ok(s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heap allocation through the Vec is unfortunate but what's worse is that the Vec's data might not be aligned properly w.r.t. T. Both should be fixed by this:

let mut uninit = std::mem::MaybeUninit::<T>::uninit();
let ptr: *mut u8 = uninit.as_mut_ptr().cast();
let slice = unsafe { std::slice::from_raw_parts_mut(ptr, std::mem::size_of::<T>()) };

self.read(addr, slice.len(), slice)?;

Ok(unsafe { uninit.assume_init() })

Might be a good idea to run miri over it to make sure the unsafe code is valid.
Marking the method itself as unsafe might be warranted too since the unsafety invariant basically requires the generic T to be repr(C) which cannot be specified by trait bounds to the caller will have to promise.

Comment on lines 135 to 156
let size = size_of::<T>() + align_of::<T>();
let mut buff = vec![0u8; size * len];

let mut byte_buff = unsafe {
std::slice::from_raw_parts_mut(
buff.as_mut_ptr() as *mut u8,
buff.len()
)
};

self.read(addr, byte_buff.len(), byte_buff)?;

let mut arr = Vec::with_capacity(len);

while byte_buff.len() >= size_of::<T>() {
let (head, tail) = byte_buff.split_at_mut(size);
let s: T = unsafe { std::ptr::read(head.as_ptr() as *const T) };
arr.push(s);
byte_buff = tail;
}

Ok(arr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same alignment and heap allocation issue here. Could be replaced with this:

let mut buff: Vec<_> = std::iter::repeat_with(std::mem::MaybeUninit::<T>::uninit)
    .take(len)
    .collect();

let ptr: *mut u8 = buff.as_mut_ptr().cast();
let slice = unsafe { std::slice::from_raw_parts_mut(ptr, size_of::<T>() * len) };

self.read(addr, slice.len(), slice)?;

let ptr: *mut T = buff.as_mut_ptr().cast();
let len = buff.len();
let cap = buff.capacity();

std::mem::forget(buff);

Ok(unsafe { Vec::from_raw_parts(ptr, len, cap) })

Again, miri could help ensuring there's nothing wrong with the unsafety and marking the whole method as unsafe might be in order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants