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

Rework init and cleanup #84115

Merged
merged 6 commits into from
Apr 25, 2021
Merged

Rework init and cleanup #84115

merged 6 commits into from
Apr 25, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 12, 2021

This PR reworks the code in std that runs before and after main and centralizes this code respectively in the functions init and cleanup in both sys_common and sys. This makes is easy to see what code is executed during initialization and cleanup on each platform just by looking at e.g. sys::windows::init.

Full list of changes:

  • new module rt in sys_common to contain init and cleanup and the runtime macros.
  • at_exit and the mechanism to register exit handlers has been completely removed. In practice this was only used for closing sockets on windows and flushing stdout, which have been moved to cleanup.
  • On windows alloc and net initialization is now done in init, this saves a runtime check in every allocation and network use.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2021
@CDirkx

This comment has been minimized.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 12, 2021
@Mark-Simulacrum
Copy link
Member

Would it be possible to avoid changing the behavior of when initialization is done (on Windows), making this a pure refactoring PR, and split that work out into a separate PR?

The changes to initialization time for the Windows code are not obviously correct to me, as I think we don't make any guarantees that the init() function is run before std code is executed - e.g., if calling into a Rust library from C - is that right? Do we have some linker trickery to ensure it runs in that case?

cc @m-ou-se as well; it'd be good to get some more eyes on this in the long run; I think we should get some eyes on PRs of this subtlety :)

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 12, 2021

Of course. I excluded the last two commits containing the changes to initialization, so this is now purely a refactoring.

@m-ou-se m-ou-se self-assigned this Apr 12, 2021
@bors
Copy link
Contributor

bors commented Apr 15, 2021

☔ The latest upstream changes (presumably #84206) made this pull request unmergeable. Please resolve the merge conflicts.

library/std/src/sys/windows/net.rs Outdated Show resolved Hide resolved
library/std/src/sys_common/rt.rs Outdated Show resolved Hide resolved
Comment on lines 10 to 11
static INIT: Once = Once::new();
INIT.call_once(|| unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

What's the use of the Once static here? Don't we already know for sure this is called exactly once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sys_common::rt::cleanup needs to be safe to call multiple times: it could be called in process::exit, then panic, which is caught by lang_start_internal, and then be called again. For symmetry I then made sys_common::rt::init also not unsafe to call, which is why I added the Once, to not have to rely on that it can only be called once remaining true in the future. I could remove it however.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exit() could also be called from multiple threads concurrently, so cleanup definitely needs this. But for init I'd remove it if it's not necessary, to not confuse the reader about what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 👍🏻

library/std/src/sys/unix/mod.rs Show resolved Hide resolved
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 18, 2021

The changes to initialization time for the Windows code are not obviously correct to me, as I think we don't make any guarantees that the init() function is run before std code is executed - e.g., if calling into a Rust library from C - is that right? Do we have some linker trickery to ensure it runs in that case?

I confirmed this is correct, init is not always run, such as when calling a Rust library externally. The additional changes I proposed are thus indeed not correct. I added some comments to explicitly state that neither init or cleanup is guaranteed to run.

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 18, 2021

I'm not sure what is going on with the runtime on hermit:

#[cfg(not(test))]
#[no_mangle]
pub unsafe extern "C" fn runtime_entry(
argc: i32,
argv: *const *const c_char,
env: *const *const c_char,
) -> ! {
use crate::sys::hermit::thread_local_dtor::run_dtors;
extern "C" {
fn main(argc: isize, argv: *const *const c_char) -> i32;
}
// initialize environment
os::init_environment(env as *const *const i8);
let result = main(argc as isize, argv);
run_dtors();
abi::exit(result);
}

It could be that runtime_entry is used instead of lang_start_internal (in which case args::init would currently never be called), or not at all (in which case os::init_environment would currently never be called), or both?

I could change it to:

    // initialize environment 
    os::init_environment(env as *const *const i8); 

    sys_common::rt::init(argc as isize, argv);
    let result = main(argc as isize, argv); 
    sys_common::rt::cleanup();

    abi::exit(result); 

and move run_dtors to cleanup.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

@CDirkx lang_start_internal is called by lang_start which is a #[lang = "start"] item. Rustc will automatically generate a main* symbol that will call that function. (Grep for declare_c_main in compiler/.) On most platforms, Rust's std relies on libc to provide the actual entry point for the binary and expects it to call main. On hermit, we apparently provide this entry point ourselves.

So, you can read that call to main() in sys/hermit/mod.rs as a call to rt::lang_start_internal.


*Unrelated to the Rust fn main, since that one is not #[no_mangle].

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 21, 2021

Unrelated to the Rust fn main, since that one is not #[no_mangle].

Ah that's where my confusion came from, then I'll leave it as is.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

@bors r+ rollup=iffy

Not rolling this one up, since it touches a lot of platform-specific code, so there's a higher chance of CI failures.

@bors
Copy link
Contributor

bors commented Apr 21, 2021

📌 Commit 83608021cca5419d8dc1424c5c42bbd94b6cad8f has been approved by m-ou-se

@bors
Copy link
Contributor

bors commented Apr 21, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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 Apr 21, 2021
@bors
Copy link
Contributor

bors commented Apr 22, 2021

☔ The latest upstream changes (presumably #84411) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 22, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 22, 2021

Rebased and resolved merge conflicts

@m-ou-se
Copy link
Member

m-ou-se commented Apr 22, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 22, 2021

📌 Commit 9f153eeb34684924aa7880fac6f0e4926a471f20 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 22, 2021
@bors
Copy link
Contributor

bors commented Apr 22, 2021

⌛ Testing commit 9f153eeb34684924aa7880fac6f0e4926a471f20 with merge 7289a3db6602624f3e3bc554f0668a599c5699b3...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 22, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 22, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 22, 2021

Added the extra required unsafe block on sgx

@m-ou-se
Copy link
Member

m-ou-se commented Apr 24, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2021

📌 Commit 7171fec has been approved by m-ou-se

@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 Apr 24, 2021
@bors
Copy link
Contributor

bors commented Apr 25, 2021

⌛ Testing commit 7171fec with merge 5da10c0...

@bors
Copy link
Contributor

bors commented Apr 25, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 5da10c0 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants