Remove most unsafe code
#162
Conversation
The only remaining `unsafe` code is used to call SIMD intrinsics.
| @@ -125,10 +123,9 @@ pub(crate) fn aesenc(value: u128, xor: u128) -> u128 { | |||
| use core::arch::aarch64::*; | |||
| #[cfg(target_arch = "arm")] | |||
| use core::arch::arm::*; | |||
| use core::mem::transmute; | |||
There was a problem hiding this comment.
Maybe #170 is caused by this?
There was a problem hiding this comment.
Looks like it. A fix has been uploaded: #171
There was a problem hiding this comment.
That we accidentally left one core::mem::transmute call in demonstrates to me that the name of zercopy::transmute! makes its use error-prone unless the transmute! call is moved out of the unsafe block (like in #171), as otherwise it is too easy to call mem::transmute instead, accidentally.
There was a problem hiding this comment.
I definitely support the goal to reduce unsafe usage. And I think zerocopy is one of the most carefully-written and best-engineered crates in the ecosystem. But, by its nature it operates at the very edge of the memory safety boundary. I do think it is worth trying a dependency-free approach to the same improvements. I suggest some concrete changes along these lines above (below?). I hope soon the portable SIMD project will make most of this moot.
| @@ -7,13 +7,13 @@ macro_rules! convert { | |||
| impl Convert<$b> for $a { | |||
| #[inline(always)] | |||
| fn convert(self) -> $b { | |||
| unsafe { core::mem::transmute::<$a, $b>(self) } | |||
| zerocopy::transmute!(self) | |||
There was a problem hiding this comment.
I looked at how convert! is used and I believe that all uses of it could be replaced with relatively straightforward use of u{64,32,16}::{from,to}_ne_bytes and array pattern matching, with 100% safe Rust code, without performance costs, and without a significant net LoC increase. IMO this is better than taking a zerocopy dependency (this is what I did in ring).
| } | ||
| } | ||
| impl Convert<$a> for $b { | ||
| #[inline(always)] | ||
| fn convert(self) -> $a { | ||
| unsafe { core::mem::transmute::<$b, $a>(self) } | ||
| zerocopy::transmute!(self) |
| middle.to_vec() | ||
| }; | ||
| use zerocopy::AsBytes; | ||
| let array = combination.as_slice().as_bytes().to_vec(); |
There was a problem hiding this comment.
This is test-only code, and clearly so, so IMO it would have been fine to leave it as it was. Or, alternatively, zerocopy as a dev-dependency would make sense.
| @@ -55,8 +56,7 @@ pub(crate) fn shuffle(a: u128) -> u128 { | |||
| use core::arch::x86::*; | |||
| #[cfg(target_arch = "x86_64")] | |||
| use core::arch::x86_64::*; | |||
| use core::mem::transmute; | |||
| unsafe { transmute(_mm_shuffle_epi8(transmute(a), transmute(SHUFFLE_MASK))) } | |||
| unsafe { transmute!(_mm_shuffle_epi8(transmute!(a), transmute!(SHUFFLE_MASK))) } | |||
There was a problem hiding this comment.
This didn't reduce the scope of unsafe like #171.
IDK if you run into performance issues doing so, but I would investigate doing something like:
#[cfg(target_arch = "x86_64")]
type Vector = __m128;
#[inline(always)]
const fn scalar(x: Vector) -> u128 {
unsafe { transmute(x) }
}
#[inline(always)]
const fn vector(x: u128) -> Vector {
unsafe { transmute(x) }
}const SHUFFLE_MASK: Vector = vector(SHUFFLE_MASK);
let a = vector(a);
scalar(unsafe { _mm_shuffle_epi8(a, SHUFFLE_MASK) })IMO, this meets the goal of clarifying that the unsafe conversions are safe with minimal work, without a dependency.
| @@ -125,10 +123,9 @@ pub(crate) fn aesenc(value: u128, xor: u128) -> u128 { | |||
| use core::arch::aarch64::*; | |||
| #[cfg(target_arch = "arm")] | |||
| use core::arch::arm::*; | |||
| use core::mem::transmute; | |||
There was a problem hiding this comment.
That we accidentally left one core::mem::transmute call in demonstrates to me that the name of zercopy::transmute! makes its use error-prone unless the transmute! call is moved out of the unsafe block (like in #171), as otherwise it is too easy to call mem::transmute instead, accidentally.
The only remaining
unsafecode is used to call SIMD intrinsics.