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

Use futex-based synchronization on Apple platforms #122408

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@
#![feature(str_internals)]
#![feature(strict_provenance)]
#![feature(strict_provenance_atomic_ptr)]
#![feature(sync_unsafe_cell)]
// tidy-alphabetical-end
//
// Library features (alloc):
Expand Down
4 changes: 4 additions & 0 deletions library/std/src/sys/locks/condvar/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ cfg_if::cfg_if! {
target_os = "fuchsia",
all(target_family = "wasm", target_feature = "atomics"),
target_os = "hermit",
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
Comment on lines +12 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use target_vendor = "apple", as that also includes visionOS. Also applies elsewhere.

))] {
mod futex;
pub use futex::Condvar;
Expand Down
47 changes: 47 additions & 0 deletions library/std/src/sys/locks/mutex/apple.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
//! Mutex for Apple platforms.
//!
//! On Apple platforms, priority inheritance is the default for locks. To avoid
//! having to use pthread's mutex, which needs some tricks to work correctly, we
//! instead use `os_unfair_lock`, which is small, movable and supports priority-
Copy link
Contributor

Choose a reason for hiding this comment

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

movable

Could you provide some documentation for this claim? The comment on os_unfair_lock states:

In Swift, note that use of the & operator on an unfair lock can copy or move the lock memory, leading to misbehavior. Use an OSAllocatedUnfairLock to safely wrap access to the lock memory instead. If you use os_unfair_lock APIs directly, always make sure to store and use the lock in memory with a stable address.

Which seems to hint at the lock not being movable? Or am I misunderstanding?

//! inheritance and appeared with macOS 10.12, which is exactly our minimum
//! supported version.

use crate::cell::SyncUnsafeCell;

// FIXME: move these definitions to libc

#[allow(non_camel_case_types)]
#[repr(C)]
struct os_unfair_lock {
_opaque: u32,
}

const OS_UNFAIR_LOCK_INIT: os_unfair_lock = os_unfair_lock { _opaque: 0 };

extern "C" {
fn os_unfair_lock_lock(lock: *mut os_unfair_lock);
fn os_unfair_lock_trylock(lock: *mut os_unfair_lock) -> bool;
fn os_unfair_lock_unlock(lock: *mut os_unfair_lock);
}

pub struct Mutex {
lock: SyncUnsafeCell<os_unfair_lock>,
}

impl Mutex {
pub const fn new() -> Mutex {
Mutex { lock: SyncUnsafeCell::new(OS_UNFAIR_LOCK_INIT) }
}

pub fn lock(&self) {
unsafe { os_unfair_lock_lock(self.lock.get()) }
}

pub fn try_lock(&self) -> bool {
unsafe { os_unfair_lock_trylock(self.lock.get()) }
}

pub unsafe fn unlock(&self) {
unsafe { os_unfair_lock_unlock(self.lock.get()) }
}
}
8 changes: 8 additions & 0 deletions library/std/src/sys/locks/mutex/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ cfg_if::cfg_if! {
))] {
mod futex;
pub use futex::Mutex;
} else if #[cfg(any(
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
))] {
mod apple;
pub use apple::Mutex;
} else if #[cfg(target_os = "fuchsia")] {
mod fuchsia;
pub use fuchsia::Mutex;
Expand Down
4 changes: 4 additions & 0 deletions library/std/src/sys/locks/rwlock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ cfg_if::cfg_if! {
target_os = "fuchsia",
all(target_family = "wasm", target_feature = "atomics"),
target_os = "hermit",
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
))] {
mod futex;
pub use futex::RwLock;
Expand Down
184 changes: 184 additions & 0 deletions library/std/src/sys/pal/unix/futex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
target_os = "openbsd",
target_os = "dragonfly",
target_os = "fuchsia",
target_os = "macos",
target_os = "ios",
target_os = "tvos",
target_os = "watchos",
))]

use crate::sync::atomic::AtomicU32;
Expand Down Expand Up @@ -135,6 +139,186 @@ pub fn futex_wake_all(futex: &AtomicU32) {
};
}

