From 94b83212f88ce44086e82a35e86f9bc343b9bb64 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Tue, 10 Aug 2021 13:51:58 -0700 Subject: [PATCH 1/3] Fix darwin ulock usage on macOS --- lib/std/Thread/Futex.zig | 19 +++++++++++-------- lib/std/c/darwin.zig | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/std/Thread/Futex.zig b/lib/std/Thread/Futex.zig index 2a1871123145..b093bd23b841 100644 --- a/lib/std/Thread/Futex.zig +++ b/lib/std/Thread/Futex.zig @@ -180,23 +180,22 @@ const DarwinFutex = struct { const darwin = std.os.darwin; fn wait(ptr: *const Atomic(u32), expect: u32, timeout: ?u64) error{TimedOut}!void { - // ulock_wait() uses micro-second timeouts, where 0 = INIFITE or no-timeout - var timeout_us: u32 = 0; - if (timeout) |timeout_ns| { - timeout_us = @intCast(u32, timeout_ns / std.time.ns_per_us); - } - // Darwin XNU 7195.50.7.100.1 introduced __ulock_wait2 and migrated code paths (notably pthread_cond_t) towards it: // https://github.com/apple/darwin-xnu/commit/d4061fb0260b3ed486147341b72468f836ed6c8f#diff-08f993cc40af475663274687b7c326cc6c3031e0db3ac8de7b24624610616be6 // // This XNU version appears to correspond to 11.0.1: // https://kernelshaman.blogspot.com/2021/01/building-xnu-for-macos-big-sur-1101.html + // + // ulock_wait() uses 32-bit micro-second timeouts, where 0 = INIFITE or no-timeout + // ulock_wait2() uses 64-bit nano-second timeouts (with the same convention) + const timeout_ns: u64 = timeout orelse 0; const addr = @ptrCast(*const c_void, ptr); const flags = darwin.UL_COMPARE_AND_WAIT | darwin.ULF_NO_ERRNO; const status = blk: { if (target.os.version_range.semver.max.major >= 11) { - break :blk darwin.__ulock_wait2(flags, addr, expect, timeout_us, 0); + break :blk darwin.__ulock_wait2(flags, addr, expect, timeout_ns, 0); } else { + const timeout_us = @intCast(u32, timeout_ns / std.time.ns_per_us); break :blk darwin.__ulock_wait(flags, addr, expect, timeout_us); } }; @@ -204,7 +203,10 @@ const DarwinFutex = struct { if (status >= 0) return; switch (-status) { darwin.EINTR => {}, - darwin.EFAULT => unreachable, + // Address of the futex is paged out. This is unlikely, but possible in theory, and + // pthread/libdispatch on darwin bother to handle it. In this case we'll return + // without waiting, but the caller should retry anyway. + darwin.EFAULT => {}, darwin.ETIMEDOUT => return error.TimedOut, else => unreachable, } @@ -223,6 +225,7 @@ const DarwinFutex = struct { if (status >= 0) return; switch (-status) { darwin.EINTR => continue, // spurious wake() + darwin.EFAULT => continue, // address of the lock was paged out darwin.ENOENT => return, // nothing was woken up darwin.EALREADY => unreachable, // only for ULF_WAKE_THREAD else => unreachable, diff --git a/lib/std/c/darwin.zig b/lib/std/c/darwin.zig index e216afd643e3..22473a3899c6 100644 --- a/lib/std/c/darwin.zig +++ b/lib/std/c/darwin.zig @@ -246,7 +246,7 @@ pub const UL_COMPARE_AND_WAIT64 = 5; pub const UL_COMPARE_AND_WAIT64_SHARED = 6; pub const ULF_WAIT_ADAPTIVE_SPIN = 0x40000; -pub extern "c" fn __ulock_wait2(op: u32, addr: ?*const c_void, val: u64, timeout_us: u32, val2: u64) c_int; +pub extern "c" fn __ulock_wait2(op: u32, addr: ?*const c_void, val: u64, timeout_ns: u64, val2: u64) c_int; pub extern "c" fn __ulock_wait(op: u32, addr: ?*const c_void, val: u64, timeout_us: u32) c_int; pub extern "c" fn __ulock_wake(op: u32, addr: ?*const c_void, val: u64) c_int; From de6b4c0714c840887945559f933de4cbd7e7671a Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 11 Aug 2021 08:32:34 -0700 Subject: [PATCH 2/3] Fix review comments, and correctly handle timeout overflow on darwin --- lib/std/Thread/Futex.zig | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/std/Thread/Futex.zig b/lib/std/Thread/Futex.zig index b093bd23b841..5b15e1544858 100644 --- a/lib/std/Thread/Futex.zig +++ b/lib/std/Thread/Futex.zig @@ -186,17 +186,24 @@ const DarwinFutex = struct { // This XNU version appears to correspond to 11.0.1: // https://kernelshaman.blogspot.com/2021/01/building-xnu-for-macos-big-sur-1101.html // - // ulock_wait() uses 32-bit micro-second timeouts, where 0 = INIFITE or no-timeout + // ulock_wait() uses 32-bit micro-second timeouts where 0 = INFINITE or no-timeout // ulock_wait2() uses 64-bit nano-second timeouts (with the same convention) const timeout_ns: u64 = timeout orelse 0; const addr = @ptrCast(*const c_void, ptr); const flags = darwin.UL_COMPARE_AND_WAIT | darwin.ULF_NO_ERRNO; + // If we're using `__ulock_wait` and `timeout` is too big to fit inside a `u32` count of + // micro-seconds (around 70min), we'll request a shorter timeout. This is fine (users + // should handle spurious wakeups), but we need to remember that we did so, so that + // we don't return `TimedOut` incorrectly. + var timeout_overflowed = false; const status = blk: { if (target.os.version_range.semver.max.major >= 11) { break :blk darwin.__ulock_wait2(flags, addr, expect, timeout_ns, 0); } else { - const timeout_us = @intCast(u32, timeout_ns / std.time.ns_per_us); - break :blk darwin.__ulock_wait(flags, addr, expect, timeout_us); + const timeout_us = timeout_ns / std.time.ns_per_us; + timeout_overflowed = timeout_us > std.math.maxInt(u32); + const timeout_us32 = if (timeout_overflowed) std.math.maxInt(u32) else @intCast(u32, timeout_us); + break :blk darwin.__ulock_wait(flags, addr, expect, timeout_us32); } }; @@ -207,7 +214,7 @@ const DarwinFutex = struct { // pthread/libdispatch on darwin bother to handle it. In this case we'll return // without waiting, but the caller should retry anyway. darwin.EFAULT => {}, - darwin.ETIMEDOUT => return error.TimedOut, + darwin.ETIMEDOUT => if (!timeout_overflowed) return error.TimedOut, else => unreachable, } } From 947f69da9e3e9759b9b357c95590936059889343 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 11 Aug 2021 18:27:22 -0700 Subject: [PATCH 3/3] Apply more review suggestions --- lib/std/Thread/Futex.zig | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/std/Thread/Futex.zig b/lib/std/Thread/Futex.zig index 5b15e1544858..81dba0799663 100644 --- a/lib/std/Thread/Futex.zig +++ b/lib/std/Thread/Futex.zig @@ -188,22 +188,29 @@ const DarwinFutex = struct { // // ulock_wait() uses 32-bit micro-second timeouts where 0 = INFINITE or no-timeout // ulock_wait2() uses 64-bit nano-second timeouts (with the same convention) - const timeout_ns: u64 = timeout orelse 0; + var timeout_ns: u64 = 0; + if (timeout) |timeout_value| { + // This should be checked by the caller. + assert(timeout_value != 0); + timeout_ns = timeout_value; + } const addr = @ptrCast(*const c_void, ptr); const flags = darwin.UL_COMPARE_AND_WAIT | darwin.ULF_NO_ERRNO; // If we're using `__ulock_wait` and `timeout` is too big to fit inside a `u32` count of // micro-seconds (around 70min), we'll request a shorter timeout. This is fine (users // should handle spurious wakeups), but we need to remember that we did so, so that - // we don't return `TimedOut` incorrectly. + // we don't return `TimedOut` incorrectly. If that happens, we set this variable to + // true so that we we know to ignore the ETIMEDOUT result. var timeout_overflowed = false; const status = blk: { if (target.os.version_range.semver.max.major >= 11) { break :blk darwin.__ulock_wait2(flags, addr, expect, timeout_ns, 0); } else { - const timeout_us = timeout_ns / std.time.ns_per_us; - timeout_overflowed = timeout_us > std.math.maxInt(u32); - const timeout_us32 = if (timeout_overflowed) std.math.maxInt(u32) else @intCast(u32, timeout_us); - break :blk darwin.__ulock_wait(flags, addr, expect, timeout_us32); + const timeout_us = std.math.cast(u32, timeout_ns / std.time.ns_per_us) catch overflow: { + timeout_overflowed = true; + break :overflow std.math.maxInt(u32); + }; + break :blk darwin.__ulock_wait(flags, addr, expect, timeout_us); } };