-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
src/memory/process.rs
Outdated
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) |
There was a problem hiding this comment.
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.
src/memory/process.rs
Outdated
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) |
There was a problem hiding this comment.
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.
Pretty sure this could be optimized even more
Draft until a legitimate use case is present