rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.9k stars 12.68k forks source link

Tracking Issue for raw slice getters (slice_ptr_get) #74265

Open RalfJung opened 4 years ago

RalfJung commented 4 years ago

This is a tracking issue for indexing methods on raw slices: get_unchecked(_mut) and as_(mut_/non_null_)ptr on raw slices (mutable and const raw pointers and NonNull).

The feature gate for the issue is #![feature(slice_ptr_get)].

Public API

impl<T> *mut [T] {
    pub const fn as_mut_ptr(self) -> *mut T {}
    pub unsafe fn get_unchecked_mut<I>(self, index: I) -> *mut I::Output where I: SliceIndex<[T]>;
}

impl<T> *const [T] {
    pub const fn as_ptr(self) -> *const T {}
    pub unsafe fn get_unchecked<I>(self, index: I) -> *const I::Output where I: SliceIndex<[T]>;
}

impl<T> NonNull<[T]> {
    pub const fn as_non_null_ptr(self) -> NonNull<T> {}
    pub const fn as_mut_ptr(self) -> *mut T {}
    pub unsafe fn get_unchecked_mut<I>(self, index: I) -> NonNull<I::Output> where I: SliceIndex<[T]>;
}

History / Steps

Open questions

RalfJung commented 4 years ago

https://github.com/rust-lang/rust/issues/74679 is a potential blocker for this.

RalfJung commented 4 years ago

With https://github.com/rust-lang/rust/pull/75152, the allocator trait started making use of this.

TimDiekmann commented 4 years ago

Especially with AllocRef using it, it would come in handy if we could also add

impl<T> NonNull<[T]> {
    #[inline]
    fn as_mut_ptr(self) -> *mut T {
        self.as_non_null_ptr().as_ptr()
    }
}

If we want this I'd make a PR.

RalfJung commented 4 years ago

Seems reasonable to me -- @Amanieu what do you think?

TimDiekmann commented 4 years ago

An alternative name would be as_raw_ptr to avoid confusion with as_ptr, which already returns *mut T. This would also apply to *const [T] and *mut [T].

Amanieu commented 4 years ago

I don't particularly care for the name bikeshed but I agree that this method would be useful.

TimDiekmann commented 4 years ago

This issue now also tracks NonNull::<[T]>::as_mut_ptr(). I can't edit the OP as I'm not a collaborator of this repository.

At "Implementation history" this has to be added:

RalfJung commented 4 years ago

While using these methods for an upcoming PR, I noticed one thing: there is no const-to-mut coercion for raw pointers (at least not for slices, or maybe method lookup just is unable to use it), so one has to use get_unchecked_mut on *mut [T].

I wonder if it would make sense to just not have the mut in the name (but it would likely be inconsistent).

steffahn commented 2 years ago

There’s discussion here about adding general *const T -> *mut T and *mut T -> *const T helper methods (as a way to avoid as casts). The method name as_mut_ptr sounds like it does exactly that, but it's currently used here for *mut [T] -> *mut T. Reading the current NonNull documentation was also highly confusing; particularly when looking at the method list in the side-bar.

