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

fix pointer aliasing an other issues in ItemSliceSend #1116

Merged
merged 4 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions gix-pack/src/cache/delta/traverse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,14 @@ where
};
size_progress.init(None, progress::bytes());
let size_counter = size_progress.counter();
let child_items = self.child_items.as_mut_slice();
let object_progress = OwnShared::new(Mutable::new(object_progress));

let start = std::time::Instant::now();
in_parallel_with_slice(
&mut self.root_items,
thread_limit,
{
let child_items = ItemSliceSend(std::ptr::slice_from_raw_parts_mut(
child_items.as_mut_ptr(),
child_items.len(),
));
let child_items = ItemSliceSend::new(&mut self.child_items);
{
let object_progress = object_progress.clone();
move |thread_index| {
Expand Down
6 changes: 3 additions & 3 deletions gix-pack/src/cache/delta/traverse/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ use crate::{
data::EntryRange,
};

pub(crate) struct State<F, MBFN, T: Send> {
pub(crate) struct State<'items, F, MBFN, T: Send> {
pub delta_bytes: Vec<u8>,
pub fully_resolved_delta_bytes: Vec<u8>,
pub progress: Box<dyn Progress>,
pub resolve: F,
pub modify_base: MBFN,
pub child_items: ItemSliceSend<Item<T>>,
pub child_items: ItemSliceSend<'items, Item<T>>,
}

#[allow(clippy::too_many_arguments)]
Expand All @@ -38,7 +38,7 @@ pub(crate) fn deltas<T, F, MBFN, E, R>(
resolve,
modify_base,
child_items,
}: &mut State<F, MBFN, T>,
}: &mut State<'_, F, MBFN, T>,
resolve_data: &R,
hash_len: usize,
threads_left: &AtomicIsize,
Expand Down
37 changes: 29 additions & 8 deletions gix-pack/src/cache/delta/traverse/util.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,49 @@
use std::marker::PhantomData;

use crate::cache::delta::Item;

pub struct ItemSliceSend<T>(pub *mut [T])
pub(crate) struct ItemSliceSend<'a, T>
where
T: Send,
{
items: *mut T,
phantom: PhantomData<&'a T>,
Copy link
Member

Choose a reason for hiding this comment

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

Nice! The phantom has the sole purpose of maintaining the lifetime of the input slice, even though internally it's reduced to a pointer.

This also means the bounds-check isn't happening anymore upon access, but then again, it's also not needed anymore as it's known that all indices fit within the slice.

}

impl<'a, T> ItemSliceSend<'a, T>
where
T: Send;
T: Send,
{
pub fn new(items: &'a mut [T]) -> Self {
ItemSliceSend {
items: items.as_mut_ptr(),
phantom: PhantomData,
}
}
}

/// SAFETY: This would be unsafe if this would ever be abused, but it's used internally and only in a way that assure that the pointers
/// don't violate aliasing rules.
impl<T> Clone for ItemSliceSend<T>
impl<T> Clone for ItemSliceSend<'_, T>
where
T: Send,
{
fn clone(&self) -> Self {
ItemSliceSend(self.0)
ItemSliceSend {
items: self.items,
phantom: self.phantom,
}
}
}

// SAFETY: T is `Send`, and we only ever access one T at a time. And, ptrs need that assurance, I wonder if it's always right.
#[allow(unsafe_code)]
unsafe impl<T> Send for ItemSliceSend<T> where T: Send {}
unsafe impl<T> Send for ItemSliceSend<'_, T> where T: Send {}

/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
pub struct Node<'a, T: Send> {
pub(crate) struct Node<'a, T: Send> {
pub item: &'a mut Item<T>,
pub child_items: ItemSliceSend<Item<T>>,
pub child_items: ItemSliceSend<'a, Item<T>>,
}

impl<'a, T: Send> Node<'a, T> {
Expand Down Expand Up @@ -57,7 +78,7 @@ impl<'a, T: Send> Node<'a, T> {
// SAFETY: The resulting mutable pointer cannot be yielded by any other node.
#[allow(unsafe_code)]
Node {
item: &mut unsafe { &mut *children.0 }[index as usize],
item: unsafe { &mut *children.items.add(index as usize) },
child_items: children.clone(),
}
})
Expand Down
Loading