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

exit: document interaction with C #136320

Merged
merged 2 commits into from
Mar 19, 2025
Merged

exit: document interaction with C #136320

merged 2 commits into from
Mar 19, 2025

Conversation

RalfJung
Copy link
Member

Cc #126600

@rustbot
Copy link
Collaborator

rustbot commented Jan 30, 2025

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 30, 2025
Copy link
Contributor

@tbu- tbu- left a comment

Choose a reason for hiding this comment

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

I think it would be nice to acknowledge the movement in POSIX and the C standard to define this as thread-safe in the future. We also do the same for setenv where we link to discussions about its unsafety.

@@ -2294,6 +2294,25 @@ impl Child {
/// considered undesirable. Note that returning from `main` also calls `exit`, so making `exit` an
/// unsafe operation is not an option.)
///
/// ## Safe interop with C code
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// ## Safe interop with C code
/// ## Safety

I think usually, this section is called "Safety".

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have any safe function with a "safety" section.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have one for the std::env::{set_var, remove_var} functions, even before they were marked as unsafe. I think this is a similar situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided the situation is not quite similar, since we won't make exit unsafe.

@RalfJung RalfJung force-pushed the exit branch 2 times, most recently from 048b18c to 8f3adf5 Compare January 31, 2025 16:09
@safinaskar
Copy link
Contributor

This text is absolutely unconvincing ( https://doc.rust-lang.org/nightly/std/process/fn.exit.html ):

Note that returning from main also calls exit, so making exit an unsafe operation is not an option

Rust's thread API (i. e. std::thread) doesn't allow creating thread such way that it falls to main. Same applies to pthread_create and even to Linux's clone glibc and musl wrapper. The only way (in Linux) to create thread such way that you can fall to main is raw Linux SYS_clone syscall, which is wildly unsafe operation (and also vfork) (and also you can probably fall to main using setjmp).

So, returning from main is totally safe operation, because it can ever be performed one time. You can return from main only once. Returning from main is safe even if you have C libraries, which create threads using pthread_create behind your back. So it is totally possible to mark process::exit as unsafe operation, while still having return from main as a safe operation.

I'm not saying that we should mark process::exit as unsafe (this may be a good idea through). I'm just saying that that text above is not valid

@RalfJung
Copy link
Member Author

RalfJung commented Feb 4, 2025

First of all, that text is pre-existing, not touched by this PR. So it's kind of off-topic in this PR. You should at least have made that clear in your comment; currently you are implying that you are commenting on something I wrote. (Maybe that's what you meant by adding the URL? It's not clear.)

Second, I don't follow your argument. Note that that text talks about the requirements imposed on atexit handlers. It doesn't even talk about exit itself not being thread safe. There's two options here:

  • The proof obligation for ensuring that atexit handlers are safe to calls falls on whomever invokes exit.
  • The proof obligation falls on whoever adds the atexit handlers.

The former would imply that exit has to be unsafe. However, the text argues (correctly) that is not sufficient, since one can also invoke those handlers by returning from main, which cannot be unsafe. Therefore, Rust picks the second option.

I don't know what you are talking about; the fact that one can return from main only once is entirely irrelevant here.

@RalfJung RalfJung closed this Feb 4, 2025
@RalfJung RalfJung reopened this Feb 4, 2025
@safinaskar
Copy link
Contributor

@RalfJung
Thanks for answer. You are always very helpful.

First of all, that text is pre-existing, not touched by this PR

Yes, this is what I meant. I thought this is good idea to fix whole exit documentation at once.

Okay, now I see you are right

@RalfJung
Copy link
Member Author

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 14, 2025
@rustbot rustbot assigned BurntSushi and unassigned thomcc Feb 14, 2025
@RalfJung
Copy link
Member Author

@the8472 maybe you could take a look at this one?

@the8472 the8472 assigned the8472 and unassigned BurntSushi Feb 18, 2025
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

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

I notice that nowhere it's explicitly said that we're calling C's exit here and not _exit, quick_exit or whatever.

The added paragraph is only relevant if we're actually calling a function which has these issues. So either we commit to calling this function (which woul constitute a new API guarantee) or add the caveat "on platforms where this is implemented as C's exit()".

@the8472
Copy link
Member

the8472 commented Mar 17, 2025

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 17, 2025

📌 Commit c133e22 has been approved by the8472

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 18, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 18, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang#136320 (exit: document interaction with C)
 - rust-lang#138301 (Implement `read_buf` for Hermit)
 - rust-lang#138508 (Clarify "owned data" in E0515.md)
 - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename)
 - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`)
 - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch)
 - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#136320 (exit: document interaction with C)
 - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`)
 - rust-lang#138301 (Implement `read_buf` for Hermit)
 - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`)
 - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors)
 - rust-lang#138642 (Unvacation myself)
 - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#136320 (exit: document interaction with C)
 - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`)
 - rust-lang#138301 (Implement `read_buf` for Hermit)
 - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`)
 - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors)
 - rust-lang#138642 (Unvacation myself)
 - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`)

r? `@ghost`
`@rustbot` modify labels: rollup
Sakib25800 added a commit to Sakib25800/rust that referenced this pull request Mar 19, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#136320 (exit: document interaction with C)
 - rust-lang#138080 (Leave a breadcrumb towards bootstrap config documentation in `bootstrap.toml`)
 - rust-lang#138301 (Implement `read_buf` for Hermit)
 - rust-lang#138569 (rustdoc-json: Add tests for `#[repr(...)]`)
 - rust-lang#138635 (Extract `for_each_immediate_subpat` from THIR pattern visitors)
 - rust-lang#138642 (Unvacation myself)
 - rust-lang#138644 (Add `#[cfg(test)]` for Transition in dfa in `rustc_transmute`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 098db78 into rust-lang:master Mar 19, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 19, 2025
@RalfJung RalfJung deleted the exit branch March 21, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants