-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Simplify command-line argument initialization on unix #87236
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,6 @@ pub unsafe fn init(argc: isize, argv: *const *const u8) { | |
imp::init(argc, argv) | ||
} | ||
|
||
/// One-time global cleanup. | ||
pub unsafe fn cleanup() { | ||
imp::cleanup() | ||
} | ||
|
||
/// Returns the command line arguments | ||
pub fn args() -> Args { | ||
imp::args() | ||
|
@@ -82,16 +77,10 @@ mod imp { | |
use crate::ptr; | ||
use crate::sync::atomic::{AtomicIsize, AtomicPtr, Ordering}; | ||
|
||
use crate::sys_common::mutex::StaticMutex; | ||
|
||
static ARGC: AtomicIsize = AtomicIsize::new(0); | ||
static ARGV: AtomicPtr<*const u8> = AtomicPtr::new(ptr::null_mut()); | ||
// We never call `ENV_LOCK.init()`, so it is UB to attempt to | ||
// acquire this mutex reentrantly! | ||
static LOCK: StaticMutex = StaticMutex::new(); | ||
|
||
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); | ||
} | ||
|
@@ -127,21 +116,16 @@ mod imp { | |
init_wrapper | ||
}; | ||
|
||
pub unsafe fn cleanup() { | ||
let _guard = LOCK.lock(); | ||
ARGC.store(0, Ordering::Relaxed); | ||
ARGV.store(ptr::null_mut(), Ordering::Relaxed); | ||
} | ||
|
||
pub fn args() -> Args { | ||
Args { iter: clone().into_iter() } | ||
} | ||
|
||
fn clone() -> Vec<OsString> { | ||
unsafe { | ||
let _guard = LOCK.lock(); | ||
let argc = ARGC.load(Ordering::Relaxed); | ||
// Load ARGC and ARGV without a lock. If the store to either ARGV or | ||
// ARGC isn't visible yet, we'll return an empty argument list. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you need to do an acquire load (and a release store elsewhere) to ensure that the non-atomic load later ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
(0..argc) | ||
.map(|i| { | ||
let cstr = CStr::from_ptr(*argv.offset(i) as *const libc::c_char); | ||
|
@@ -159,8 +143,6 @@ mod imp { | |
|
||
pub unsafe fn init(_argc: isize, _argv: *const *const u8) {} | ||
|
||
pub fn cleanup() {} | ||
|
||
#[cfg(target_os = "macos")] | ||
pub fn args() -> Args { | ||
use crate::os::unix::prelude::*; | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the system-provided argc/argv. I submitted #87279 to address this.