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

Eliminate some simple unsafe operations #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Mar 6, 2023

Replace pointer methods with slice copies and a transmute with into

@Yatekii
Copy link
Member

Yatekii commented Mar 6, 2023

Did you test this on target? :)

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 6, 2023

This week I have no target with me :(

I did do a unit tests https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=80b5d3b9bc4063e43f01b4adf4269707 but testing on hardware would be better of course

@mvirkkunen
Copy link
Contributor

I think the ptr::copy_nonoverlapping was meant to try to persuade the compiler to never optimize the writes in any way, but I guess the real way to do that would be to use ptr::write_volatile for that (too bad we still don't have volatile_copy_nonoverlapping_memory)

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 6, 2023

Hm, does the fence not provide sufficient guarantees there?

A volatile memcpy would work (with the bonus of zip being limited to the shorter of the two buffer lengths):

const ID_VAL: &[u8] = b"SEGGER RTT";
debug_assert!(ID_VAL.len() <= id.len());
for (src, dest) in ID_VAL.iter().zip(id.iter_mut()) {
    unsafe { ptr::write_volatile(dest, *src) };
}

copy_from_slice is just a wrapper around copy_nonoverlapping fwiw

https://github.com/rust-lang/rust/blob/f63ccaf25f74151a5d8ce057904cd944074b01d2/library/core/src/slice/mod.rs#L3354-L3356

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.

3 participants