Simplify command-line argument initialization on unix #87236
Conversation
As of 91c3eee, the global ARGC and ARGV no longer reference dynamically-allocated memory, so they don't need to be cleaned up.
In the command-line argument initialization code, remove the Mutex around the `ARGV` and `ARGC` variables, and simply check whether ARGV is non-null before dereferencing it. This way, if either of ARGV or ARGC is not initialized, we'll get an empty argument list. This allows simple cdylibs to avoid having `pthread_mutex_lock`/`pthread_mutex_unlock` appear in their symbol tables if they don't otherwise use threads.
|
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
|
📌 Commit c3df0ae has been approved by |
bors
added
S-waiting-on-bors
| let argv = ARGV.load(Ordering::Relaxed); | ||
| let argc = if argv.is_null() { 0 } else { ARGC.load(Ordering::Relaxed) }; |
There was a problem hiding this comment.
Don't you need to do an acquire load (and a release store elsewhere) to ensure that the non-atomic load later (*argv.offset(i)) does not cause a data race?
There was a problem hiding this comment.
I assume by this point the memory pointed to by argv is already ready to be loaded from without synchronization, since that's how C code would use argv.
|
|
||
| unsafe fn really_init(argc: isize, argv: *const *const u8) { | ||
| let _guard = LOCK.lock(); | ||
| ARGC.store(argc, Ordering::Relaxed); | ||
| ARGV.store(argv as *mut _, Ordering::Relaxed); |
There was a problem hiding this comment.
This should probably be a release store to ensure that the writes to the data that sits behind this argv pointer are visible.
There was a problem hiding this comment.
Those writes are performed before lang_start is called, by code that must assume that we may read from that memory immediately without atomics or synchronization.
For a spec reference, I found C11 5.1.2.2.1p2 which says the argv array is given values "prior to program startup".
There was a problem hiding this comment.
So this is directly using the system-provided argc/argv, not something constructed by Rust itself? I guess it's technically correct then. But this seems subtle enough that it warrants a comment.
I'd probably still use release/acquire accesses. They are just more obviously correct. IMO Relaxed should only be used when there is a good perf reason to do so, and needs to come with comments explaining why not doing release/acquire is okay. Relaxed is a very subtle memory order that should not be used lightly.
There was a problem hiding this comment.
Yes, this is the system-provided argc/argv. I submitted #87279 to address this.
…laumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#86230 (Add --nocapture option to rustdoc) - rust-lang#87210 (Rustdoc accessibility: make the sidebar headers actual headers) - rust-lang#87227 (Move asm! and global_asm! to core::arch) - rust-lang#87236 (Simplify command-line argument initialization on unix) - rust-lang#87251 (Fix "item info" width) - rust-lang#87256 (Extend HIR-based WF checking to associated type defaults) - rust-lang#87259 (triagebot shortcut config) - rust-lang#87268 (Don't create references to uninitialized data in `List::from_arena`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Following up on rust-lang#87236, add comments to the unix command-line argument support explaining that the code doesn't mutate the system-provided argc/argv, and that this is why the code doesn't need a lock or special memory ordering.
…alfJung Add comments explaining the unix command-line argument support. Following up on rust-lang#87236, add comments to the unix command-line argument support explaining that the code doesn't mutate the system-provided argc/argv, and that this is why the code doesn't need a lock or special memory ordering. r? `@RalfJung`
…alfJung Add comments explaining the unix command-line argument support. Following up on rust-lang#87236, add comments to the unix command-line argument support explaining that the code doesn't mutate the system-provided argc/argv, and that this is why the code doesn't need a lock or special memory ordering. r? ``@RalfJung``
…alfJung Add comments explaining the unix command-line argument support. Following up on rust-lang#87236, add comments to the unix command-line argument support explaining that the code doesn't mutate the system-provided argc/argv, and that this is why the code doesn't need a lock or special memory ordering. r? ```@RalfJung```
Simplify Rust's command-line argument initialization code on unix:
CStrings and aVecat startup time.Mutexisn't needed; if there's somehow a call toargs()before argument initialization has happened, the code returns return an empty list, which we can do with a null check.With these changes, a simple cdylib that doesn't use threads avoids getting
pthread_mutex_lock/pthread_mutex_unlockin its symbol table.