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.

Sign up for GitHub

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

Jump to bottom

Remove most unsafe code #162

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Remove most unsafe code #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31ac289
Remove most `unsafe` code
joshlf Aug 25, 2023
bd9e635
Merge branch 'master' into remove-unsafe
tkaitchuck Oct 20, 2023
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
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ serde = { version = "1.0.117", optional = true }
cfg-if = "1.0"
atomic-polyfill = { version="1.0.1", optional=true}
getrandom = { version = "0.2.7", optional = true }
zerocopy = { version = "0.7.0", default-features = false, features = ["simd"] }

[target.'cfg(not(all(target_arch = "arm", target_os = "none")))'.dependencies]
once_cell = { version = "1.13.1", default-features = false, features = ["unstable", "alloc"] }
Expand Down
7 changes: 3 additions & 4 deletions src/convert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

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

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)
Copy link

Choose a reason for hiding this comment

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

Ditto.

}
}
};
Expand Down Expand Up @@ -65,8 +65,7 @@ macro_rules! as_array {
{
#[inline(always)]
fn as_array<T>(slice: &[T]) -> &[T; $len] {
assert_eq!(slice.len(), $len);
unsafe { &*(slice.as_ptr() as *const [_; $len]) }
core::convert::TryFrom::try_from(slice).unwrap()
}
as_array($input)
}
Expand Down
8 changes: 2 additions & 6 deletions src/hash_quality_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,8 @@ fn test_no_full_collisions<T: Hasher>(gen_hash: impl Fn() -> T) {
gen_combinations(&options, 7, Vec::new(), &mut combinations);
let mut map: HashMap<u64, Vec<u8>> = HashMap::new();
for combination in combinations {
let array = unsafe {
let (begin, middle, end) = combination.align_to::<u8>();
assert_eq!(0, begin.len());
assert_eq!(0, end.len());
middle.to_vec()
};
use zerocopy::AsBytes;
let array = combination.as_slice().as_bytes().to_vec();
Copy link

Choose a reason for hiding this comment

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

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.

let mut hasher = gen_hash();
hasher.write(&array);
let hash = hasher.finish();
Expand Down
29 changes: 12 additions & 17 deletions src/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::convert::*;
use zerocopy::transmute;

Check warning on line 2 in src/operations.rs

View workflow job for this annotation

GitHub Actions / thumbv6m 

unused import: `zerocopy::transmute`

Check warning on line 2 in src/operations.rs

View workflow job for this annotation

GitHub Actions / Linux ARMv7 

unused import: `zerocopy::transmute`

Check warning on line 2 in src/operations.rs

View workflow job for this annotation

GitHub Actions / wasm 

unused import: `zerocopy::transmute`

///This constant comes from Kunth's prng (Empirically it works better than those from splitmix32).
pub(crate) const MULTIPLE: u64 = 6364136223846793005;
Expand Down Expand Up @@ -55,8 +56,7 @@
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))) }
Copy link

Choose a reason for hiding this comment

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

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.

}
#[cfg(not(all(target_feature = "ssse3", not(miri))))]
{
Expand All @@ -81,13 +81,12 @@
#[cfg(all(any(target_arch = "x86", target_arch = "x86_64"), target_feature = "sse2", not(miri)))]
#[inline(always)]
pub(crate) fn add_by_64s(a: [u64; 2], b: [u64; 2]) -> [u64; 2] {
use core::mem::transmute;
unsafe {
#[cfg(target_arch = "x86")]
use core::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use core::arch::x86_64::*;
transmute(_mm_add_epi64(transmute(a), transmute(b)))
transmute!(_mm_add_epi64(transmute!(a), transmute!(b)))
}
}

Expand All @@ -105,10 +104,9 @@
use core::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use core::arch::x86_64::*;
use core::mem::transmute;
unsafe {
let value = transmute(value);
transmute(_mm_aesenc_si128(value, transmute(xor)))
let value = transmute!(value);
transmute!(_mm_aesenc_si128(value, transmute!(xor)))
}
}

Expand All @@ -125,10 +123,9 @@
use core::arch::aarch64::*;
#[cfg(target_arch = "arm")]
use core::arch::arm::*;
use core::mem::transmute;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe #170 is caused by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it. A fix has been uploaded: #171

Copy link

Choose a reason for hiding this comment

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

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.

unsafe {
let value = transmute(value);
xor ^ transmute::<_, u128>(vaesmcq_u8(vaeseq_u8(value, transmute(0u128))))
let value = transmute!(value);
xor ^ transmute::<_, u128>(vaesmcq_u8(vaeseq_u8(value, transmute!(0u128))))
}
}

Expand All @@ -140,10 +137,9 @@
use core::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use core::arch::x86_64::*;
use core::mem::transmute;
unsafe {
let value = transmute(value);
transmute(_mm_aesdec_si128(value, transmute(xor)))
let value = transmute!(value);
transmute!(_mm_aesdec_si128(value, transmute!(xor)))
}
}

Expand All @@ -160,10 +156,9 @@
use core::arch::aarch64::*;
#[cfg(target_arch = "arm")]
use core::arch::arm::*;
use core::mem::transmute;
unsafe {
let value = transmute(value);
xor ^ transmute::<_, u128>(vaesimcq_u8(vaesdq_u8(value, transmute(0u128))))
let value = transmute!(value);
xor ^ transmute::<_, u128>(vaesimcq_u8(vaesdq_u8(value, transmute!(0u128))))
}
}

Expand Down Expand Up @@ -207,7 +202,7 @@
// #[cfg(target_arch = "x86_64")]
// use core::arch::x86_64::*;
// MASK.with(|mask| {
// unsafe { transmute(_mm_shuffle_epi8(transmute(a), transmute(mask.get()))) }
// unsafe { transmute!(_mm_shuffle_epi8(transmute!(a), transmute!(mask.get()))) }
// })
// }
//
Expand Down
Loading