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

feat: mman NonNull #2000

Merged
merged 8 commits into from
Nov 13, 2023
Merged

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Feb 15, 2023

Updates functions in nix::sys::mman to use NonNull.

The case where the returned pointer is null is the error case, therefore in the Ok case we can return NonNull.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/sys/mman.rs Outdated
Ok(ret)
}

NonNull::new(ret).ok_or_else(Errno::last)
Copy link
Member

Choose a reason for hiding this comment

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

NonNull::new() will ONLY return a None if its argument is a null pointer, the error case of mmap, whose return value libc::MAP_FAILED ((void *) -1 = 0xffffffff) is not null:

use std::ptr::NonNull;

fn main() {
    let ptr = NonNull::new(libc::MAP_FAILED);
    println!("{:?}", ptr);
}
$ cargo r -q
Some(0xffffffffffffffff)

which means that the current impl will return Ok(NonNull(0xffffffffff)) even when the underlying mmap() failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the implementation considering this.

@SteveLauC
Copy link
Member

Given that mmap() won't return a null pointer at all, I am actually not sure if this change is needed, thoughts on this?

cc @asomers

@SteveLauC SteveLauC self-assigned this Oct 2, 2023
@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Oct 9, 2023

Given that mmap() won't return a null pointer at all, I am actually not sure if this change is needed, thoughts on this?

If it always returns a non-null pointer this seems even more reason to return NonNull.

src/sys/mman.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member

Given that mmap() won't return a null pointer at all, I am actually not sure if this change is needed, thoughts on this?

If it always returns a non-null pointer this seems even more reason to return NonNull.

I think both our points are correct here:

From the interface safety perspetive, mmap() won't return a NULL pointer, so wrapping it in a NonNull is useless. However, for pointers that are not NULL, wrapping it in a NonNull will definitely make the interface more Rusty.

So I am not against this now, but still ping @asomers, I would like to hear your thoughts

@Jan561
Copy link
Contributor

Jan561 commented Nov 10, 2023

Just adding my two cents here.

NonNull<T> differs from *mut T in two aspects:

  • The pointer is guaranteed to be not NULL (obviously). This allows the compiler to perform some optimizations.
  • NonNull<T> is covariant in T. This is usually desirable when dealing with unique pointers (i.e. we own T, e.g. Vec, Box) or when dealing with shared pointers (e.g. shared ownership Rc, Arc).

mmap allocates new virtual memory and returns a unique pointer to the allocation, which exactly fits the use case of representing ownership with NonNull. This alone makes this change worthwhile for me.

Additionally, this function is fallible and we are only one call away of turning the result into an Option by calling Result::ok, where the aforementioned optimizations apply.

The only reason why NonNull is so underused even in the standard library is that many functions have already been stabilized before NonNull was invented. The first (?) API in std that will use NonNull instead of *mut is the upcoming Allocator API, which coincidentally is also what mmap essentially is.

@SteveLauC
Copy link
Member

Just adding my two cents here.

NonNull<T> differs from *mut T in two aspects:

  • The pointer is guaranteed to be not NULL (obviously). This allows the compiler to perform some optimizations.
  • NonNull<T> is covariant in T. This is usually desirable when dealing with unique pointers (i.e. we own T, e.g. Vec, Box) or when dealing with shared pointers (e.g. shared ownership Rc, Arc).

mmap allocates new virtual memory and returns a unique pointer to the allocation, which exactly fits the use case of representing ownership with NonNull. This alone makes this change worthwhile for me.

Additionally, this function is fallible and we are only one call away of turning the result into an Option by calling Result::ok, where the aforementioned optimizations apply.

The only reason why NonNull is so underused even in the standard library is that many functions have already been stabilized before NonNull was invented. The first (?) API in std that will use NonNull instead of *mut is the upcoming Allocator API, which coincidentally is also what mmap essentially is.

Makes sense to me, if we are gonna merge this change, should we also update the munmap(2) interface to something like?