/// With macOS version 14.4, Apple introduced a public futex API. Unfortunately,
/// our minimum supported version is 10.12, so we need a fallback API. Luckily
/// for us, the underlying syscalls have been available since exactly that
/// version, so we just use those when needed. This is private API however,
/// which means we need to take care to avoid breakage if the syscall is removed
/// and to avoid apps being rejected from the App Store. To do this, we use weak
/// linkage emulation for both the public and the private API.
///
/// See os/os_sync_wait_on_address.h (Apple has failed to upload the documentation
/// to their website) for documentation of the public API and
/// https://github.com/apple-oss-distributions/xnu/blob/1031c584a5e37aff177559b9f69dbd3c8c3fd30a/bsd/sys/ulock.h#L69
/// for the header file of the private API, along with its usage in libpthread
/// https://github.com/apple-oss-distributions/libpthread/blob/d8c4e3c212553d3e0f5d76bb7d45a8acd61302dc/src/pthread_cond.c#L463
Comment on lines +150 to +154
Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos",))]
mod apple {
use crate::ffi::{c_int, c_void};
use crate::sys::pal::weak::weak;

pub const OS_CLOCK_MACH_ABSOLUTE_TIME: u32 = 32;
pub const OS_SYNC_WAIT_ON_ADDRESS_NONE: u32 = 0;
pub const OS_SYNC_WAKE_BY_ADDRESS_NONE: u32 = 0;

pub const UL_COMPARE_AND_WAIT: u32 = 1;
pub const ULF_WAKE_ALL: u32 = 0x100;
// The syscalls support directly returning errors instead of going through errno.
pub const ULF_NO_ERRNO: u32 = 0x1000000;

// These functions appeared with macOS 14.4, iOS 17.4, tvOS 17.4, watchOS 10.4, visionOS 1.1.
weak! {
pub fn os_sync_wait_on_address(*mut c_void, u64, usize, u32) -> c_int
}

weak! {
pub fn os_sync_wait_on_address_with_timeout(*mut c_void, u64, usize, u32, u32, u64) -> c_int
}

weak! {
pub fn os_sync_wake_by_address_any(*mut c_void, usize, u32) -> c_int
}

weak! {
pub fn os_sync_wake_by_address_all(*mut c_void, usize, u32) -> c_int
}

// This syscall appeared with macOS 11.0.
// It is used to support nanosecond precision for timeouts, among other features.
weak! {
pub fn __ulock_wait2(u32, *mut c_void, u64, u64, u64) -> c_int
}

// These syscalls appeared with macOS 10.12.
weak! {
pub fn __ulock_wait(u32, *mut c_void, u64, u32) -> c_int
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind explaining more what the thought process of these going undetected is? Seems like a risky chance that might require a hot fix if the MAS or iOS app store scanners see through this.

In reality I think this won't be enough and we need to call ulock things via syscall directly instead. This means that the "fallback" Apple futex implementation is only going to be usable on macOS.

I know Electron apps on the MAS use direct syscalls to emulate private functions, so this is probably what Rust should do too on macOS with precedence:

#define	SYS_ulock_wait     515
#define	SYS_ulock_wake     516
#define	SYS_ulock_wait2    544

For iOS, detect if the os_sync_ functions are available or fallback to the non-futex implementation :/ Calling a syscall directly is forbidden per the iOS headers so std can't do the same as macOS:

__WATCHOS_PROHIBITED __TVOS_PROHIBITED
__OS_AVAILABILITY_MSG(ios,deprecated=10.0,"syscall(2) is unsupported; "
    "please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost().")
__OS_AVAILABILITY_MSG(macosx,deprecated=10.12,"syscall(2) is unsupported; "
    "please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost().")
int	 syscall(int, ...);

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a "spirit of the law" kind of situation. Apple understandably wants App Store software to continue working in newer versions, so they prohibit private API use, as they want to be able to remove or change that API at will. By only using the private API as fallback and not linking it directly, we fulfil this wish. If we directly linked the private API instead, we'd get dynamic linker errors if they remove it, so we can't do that.

On the other hand, we break the "letter of the law" as we are using private API. In my opinion, this is totally fine and justified, but of course, this is only my interpretation. It would be great if someone from Apple could look over this, just so that we know that they are aware of this. My past attempt at reaching out to them for this has been unsuccessful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider the solution of atomic-wait? They use libc++.

https://github.com/m-ou-se/atomic-wait/blob/main/src/macos.rs

Copy link
Member Author

Choose a reason for hiding this comment

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

But these only exist starting with macOS 11, which is way above our minimum of 10.12, so the ulock fallback would be needed regardless.

Copy link
Contributor

@madsmtm madsmtm Sep 26, 2024

Choose a reason for hiding this comment

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

This sets precedent for using private APIs in the standard library (our current use of weak! is only used for symbols that are available in newer versions), which I think is a fairly big deal, and should perhaps be discussed more broadly (in a separate issue? (that could be FCP'ed?)).

Somewhat related is #114186. Tagging in particular @thomcc and @workingjubilee, as they seem to have been involved in this kind of stuff before?


At the very least, we should use the dlsym! macro explicitly here, to make it very clear that we're not weakly linking the symbol (which would be very visible in the binary), but actually loading it at runtime, and thereby evading the App Store's checks (dlsym itself seems to be allowed).

I believe this is sufficient such that we do not need to do raw syscalls (which I remember to have caused problems for Go in the past since as the ABI isn't stable (?)).

}

weak! {
pub fn __ulock_wake(u32, *mut c_void, u64) -> c_int
}
}

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos",))]
pub fn futex_wait(futex: &AtomicU32, expected: u32, timeout: Option<Duration>) -> bool {
use crate::mem::size_of;
use apple::*;

let addr = futex.as_ptr().cast();
let value = expected as u64;
let size = size_of::<u32>();
if let Some(timeout) = timeout {
let timeout_ns = timeout.as_nanos().clamp(1, u64::MAX as u128) as u64;
let timeout_ms = timeout.as_micros().clamp(1, u32::MAX as u128) as u32;

if let Some(wait) = os_sync_wait_on_address_with_timeout.get() {
let r = unsafe {
wait(
addr,
value,
size,
OS_SYNC_WAIT_ON_ADDRESS_NONE,
OS_CLOCK_MACH_ABSOLUTE_TIME,
timeout_ns,
)
};

// We promote spurious wakeups (reported as EINTR) to normal ones for
// simplicity and because Apple's documentation is unclear as to what the
// unit for the deadline of `os_sync_wait_on_address_with_deadline` is,
// making it hard to support timeouts in a fashion similar to the Linux
// futex implementation.
joboet marked this conversation as resolved.
Show resolved Hide resolved
r != -1 || super::os::errno() != libc::ETIMEDOUT
} else if let Some(wait) = __ulock_wait2.get() {
unsafe {
wait(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, value, timeout_ns, 0)
!= -libc::ETIMEDOUT
}
} else if let Some(wait) = __ulock_wait.get() {
unsafe {
wait(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, value, timeout_ms)
!= -libc::ETIMEDOUT
}
} else {
panic!("your system is below the minimum supported version of Rust");
}
} else {
if let Some(wait) = os_sync_wait_on_address.get() {
unsafe {
wait(addr, value, size, OS_SYNC_WAIT_ON_ADDRESS_NONE);
}
} else if let Some(wait) = __ulock_wait.get() {
unsafe {
wait(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, value, 0);
}
} else {
panic!("your system is below the minimum supported version of Rust");
Copy link
Contributor

Choose a reason for hiding this comment

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

These panics also get to be deleted if raw syscalls are used :)

Copy link
Contributor

@madsmtm madsmtm Sep 26, 2024

Choose a reason for hiding this comment

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

If we do keep the panics, I'd go for an abort here instead, this is not recoverable in any way and avoiding the overhead from unwind landing pads would be nice.

}
true
}
}

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos",))]
pub fn futex_wake(futex: &AtomicU32) -> bool {
use crate::io::Error;
use crate::mem::size_of;
use apple::*;

let addr = futex.as_ptr().cast();
if let Some(wake) = os_sync_wake_by_address_any.get() {
unsafe { wake(addr, size_of::<u32>(), OS_SYNC_WAKE_BY_ADDRESS_NONE) == 0 }
} else if let Some(wake) = __ulock_wake.get() {
// __ulock_wake can get interrupted, so retry until either waking up a
// waiter or failing because there are no waiters (ENOENT).
loop {
let r = unsafe { wake(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, 0) };

if r >= 0 {
return true;
} else {
match -r {
libc::ENOENT => return false,
libc::EINTR => continue,
err => panic!("__ulock_wake failed: {}", Error::from_raw_os_error(err)),
}
}
}
Comment on lines +268 to +285
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are you sure this is correct? The implementation of os_sync_wake_by_address_any seems to just call __ulock_wake without a loop? So either we need to loop both places, or in none of them?

Same goes for futex_wake.

} else {
panic!("your system is below the minimum supported version of Rust");
}
}

#[cfg(any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos",))]
pub fn futex_wake_all(futex: &AtomicU32) {
use crate::io::Error;
use crate::mem::size_of;
use apple::*;

let addr = futex.as_ptr().cast();

if let Some(wake) = os_sync_wake_by_address_all.get() {
unsafe {
wake(addr, size_of::<u32>(), OS_SYNC_WAKE_BY_ADDRESS_NONE);
}
} else if let Some(wake) = __ulock_wake.get() {
loop {
let r = unsafe { wake(UL_COMPARE_AND_WAIT | ULF_WAKE_ALL | ULF_NO_ERRNO, addr, 0) };

if r >= 0 {
return;
} else {
match -r {
libc::ENOENT => return,
libc::EINTR => continue,
err => panic!("__ulock_wake failed: {}", Error::from_raw_os_error(err)),
}
}
}
} else {
panic!("your system is below the minimum supported version of Rust");
}
}

#[cfg(target_os = "openbsd")]
pub fn futex_wait(futex: &AtomicU32, expected: u32, timeout: Option<Duration>) -> bool {
use super::time::Timespec;
Expand Down
Loading
Loading