From 8b2bdfd453d196f4d108183efe1f5a58292d5f11 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 20 Sep 2020 14:46:10 +0200 Subject: [PATCH 1/4] Improve std::sys::windows::compat. - Module name can now be any string, not just an ident. (Not all Windows api modules are valid Rust identifiers.) - Adds c::FuncName::is_available() for checking if a function is really available without having to do a duplicate lookup. - Add comment explaining the lack of locking. - Use `$_:block` to simplify the macro_rules. - Apply allow(unused_variables) only to the fallback instead of everything. --- library/std/src/sys/windows/c.rs | 2 +- library/std/src/sys/windows/compat.rs | 63 +++++++++++++++------------ 2 files changed, 36 insertions(+), 29 deletions(-) diff --git a/library/std/src/sys/windows/c.rs b/library/std/src/sys/windows/c.rs index f440442ca30..559c4dc9c7c 100644 --- a/library/std/src/sys/windows/c.rs +++ b/library/std/src/sys/windows/c.rs @@ -1032,7 +1032,7 @@ extern "system" { // Functions that aren't available on every version of Windows that we support, // but we still use them and just provide some form of a fallback implementation. compat_fn! { - kernel32: + "kernel32": pub fn CreateSymbolicLinkW(_lpSymlinkFileName: LPCWSTR, _lpTargetFileName: LPCWSTR, diff --git a/library/std/src/sys/windows/compat.rs b/library/std/src/sys/windows/compat.rs index d6d433f9d08..897f49445ee 100644 --- a/library/std/src/sys/windows/compat.rs +++ b/library/std/src/sys/windows/compat.rs @@ -12,7 +12,6 @@ //! function is available but afterwards it's just a load and a jump. use crate::ffi::CString; -use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sys::c; pub fn lookup(module: &str, symbol: &str) -> Option { @@ -28,45 +27,53 @@ pub fn lookup(module: &str, symbol: &str) -> Option { } } -pub fn store_func(ptr: &AtomicUsize, module: &str, symbol: &str, fallback: usize) -> usize { - let value = lookup(module, symbol).unwrap_or(fallback); - ptr.store(value, Ordering::SeqCst); - value -} - macro_rules! compat_fn { - ($module:ident: $( + ($module:literal: $( $(#[$meta:meta])* - pub fn $symbol:ident($($argname:ident: $argtype:ty),*) - -> $rettype:ty { - $($body:expr);* - } + pub fn $symbol:ident($($argname:ident: $argtype:ty),*) -> $rettype:ty $body:block )*) => ($( - #[allow(unused_variables)] $(#[$meta])* - pub unsafe fn $symbol($($argname: $argtype),*) -> $rettype { + pub mod $symbol { + use super::*; use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::mem; - type F = unsafe extern "system" fn($($argtype),*) -> $rettype; static PTR: AtomicUsize = AtomicUsize::new(0); + #[allow(unused_variables)] + unsafe extern "system" fn fallback($($argname: $argtype),*) -> $rettype $body + + #[cold] fn load() -> usize { - crate::sys::compat::store_func(&PTR, - stringify!($module), - stringify!($symbol), - fallback as usize) - } - unsafe extern "system" fn fallback($($argname: $argtype),*) - -> $rettype { - $($body);* + // There is no locking here. It's okay if this is executed by multiple threads in + // parallel. `lookup` will result in the same value, and it's okay if they overwrite + // eachothers result as long as they do so atomically. We don't need any guarantees + // about memory ordering, as this involves just a single atomic variable which is + // not used to protect or order anything else. + let addr = crate::sys::compat::lookup($module, stringify!($symbol)) + .unwrap_or(fallback as usize); + PTR.store(addr, Ordering::Relaxed); + addr } - let addr = match PTR.load(Ordering::SeqCst) { - 0 => load(), - n => n, - }; - mem::transmute::(addr)($($argname),*) + fn addr() -> usize { + match PTR.load(Ordering::Relaxed) { + 0 => load(), + addr => addr, + } + } + + #[allow(dead_code)] + pub fn is_available() -> bool { + addr() != fallback as usize + } + + pub unsafe fn call($($argname: $argtype),*) -> $rettype { + type F = unsafe extern "system" fn($($argtype),*) -> $rettype; + mem::transmute::(addr())($($argname),*) + } } + + pub use $symbol::call as $symbol; )*) } From 93310efdbe80d01e287b935e0f86a834bc6c4574 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 20 Sep 2020 14:49:04 +0200 Subject: [PATCH 2/4] Use AcquireSRWLockExclusive::is_available() instead of an extra lookup. --- library/std/src/sys/windows/mutex.rs | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/library/std/src/sys/windows/mutex.rs b/library/std/src/sys/windows/mutex.rs index 1e09b95c872..1028dc9ff50 100644 --- a/library/std/src/sys/windows/mutex.rs +++ b/library/std/src/sys/windows/mutex.rs @@ -23,7 +23,6 @@ use crate::cell::{Cell, UnsafeCell}; use crate::mem::{self, MaybeUninit}; use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sys::c; -use crate::sys::compat; pub struct Mutex { // This is either directly an SRWLOCK (if supported), or a Box otherwise. @@ -40,8 +39,8 @@ struct Inner { #[derive(Clone, Copy)] enum Kind { - SRWLock = 1, - CriticalSection = 2, + SRWLock, + CriticalSection, } #[inline] @@ -130,21 +129,11 @@ impl Mutex { } fn kind() -> Kind { - static KIND: AtomicUsize = AtomicUsize::new(0); - - let val = KIND.load(Ordering::SeqCst); - if val == Kind::SRWLock as usize { - return Kind::SRWLock; - } else if val == Kind::CriticalSection as usize { - return Kind::CriticalSection; + if c::AcquireSRWLockExclusive::is_available() { + Kind::SRWLock + } else { + Kind::CriticalSection } - - let ret = match compat::lookup("kernel32", "AcquireSRWLockExclusive") { - None => Kind::CriticalSection, - Some(..) => Kind::SRWLock, - }; - KIND.store(ret as usize, Ordering::SeqCst); - ret } pub struct ReentrantMutex { From 09cbaf436713ddb864725a50ccdff67e2ab9bc77 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 20 Sep 2020 16:30:11 +0200 Subject: [PATCH 3/4] Formatting. --- library/std/src/sys/windows/mutex.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/library/std/src/sys/windows/mutex.rs b/library/std/src/sys/windows/mutex.rs index 1028dc9ff50..e2aaca59fe2 100644 --- a/library/std/src/sys/windows/mutex.rs +++ b/library/std/src/sys/windows/mutex.rs @@ -129,11 +129,7 @@ impl Mutex { } fn kind() -> Kind { - if c::AcquireSRWLockExclusive::is_available() { - Kind::SRWLock - } else { - Kind::CriticalSection - } + if c::AcquireSRWLockExclusive::is_available() { Kind::SRWLock } else { Kind::CriticalSection } } pub struct ReentrantMutex { From 63b6007d5b68023e02a43c2f9c2ed21762cd8011 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Thu, 1 Oct 2020 16:49:55 +0200 Subject: [PATCH 4/4] Work around potential merging/duplication issues in sys/windows/compat. --- library/std/src/sys/windows/compat.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/library/std/src/sys/windows/compat.rs b/library/std/src/sys/windows/compat.rs index 897f49445ee..3f25f05e1b9 100644 --- a/library/std/src/sys/windows/compat.rs +++ b/library/std/src/sys/windows/compat.rs @@ -38,11 +38,28 @@ macro_rules! compat_fn { use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::mem; + type F = unsafe extern "system" fn($($argtype),*) -> $rettype; + static PTR: AtomicUsize = AtomicUsize::new(0); #[allow(unused_variables)] unsafe extern "system" fn fallback($($argname: $argtype),*) -> $rettype $body + /// This address is stored in `PTR` to incidate an unavailable API. + /// + /// This way, call() will end up calling fallback() if it is unavailable. + /// + /// This is a `static` to avoid rustc duplicating `fn fallback()` + /// into both load() and is_available(), which would break + /// is_available()'s comparison. By using the same static variable + /// in both places, they'll refer to the same (copy of the) + /// function. + /// + /// LLVM merging the address of fallback with other functions + /// (because of unnamed_addr) is fine, since it's only compared to + /// an address from GetProcAddress from an external dll. + static FALLBACK: F = fallback; + #[cold] fn load() -> usize { // There is no locking here. It's okay if this is executed by multiple threads in @@ -51,7 +68,7 @@ macro_rules! compat_fn { // about memory ordering, as this involves just a single atomic variable which is // not used to protect or order anything else. let addr = crate::sys::compat::lookup($module, stringify!($symbol)) - .unwrap_or(fallback as usize); + .unwrap_or(FALLBACK as usize); PTR.store(addr, Ordering::Relaxed); addr } @@ -65,11 +82,10 @@ macro_rules! compat_fn { #[allow(dead_code)] pub fn is_available() -> bool { - addr() != fallback as usize + addr() != FALLBACK as usize } pub unsafe fn call($($argname: $argtype),*) -> $rettype { - type F = unsafe extern "system" fn($($argtype),*) -> $rettype; mem::transmute::(addr())($($argname),*) } }