fix pointer aliasing an other issues in ItemSliceSend

[martinvonz]

No description provided.

[Byron]

Thanks a lot!

I think the main achievement here is to keep track of the lifetime of the input slice, which I will remember should there ever be another requirement fro unsafe in the future.

The switch away from a pointer to a slice seems more arbitrary unless one wants to remove bound checks. By now, since the logic seems to work, I think it's fine to do that even though in future, I'd still go with pointer slices initially as they'd provide bound-checks by default. But please let me know if there is anything I am missing. After all, unsafe is a bit like a language on its own and I am not very well versed in it.

Reading the above, and with my current probably incomplete knowledge, it can boil down to a style question: Do you want data: *mut [T] possibly paired with &mut (*data).get_unchecked(index) or rather data: *mut T which hides that we are looking at a slice.

[Byron]

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.

[martinvonz]

The switch away from a pointer to a slice seems more arbitrary unless one wants to remove bound checks.

Do you mean in the constructor in the first commit? I just felt like slices are preferred over pointers in general. I would personally only use a pointer if I really had to. I don't know if I've ever used pointers outside of FFI in Rust.

Do you want data: *mut [T] possibly paired with &mut (*data).get_unchecked(index) or rather data: *mut T which hides that we are looking at a slice.

That's the same reasoning as in #1115: Dereferencing that *mut [T] concurrently (e.g. in different threads) is undefined behavior. Using *mut T and pointer::add() is not. That's my understanding of the issue anyway.

[Byron]

Do you mean in the constructor in the first commit? I just felt like slices are preferred over pointers in general. I would personally only use a pointer if I really had to. I don't know if I've ever used pointers outside of FFI in Rust.

I meant the internal representation, having a constructor is definitely nicer as well. If using *mut [T] from multiple threads generally is undefined behaviour, it makes sense to not do that of course. However, from my limited knowledge, all reasons for that that I have in mind (and do actively remember) are… actually it's only one reason being the usage of uninitialized memory to the point where one can't even look at it. But dereferencing a mutable slice should be fine as long as memory isn't aliased.
But I write that here just to share it, not to make any claims of correctness. After all, unsafe is mostly forbidden territory for me exactly for that reason - I never feel like I know enough and with certainty 😅.

[martinvonz]

I also don't know much about these things (that's why we have review of unsafe code by people who know better) :) I suspect the old version was perfectly safe with the current rustc, but the UB could mean that some future rustc could break it somehow. @cramertj provided me this nice illustration in our internal review: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3e20c65f7c47979cf57e7effd2e1195a. Click Tools->Miri to see the error.

[Byron]

Thanks so much for sharing!
I'd love to say I understand why the first version doesn't work and it's a bit of a hunch that it might be related to the new borrow checker not understanding something that it should understand (even though it's too trivial, too). Removing one assignment for example also works with version 1 (that fails with two assignments), which should mean that it's generally nothing that Miri would complain about.

The borrow-checker not being able to validate borrowing rules seems separate.

Oh well, if you ever stumble over an answer I can understand, please do share it with me :).

[martinvonz]

I think it's that *args_slice_ptr results in a &mut [u8], mutably borrowing the entire slice. So I think it's like writing this:

let x = &mut args[0];
let y = &mut args[1];

That's of course not allowed (I mean, it could be if Rust understood that you only borrowed a single element, but that's not how Rust currently works).

By switching to a pointer, I think we're basically telling the compiler to just assume that the mutable pointers to individual elements cannot be aliased (which is a valid assumption, barring any bugs).

[Byron]

Ah, I understand now, thanks for bearing with me 😅. Using a slice like was done here before is then also fighting the borrow-checker more than necessary, and who knows what that could cover up, I suppose.