pub unsafe fn munmap(addr: NonNull<c_void>, len: size_t) -> Result<()>

@Jan561
Copy link
Contributor

Jan561 commented Nov 12, 2023

Yes, good point. For symmetry reasons, I believe all other functions in mman should implement this change, too.

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 12, 2023

Yes, good point. For symmetry reasons, I believe all other functions in mman should implement this change, too.

I've updated the PR with all the other changes.

I think a follow-up PR could update the functions to take generic NonNull<[T]> instead of taking NonNull<c_void> and size_t.

@JonathanWoollett-Light JonathanWoollett-Light changed the title feat: mmap NonNull feat: mman NonNull Nov 12, 2023
@Jan561
Copy link
Contributor

Jan561 commented Nov 12, 2023

Yes, good point. For symmetry reasons, I believe all other functions in mman should implement this change, too.

I've updated the PR with all the other changes.

I think a follow-up PR could update the functions to take generic NonNull<[T]> instead of taking NonNull<c_void> and size_t.

I agree, this is a way nicer abstraction, but since this function allocates in pages and doesn't care about the layout of T, an idiomatic mmap should probably just return a page-aligned NonNull<[u8]> and the other functions taking the allocation as an argument should use the page-aligned starting pointer NonNull<u8>, similar to the std allocator trait.

The metadata of the slice pointer would then be the length rounded up to the next multiple of the page size.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

I was on the fence about this, because NonNull is less ergonomic than a plain pointer. But symmetry with Allocator is a good enough reason to do this, I think. I just noted one minor problem in some of the tests.

@@ -43,7 +47,7 @@ fn test_mremap_grow() {
let slice: &mut [u8] = unsafe {
#[cfg(target_os = "linux")]
let mem = mremap(
slice.as_mut_ptr().cast(),
NonNull::from(&slice[..]).cast(),
Copy link
Member

Choose a reason for hiding this comment

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

Here you're mutating through an immutable pointer. That's not ok. You should do Slice::as_mut_ptr or &mut slice instead. It happens a few places below, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to &mut slice[..]

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we can get rid of that #![allow(clippy::redundant_slicing)] if we just use the slice here, slice itself is of type &mut [u8], the constructed NonNull will be NonNull<[u8]>, then we cast() it to NonNull<c_void>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this, it fails in testing.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, sorry, &mut [u8] is not Copy, manually reborrowing here should do the trick

}
else {
Ok(unsafe { NonNull::new_unchecked(ret) })
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is unsafe, do you think that we could add a comment stating it is safe, maybe something like:

mmap() will never return a NULL pointer, it is safe to use NonNull::new_unchecked() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

@@ -468,7 +467,7 @@ pub unsafe fn mmap_anonymous(
if ret == libc::MAP_FAILED {
Err(Errno::last())
} else {
Ok(ret)
Ok(unsafe { NonNull::new_unchecked(ret) })
Copy link
Member

Choose a reason for hiding this comment

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

Also this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

new_size,
flags.bits(),
);

if ret == libc::MAP_FAILED {
Err(Errno::last())
} else {
Ok(ret)
Ok(unsafe { NonNull::new_unchecked(ret) })
Copy link
Member

Choose a reason for hiding this comment

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

And this one: mremap() will never return a NULL pointer, it is safe to use NonNull::new_unchecked() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

src/sys/mman.rs Outdated
Comment on lines 521 to 522
pub unsafe fn munmap(mut addr: NonNull<c_void>, len: size_t) -> Result<()> {
Errno::result(libc::munmap(addr.as_mut(), len)).map(drop)
Copy link
Member

Choose a reason for hiding this comment

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

Why we are using NonNull.as_mut() for munmap()? Other functions are all using NonNull.as_ptr()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to NonNull::as_ptr

@SteveLauC SteveLauC added this pull request to the merge queue Nov 13, 2023
Merged via the queue into nix-rust:master with commit 1ea59c7 Nov 13, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants