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

Extract SHA-1’s cast with as into a dedicated function. #159

Closed
wants to merge 1 commit into from
+16 −3
Closed

Extract SHA-1’s cast with as into a dedicated function. #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions src/digest/sha1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ type W32 = Wrapping<u32>;
pub unsafe extern fn block_data_order(state: &mut [u64; MAX_CHAINING_LEN / 8],
data: *const u8,
num: c::size_t) {
let data = data as *const [u8; BLOCK_LEN];
let blocks = core::slice::from_raw_parts(data, num);
block_data_order_safe(state, blocks)
block_data_order_safe(state, ptr_as_array_slice!(data, BLOCK_LEN, num))
}

fn block_data_order_safe(state: &mut [u64; MAX_CHAINING_LEN / 8],
Expand Down
15 changes: 15 additions & 0 deletions src/polyfill.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,18 @@ macro_rules! slice_as_array_ref_mut {
}
}
}

/// Returns a slice of arrays starting at the given pointer.
macro_rules! ptr_as_array_slice {
($ptr:expr, $block_len:expr, $block_num:expr) => {
Copy link
Owner

Choose a reason for hiding this comment

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

It's probably helpful to check out my latest comment in the issue to understand the design goals for this submodule. In particular, it doesn't make sense for a feature of this submodule to take a pointer as a parameter because pointers are unsafe and the goal of this submodule is to define a purely-not-unsafe API implemented on top of unsafe. And, also, the goal is to do so in a way where the feature could reasonably be expected to make it into the Rust language or stdlib.

I actually started trying to do this but I had to switch to working on non-ring stuff before I finished it. Here's my in-progress attempt:

/// Chunks `$slice` into `$len`-length parts, returning (`chunks`, `leftover`),
/// where `chunks` is a subslice of `$slice` with elements of type `[T; $len]`
/// and `leftover` is a subslice of leftover elements.
macro_rules! slice_as_array_ref_chunks {
    ($slice:expr, $chunk_len:expr) => {
        {
            #[allow(unsafe_code)]
            fn slice_as_array_ref_chunks<'a, T>(slice: &'a [T])
                    -> (&'a [[T; $chunk_len]], &'a [T]) {
                let (to_chunk, leftover) = match slice.len() % $chunk_len {
                    n => slice.split_at(slice.len() - ($chunk_len - n))
                };
                let to_chunk = unsafe {
                    to_chunk.as_ptr() as *const [T; $chunk_len]
                };
                let chunks =
                    core::slice::from_raw_parts(to_chunk,
                                                to_chunk.len() / $chunk_len);
                (chunks, leftover)
            }
            slice_as_array_ref_chunks($slice)
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as sha1::block_data_order takes data: *const u8, num: c::size_t, calling this new API would require an unsafe slice::from_raw_parts call:

let data = core::slice::from_raw_parts(data, num * BLOCK_LEN);
let (blocks, rest) = slice_as_array_ref_chunks(data, BLOCK_LEN);
debug_assert!(rest.is_empty());

do so in a way where the feature could reasonably be expected to make it into the Rust language or stdlib.

In my opinion this would be very tough sell for slice_as_array_ref_chunks. It’s a very narrow use case. Who knows how many slightly different functions like this would be needed to cover every use of as ever?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry to take so long to get back to you.

The purpose of the array_ref stuff is to basically work around the lack of generics over integer values. I don't expect the Rust lang/lib teams to ever implement slice_as_array_ref_chunks but I do expect people can agree that it is useful to have some kind of functionality like that.

Anyway, one goal of the polyfill module to to expose only an interface that is "safe" (doesn't require unsafe), so we should try to find some solution that achieves that goal. I am definitely open to alternatives to what I suggested above.

{
unsafe fn ptr_as_array_slice<'a, T>(ptr: *const T,
block_num: usize)
-> &'a [[T; $block_len]] {
let ptr = ptr as *const [T; $block_len];
core::slice::from_raw_parts(ptr, block_num)
}
ptr_as_array_slice($ptr, $block_num)
}
}
}