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

Unconditionally call really_init on GNU/Linux #124447

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Apr 27, 2024

This makes miri not diverge in behavior, it fixes running Rust linux-gnu binaries on musl with gcompat, it fixes dlopen edge-cases that cranelift somehow hits, etc.

Fixes #124126

thou hast gazed into this abyss with me:
r? @ChrisDenton

This makes miri not diverge in behavior, it fixes running Rust linux-gnu
binaries on musl with gcompat, it fixes dlopen edge-cases that cranelift
somehow hits, etc.
@rustbot rustbot added O-unix Operating system: Unix-like 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 Apr 27, 2024
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Making the gnu case the same as the musl case makes sense to me. It is just setting two atomics so I don't think it saves much to skip init from main

@ChrisDenton
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 27, 2024

📌 Commit fa73ebb has been approved by ChrisDenton

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 Apr 27, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 27, 2024
…nu, r=ChrisDenton

Unconditionally call `really_init` on GNU/Linux

This makes miri not diverge in behavior, it fixes running Rust linux-gnu binaries on musl with gcompat, it fixes dlopen edge-cases that cranelift somehow hits, etc.

Fixes rust-lang#124126

thou hast gazed into this abyss with me:
r? `@ChrisDenton`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123942 (`x vendor`)
 - rust-lang#124165 (add test for incremental ICE: slice-pattern-const.rs rust-lang#83085)
 - rust-lang#124210 (Abort a process when FD ownership is violated)
 - rust-lang#124242 (bootstrap: Describe build_steps modules)
 - rust-lang#124406 (Remove unused `[patch]` for clippy_lints)
 - rust-lang#124429 (bootstrap: Document `struct Builder` and its fields)
 - rust-lang#124447 (Unconditionally call `really_init` on GNU/Linux)

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

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123248 (1.78 release notes)
 - rust-lang#123942 (`x vendor`)
 - rust-lang#124165 (add test for incremental ICE: slice-pattern-const.rs rust-lang#83085)
 - rust-lang#124242 (bootstrap: Describe build_steps modules)
 - rust-lang#124406 (Remove unused `[patch]` for clippy_lints)
 - rust-lang#124429 (bootstrap: Document `struct Builder` and its fields)
 - rust-lang#124447 (Unconditionally call `really_init` on GNU/Linux)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit be89760 into rust-lang:master Apr 28, 2024
10 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2024
Rollup merge of rust-lang#124447 - workingjubilee:set-argv-twice-on-gnu, r=ChrisDenton

Unconditionally call `really_init` on GNU/Linux

This makes miri not diverge in behavior, it fixes running Rust linux-gnu binaries on musl with gcompat, it fixes dlopen edge-cases that cranelift somehow hits, etc.

Fixes rust-lang#124126

thou hast gazed into this abyss with me:
r? ``@ChrisDenton``
@rustbot rustbot added this to the 1.79.0 milestone Apr 28, 2024
@workingjubilee workingjubilee deleted the set-argv-twice-on-gnu branch April 28, 2024 19:32
@joshtriplett
Copy link
Member

Could we check, first, whether we've already had these initialized, and only call this in the unusual case where they're not initialized? That would avoid the extra work in the common case, and do the work in the unusual case.

@workingjubilee
Copy link
Member Author

@joshtriplett how, exactly, is updating an atomic a single extra time in a program's entire lifetime "extra work"?

@workingjubilee
Copy link
Member Author

Doing a branch and only then updating is quite plausibly more work because it impacts the branch prediction tables.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 11, 2024

I'm happy to look at any PR changing this but, as jubilee says, I'm really not clear how we could possibly do less work then a couple of relaxed stores? I mean other than doing nothing, which I'm assuming is still undesirable.

@workingjubilee
Copy link
Member Author

I can add a comment explaining that the reason the scare-quotes are there is because the "duplicate work" is so enormously trivial that on almost all systems it is actually more costly to write the if, as anything that doesn't have a branch predictor also makes its atomic locking implementation dead-simple.

@joshtriplett
Copy link
Member

@workingjubilee I read the couple of GitHub issues too quickly, didn't dig into the body of really_init, and forgot that really_init is only saving off argc and argv as values, not doing the parsing that turns them into the form provided by args()/args_os(). Doing a "duplicate" initialization of two relaxed stores is definitely fine to duplicate, and as you said, would cost more to try to avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like 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.

std::env::args_os always returns an empty iterator in a musl+gcompat environment
5 participants