-
Notifications
You must be signed in to change notification settings - Fork 166
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
Add rustix::process::exit
to terminate processes
#1133
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if you could describe your usecase a little more. I had previously omitted this functionality from the public API because I didn't know of any use cases for it, outside of, saying writing your own libc. Which is fine, I'm just curious.
src/process/exit.rs
Outdated
/// [POSIX]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdlib.h.html | ||
/// [Linux]: https://www.man7.org/linux/man-pages/man2/exit.2.html | ||
#[inline] | ||
pub fn exit(status: i32) -> ! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think of naming this function exit_immediately
? And we can add #[doc(alias = "_Exit")]
for people searching for this using the C name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about immediate_exit
, which would align better with quick_exit
?
Generally speaking, any For the specific use-case, we are writing a The point of this library is to serve as a runtime for testing via symbolic execution, which means we really want to run below the "user-facing libc". I am currently trying to move away from this multiple-but-different libc approach and make our runtime (which is mostly written in Rust) be pure Rust, ideally using the x86_64-unknown-linux-none target. The OS APIs that we are currently using are mostly covered by Rustix, but sometimes we need to immediately exit without going through user code (the code under test is after all by definition untrustworthy). |
Turns out that I was not quite thorough enough, and that I still think that a more discoverable function (probably in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that I was not quite thorough enough, and that
rustix::runtime::exit_group
was exactly what I was looking for, I just could not find it without your comments.
Keep in mind though that rustix::runtime::*
are not stable APIs.
I still think that a more discoverable function (probably in
rustix::process
) would be good for more generalno_std
libraries, what do you think?
My understanding is that this is niche. Normal libraries don't usually exit, because exit would make a major assumption about the application using them. And a lot of no_std code doesn't know enough about the OS it's running on to even know if invoking linux syscalls is appropriate.
That said, I am open to adding this. I think the doc comment you wrote here is good, and I'm now just thinking about whether there are any other caveats we should add.
@@ -742,3 +742,14 @@ pub(crate) fn getgroups(buf: &mut [Gid]) -> io::Result<usize> { | |||
|
|||
unsafe { ret_usize(c::getgroups(len, buf.as_mut_ptr().cast()) as isize) } | |||
} | |||
|
|||
#[inline] | |||
pub(crate) fn _exit(status: i32) -> ! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identifiers with leading underscores have a special meaning in Rust, so I suggest we rename this internal function to immediate_exit
as well.
#[allow(unreachable_code)] | ||
{ | ||
unreachable!("_exit failed to exit the process") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave these four lines out. The _exit
function already has a return type of !
so Rust is already assuming it won't return.
#[allow(unreachable_code)] | ||
{ | ||
unreachable!("_exit failed to exit the process") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we can leave these four lines out because the noreturn
in the inline asm is already telling Rust that the inline asm won't return.
/// `EXIT_SUCCESS` for use with [`exit`]. | ||
/// | ||
/// [`exit`]: std::process::exit | ||
/// `EXIT_SUCCESS` for use with [`exit`] or [`std::process::exit`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is named immediate_exit
now. And it's a minor nit, but please put std::process::exit
first, as that's still the main function here.
/// `EXIT_FAILURE` for use with [`exit`]. | ||
/// | ||
/// [`exit`]: std::process::exit | ||
/// `EXIT_FAILURE` for use with [`exit`] or [`std::process::exit`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as for EXIT_SUCCESS
.
This PR adds
rustix::process::exit
to exit the current process. This is useful forno_std
code, which cannot callstd::process::exit
.The kernel backend uses the
exit_group
syscall, which is equivalent to the_exit
/_Exit
POSIX/C functions. For consistency, I also used the_exit
function for the C backend.There are two main ways in which this could be changed:
rustix::process::_exit
instead, which would differ from the usual Rust naming scheme.exit
in the C backend instead, which would callat_exit
functions only when the C backend is enabled.This PR is additive and should not require a semver bump.