I would personally appreciate e.g. some mention to slice in the name of the as_[…]ptr family of methods; in any case, I just mainly wanted to note here (just so it won't be forgotten) that in case such general conversion methods are going to be wanted, there might be interest in a different meaning for the method name as_mut_ptr.

RalfJung commented 2 years ago

These methods are needed to work around the problem described in https://github.com/rust-lang/rust/issues/99437. Though interestingly, to preserve the full semantics of the original code but fix the aliasing problems, we would want bounds-checked getters on raw slices.

kornelski commented 2 years ago

@steffahn these methods could be named cast_mut() and cast_shared() (or is it const?), alluding to ptr::cast(). cast is the replacement for the as operator in pointers. In types like slice.as_ptr() and vec.as_ptr() this method changes the type, but not shared/mut status.

NonNull is weird tho. Maybe it could get a redundant as_mut_ptr() just to reduce the weirdness?

JonathanWoollett-Light commented 11 months ago

Has there been any progress? It appears all required work is done? This feature will be useful in adding safety munmap, madvise and other mman functions in https://crates.io/crates/nix specifically extension work to https://github.com/nix-rust/nix/pull/2000 as mentioned https://github.com/nix-rust/nix/pull/2000#issuecomment-1807147263.

Ralith commented 11 months ago

The API docs for get_unchecked and get_unchecked_mut say:

Calling this method [...] when self is not dereferenceable is undefined behavior

This is too strong, right? self is not dereferenceable when e.g. an exclusive reference to any of its elements exists, but it should be sound to use these methods to construct pointers to other elements, or even to an already-referenced element if the constructed pointer isn't dereferenced. Was the intention to instead specify that the pointer must address allocated memory?

RalfJung commented 11 months ago

"dereferenceable" generally means "points to memory that is inbounds of an allocation".

However, these functions take references, so it is indeed UB to call them when there are aliasing conflicts: get_unchecked acts like a read of the entire slice, and get_unchecked_mut acts like a write of the entire slice. These rules are not final, but until Rust has a precise aliasing model you should follow these conservative rules to ensure maximal chance of being compatible with the future memory model.

Ralith commented 11 months ago

"dereferenceable" generally means "points to memory that is inbounds of an allocation".

That's surprising; in Rust API documentation, of a pointer, I'd expect it to mean "it is legal to convert this pointer into a reference".

these functions take references

Are we talking about the same functions? <*const [T]>::get_unchecked takes a raw pointer by value, and passes it on to SliceIndex::get_unchecked:

https://github.com/rust-lang/rust/blob/0ea7ddcc35a2fcaa5da8a7dcfc118c9fb4a63b95/library/core/src/ptr/const_ptr.rs#L1693

which adds the index and returns the pointer:

https://github.com/rust-lang/rust/blob/0ea7ddcc35a2fcaa5da8a7dcfc118c9fb4a63b95/library/core/src/slice/index.rs#L238

I don't see a single reference being accepted, constructed or manipulated at any stage here.

RalfJung commented 11 months ago

Are we talking about the same functions?

Sorry, I thought you meant slice::get_unchecked. I didn't realize which issue this is in :joy:

That's surprising; in Rust API documentation, of a pointer, I'd expect it to mean "it is legal to convert this pointer into a reference".

No, that's not what it means. It's not "referenceable" after all, it's "dereferenceable", referring to the dereference operator *. Though the semantics of that got relaxed recently so dereferencing can be allowed on pointers that are not dereferenceable... maybe we need a new term.^^

RalfJung commented 11 months ago

I think this has been sitting around long enough, clearly a fix for https://github.com/rust-lang/rust/issues/73987 will not magically materialize. And even with https://github.com/rust-lang/rust/issues/73987 unresolved, as long as we don't let users write arbitrary-self methods on raw pointers, we can still change the autoref behavior later to use raw pointers when that is enough. So while these methods might have a footgun, they are still better than the status quo.

So I propose we move towards stabilizing the raw pointer part of this. (I am less sure about the NonNull part.)

@rust-lang/types are you aware of anything that would block stabilizing a function that uses a raw slice pointer self type? (AFAIK these would be the first stable functions that do that.)

lcnr commented 10 months ago

https://github.com/rust-lang/rust/issues/71146#issuecomment-1838908803 applies to this feature as well

the8472 commented 9 months ago

This happened to come up in a libs-api meeting. We want to note that stabilization of this feature will likely be blocked on the arbitrary_self_types feature (https://github.com/rust-lang/rfcs/pull/3519) since that will allow moving these methods to

impl [T] {
   fn as_mut_ptr(*const Self) -> *mut T 
}
RalfJung commented 9 months ago

Yeah I was about to ask, given @joshtriplett's reply here, what the implications are for this feature.

However, I think we should really offer some way to work with raw slice pointers. The current situation is pretty unfortunate and nothing has improved for many years. If we can't stabilize this as-is, does @rust-lang/libs-api have any alternatives we could pursue?

the8472 commented 9 months ago

Well, part of the issue is that similar methods have been proposed for array pointers. We'd either have to duplicate them or see what kind of deref-like coercions self types will provide.

RalfJung commented 9 months ago

or see what kind of deref-like coercions self types will provide

So far, Rust has largely avoided coercions on raw pointers. That's pretty orthogonal to arbitrary-self-types, so I don't expect https://github.com/rust-lang/rfcs/pull/3519 to change that. So another T-lang RFC would be needed to have such coercions on raw ptr methods I think.

Rua commented 4 months ago

Is there any interest in implementing a method that does perform bounds checking, while still operating on pointers rather than references?

RalfJung commented 4 months ago

That would make sense to me, yeah. I am not in t-libs-api though, so this should probably go through an ACP.

Rua commented 4 months ago

I ran into an issue with this. get_unchecked is implemented for pointers to slices, but not for pointers to fixed-size arrays. We probably want to implement it for arrays too?

yotamofek commented 4 months ago

I ran into an issue with this. get_unchecked is implemented for pointers to slices, but not for pointers to fixed-size arrays. We probably want to implement it for arrays too?

Would love to have get_unchecked(_mut) on pointers to arrays, they could maybe live under the array_ptr_get feature. Two notes: 1) you could go through the as_(mut_)slice methods from that feature; 2) if implemented, stabilization of methods on pointers to arrays will likely be blocked on the arbitrary_self_type features, as discussed in the ACP.