libs-team icon indicating copy to clipboard operation
libs-team copied to clipboard

std::slice::from_raw_parts alternative that would accept a null pointer if len == 0 by returning an empty slice

Open procedural opened this issue 2 years ago • 25 comments

Proposal

Problem statement

Currently, it's not allowed to pass a NULL pointer to std::slice::from_raw_parts, but

I have a pointer that comes from a C function that can be either:

A) A non-NULL with a len > 0 B) A NULL with a len == 0

Solution sketch

It would be neat if it was possible to just write something like:

let values = unsafe { std::slice::from_raw_parts_or_empty(c_ptr_to_values, len) }; // Still panic if c_ptr_to_values == NULL and len > 0.
for value in values {
   // This can be skipped if values is an empty slice when c_ptr_to_values == NULL and len == 0.
   // Or values can be iterated when c_ptr_to_values != NULL and len > 0.
}

procedural avatar Mar 07 '24 10:03 procedural

Pondering: is there a reasonable way we could have a takes-NonNull version of from_raw_parts (and the _mut one)? If so, that one could be the one without the check for null -- after all, there's a type proof it's not null -- and we could change the existing method to always check for null-and-zero-length, returning <&[_]>::default() in those cases.

I don't know if that's a good idea, though. I don't know how to weight the tradeoff between fixing any broken unsafe code that's trying to do this today vs the potential cost of either the extra extraneous check or people migrating.

Actually, maybe it's completely fine, since if a parameter is ptr noundef nonnull, LLVM seems perfectly capable of removing the extra null check: https://rust.godbolt.org/z/PbY1recY3

scottmcm avatar Mar 07 '24 10:03 scottmcm

Just to be clear why I'm asking for this, I'm currently wrapping REDGPU C API in Rust and it's a valid behavior to expect either a non-NULL array of GPUs available on the system or a NULL pointer to GPUs in case there are no capable GPUs available:

https://github.com/redgpu/redgpu/blob/master/RedGpuSDK/redgpu.h#L475-L476

procedural avatar Mar 07 '24 10:03 procedural

Perhaps a more appropriate API for this situation would be NonNull<T>::nonnull_or_dangling(ptr: *mut T) ->NonNull<T> (probably not the best name) where you either get a validated non-null pointer or an aligned dangling pointer.

ajwock avatar Mar 10 '24 00:03 ajwock

Note: you can do NonNull::new(ptr).unwrap_or(NonNull::dangling()) to do it in not much more code than a hypothetical NonNull::new_or_dangling(ptr), and with optimizations enabled, that should result in a simple conditional move or select being generated on about any platform out there.

A std::ptr::dangling::<T>() -> *const T and std::ptr::dangling_mut::<T>() -> *mut T analogous to std::ptr::null()/std::ptr::null_mut() that essentially returns what NonNull::dangling().get() returns today would be very nice for pointer-heavy stuff.

dead-claudia avatar Mar 10 '24 03:03 dead-claudia

I'm temporarily using this:

pub unsafe fn slice_from_raw_parts_or_empty<'a, T>(data: *const T, len: usize) -> &'a [T] {
  if data.is_null() && len == 0 { &[] } else { std::slice::from_raw_parts(data, len) }
}

pub unsafe fn slice_from_raw_parts_mut_or_empty<'a, T>(data: *mut T, len: usize) -> &'a mut [T] {
  if data.is_null() && len == 0 { &mut [] } else { std::slice::from_raw_parts_mut(data, len) }
}

(thanks to @cuviper for pointing to an issue with earlier versions of these functions)

procedural avatar Mar 10 '24 08:03 procedural

I'm in favor of adding this for the sole reason that its absence will encourage people to either implement their own version or use an incorrect implementation based on from_raw_parts.

Amanieu avatar Mar 12 '24 17:03 Amanieu

why not make std::slice::from_raw_parts (not std::ptr::from_raw_parts) automatically check data == null() && len == 0 and return (dangling(), 0) then

and should this ACP affect std::ptr::slice_from_raw_parts? (create a std::ptr::slice_from_raw_parts_or_empty function)

kennytm avatar Mar 13 '24 10:03 kennytm

I don't think we should be touching the existing functions simply because of how widely used they are in the ecosystem. Even adding a simple check there could have a large performance impact.

Amanieu avatar Mar 15 '24 09:03 Amanieu

@Amanieu I think it would depend on quantifying the impact. If it usually ends up optimizing out, as it does in the godbolt in https://github.com/rust-lang/libs-team/issues/350#issuecomment-1983189027, then it might be feasible -- especially if it was done a few releases after we made a version available taking NonNull that doesn't have the check that people could use if they really really can't have the check.

scottmcm avatar Mar 17 '24 20:03 scottmcm

If the impact is low enough, I very much prefer making std::slice::from_raw_parts work when the data pointer is null, to remove that footgun. (We could add something like a slice::from_raw_parts_nonnull taking a NonNull to get the current behaviour, if necessary. Then the lack of null check is explicit as a call to NonNull::new_unchecked().)

m-ou-se avatar Mar 19 '24 14:03 m-ou-se

This briefly came up in the libs-api meeting. We thought it'd probably be useful to do a perf run to see the impact a check in std::slice::from_raw_parts would have on rustc.

m-ou-se avatar Mar 19 '24 15:03 m-ou-se

I feel really wary of having slice::from_raw_parts that would not round-trip with as_mut_ptr, but it seems less concerning with the _or_empty version to indicate that it's not just the raw values.

cuviper avatar Mar 19 '24 15:03 cuviper

but currently std::slice::from_raw_parts(null(), 0) (not std::ptr::from_raw_parts) is undefined behavior according to the doc (&[T] must be non-null), so it not round-tripping with .as_mut_ptr() for the null case is certainly allowed :upside_down_face:

kennytm avatar Mar 19 '24 16:03 kennytm

so it not round-tripping with .as_mut_ptr() for the null case is certainly allowed 🙃

I suppose -- but at the very least, it should not be solely based on len == 0 like https://github.com/rust-lang/libs-team/issues/350#issuecomment-1987137744. If given a non-null pointer, regardless of length, that should be preserved.

cuviper avatar Mar 19 '24 17:03 cuviper

@cuviper Nice catch, indeed I didn't think of that, I updated the code...

procedural avatar Mar 19 '24 17:03 procedural

Thinking about this some more, NonNull::slice_from_raw_parts(p, n).as_ref() is a stable version of slice::from_raw_parts that definitely doesn't have a null check since we have the type proof from p: NonNull<T>. So there's a way that's been stable for 5 releases already that the "definitely won't have a check" can be written.

So if the extra "replace null with dangling" check can almost always be optimized out, that seems plausibly fine since people could use the other thing in the cases where it's really perf-sensitive to not have the check.

scottmcm avatar May 03 '24 00:05 scottmcm