rust-lang / libs-team

The home of the library team
Apache License 2.0
110 stars 18 forks source link

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

Open procedural opened 4 months ago

procedural commented 4 months ago

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.
}
scottmcm commented 4 months ago

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

procedural commented 4 months ago

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

ajwock commented 4 months ago

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.

dead-claudia commented 4 months ago

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%0A%7D%0A'),l:'5',n:'0',o:'Rust+source+%231',t:'0')),k:50,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:r1760,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:rust,libs:!(),options:'-C+opt-level%3D1',overrides:!((name:edition,value:'2021')),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+rustc+1.76.0+(Editor+%231)',t:'0')),k:50,l:'4',n:'0',o:'',s:0,t:'0')),l:'2',n:'0',o:'',t:'0')),version:4).

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.

procedural commented 4 months ago

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)

Amanieu commented 4 months ago

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.

kennytm commented 4 months ago

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)

Amanieu commented 4 months ago

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.

scottmcm commented 4 months ago

@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.

m-ou-se commented 4 months ago

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 commented 4 months ago

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.

cuviper commented 4 months ago

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.

kennytm commented 4 months ago

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:

cuviper commented 4 months ago

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.

procedural commented 4 months ago

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

scottmcm commented 2 months ago

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.