From 0b2ee093788ba84c5d130c021f347844b5939889 Mon Sep 17 00:00:00 2001 From: r00ster91 Date: Tue, 21 Feb 2023 18:09:00 +0100 Subject: [PATCH 1/3] std.Thread.setName: use unused code I noticed a comment saying that the intent of a code's author was unclear. What happened is that the author forgot to put the check for whether the thread is the calling thread (`self.getHandle() == std.c.pthread_self()`) in the `if (use_pthreads)`. If the thread is the calling thread, we use `prctl` to set or get the thread's name and it does not take a thread id because it knows the id of the thread we're calling `getName` or `setName` from. I have found a source saying that using `pthread_setname_np` on either the calling thread or any other thread by thread id would work too (so we don't need to call `prctl`) but I was not sure if that is the case on all systems so we keep using `pthread_setname_np` if we have a specific thread that is not the thread we're calling from, and `prctl` otherwise. --- lib/std/Thread.zig | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/std/Thread.zig b/lib/std/Thread.zig index 8004f94d7f..a0f66b3fa8 100644 --- a/lib/std/Thread.zig +++ b/lib/std/Thread.zig @@ -62,18 +62,20 @@ pub fn setName(self: Thread, name: []const u8) SetNameError!void { switch (target.os.tag) { .linux => if (use_pthreads) { - const err = std.c.pthread_setname_np(self.getHandle(), name_with_terminator.ptr); - switch (err) { - .SUCCESS => return, - .RANGE => unreachable, - else => |e| return os.unexpectedErrno(e), - } - } else if (use_pthreads and self.getHandle() == std.c.pthread_self()) { - // TODO: this is dead code. what did the author of this code intend to happen here? - const err = try os.prctl(.SET_NAME, .{@ptrToInt(name_with_terminator.ptr)}); - switch (@intToEnum(os.E, err)) { - .SUCCESS => return, - else => |e| return os.unexpectedErrno(e), + if (self.getHandle() == std.c.pthread_self()) { + // Set the name of the calling thread (no thread id required). + const err = try os.prctl(.SET_NAME, .{@ptrToInt(name_with_terminator.ptr)}); + switch (@intToEnum(os.E, err)) { + .SUCCESS => return, + else => |e| return os.unexpectedErrno(e), + } + } else { + const err = std.c.pthread_setname_np(self.getHandle(), name_with_terminator.ptr); + switch (err) { + .SUCCESS => return, + .RANGE => unreachable, + else => |e| return os.unexpectedErrno(e), + } } } else { var buf: [32]u8 = undefined; @@ -177,6 +179,7 @@ pub fn getName(self: Thread, buffer_ptr: *[max_name_len:0]u8) GetNameError!?[]co else => |e| return os.unexpectedErrno(e), } } else if (use_pthreads and self.getHandle() == std.c.pthread_self()) { + // Get the name of the calling thread (no thread id required). const err = try os.prctl(.GET_NAME, .{@ptrToInt(buffer.ptr)}); switch (@intToEnum(os.E, err)) { .SUCCESS => return std.mem.sliceTo(buffer, 0), @@ -325,10 +328,10 @@ pub const SpawnError = error{ Unexpected, }; -/// Spawns a new thread which executes `function` using `args` and returns a handle the spawned thread. +/// Spawns a new thread which executes `function` using `args` and returns a handle to the spawned thread. /// `config` can be used as hints to the platform for now to spawn and execute the `function`. /// The caller must eventually either call `join()` to wait for the thread to finish and free its resources -/// or call `detach()` to excuse the caller from calling `join()` and have the thread clean up its resources on completion`. +/// or call `detach()` to excuse the caller from calling `join()` and have the thread clean up its resources on completion. pub fn spawn(config: SpawnConfig, comptime function: anytype, args: anytype) SpawnError!Thread { if (builtin.single_threaded) { @compileError("Cannot spawn thread when building in single-threaded mode"); From f10100cf9b3d6929f2ed2df64835b2773e117665 Mon Sep 17 00:00:00 2001 From: r00ster91 Date: Fri, 24 Feb 2023 20:58:09 +0100 Subject: [PATCH 2/3] std.Thread: drop is_gnu check I believe the reason we had that check in the first place was because for both `pthread_setname_np` and `pthread_getname_np` man says: ``` CONFORMING TO These functions are nonstandard GNU extensions; hence the suffix "_np" (nonportable) in the names. ``` However, this `is_gnu` check was never consistently applied; it was missing in `setName` in the Linux case. It is also missing on all other call sites for other platforms (macOS, iOS, et al.). Though, that could be because it may only apply to Linux. I think for a best-effort approach it is okay to drop this. It's probably less non-standard than man makes it out to be. --- lib/std/Thread.zig | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/std/Thread.zig b/lib/std/Thread.zig index a0f66b3fa8..795dfb2daa 100644 --- a/lib/std/Thread.zig +++ b/lib/std/Thread.zig @@ -18,7 +18,6 @@ pub const Condition = @import("Thread/Condition.zig"); pub const RwLock = @import("Thread/RwLock.zig"); pub const use_pthreads = target.os.tag != .windows and target.os.tag != .wasi and builtin.link_libc; -const is_gnu = target.abi.isGnu(); const Thread = @This(); const Impl = if (target.os.tag == .windows) @@ -171,21 +170,27 @@ pub fn getName(self: Thread, buffer_ptr: *[max_name_len:0]u8) GetNameError!?[]co var buffer: [:0]u8 = buffer_ptr; switch (target.os.tag) { - .linux => if (use_pthreads and is_gnu) { - const err = std.c.pthread_getname_np(self.getHandle(), buffer.ptr, max_name_len + 1); - switch (err) { - .SUCCESS => return std.mem.sliceTo(buffer, 0), - .RANGE => unreachable, - else => |e| return os.unexpectedErrno(e), + .linux => if (use_pthreads) { + if (self.getHandle() == std.c.pthread_self()) { + // Get the name of the calling thread (no thread id required). + const err = try os.prctl(.GET_NAME, .{@ptrToInt(buffer.ptr)}); + switch (@intToEnum(os.E, err)) { + .SUCCESS => return std.mem.sliceTo(buffer, 0), + else => |e| return os.unexpectedErrno(e), + } + } else { + if (target.abi.isMusl()) { + // musl doesn't provide pthread_getname_np and there's no way to retrieve the thread id of an arbitrary thread. + return error.Unsupported; + } + const err = std.c.pthread_getname_np(self.getHandle(), buffer.ptr, max_name_len + 1); + switch (err) { + .SUCCESS => return std.mem.sliceTo(buffer, 0), + .RANGE => unreachable, + else => |e| return os.unexpectedErrno(e), + } } - } else if (use_pthreads and self.getHandle() == std.c.pthread_self()) { - // Get the name of the calling thread (no thread id required). - const err = try os.prctl(.GET_NAME, .{@ptrToInt(buffer.ptr)}); - switch (@intToEnum(os.E, err)) { - .SUCCESS => return std.mem.sliceTo(buffer, 0), - else => |e| return os.unexpectedErrno(e), - } - } else if (!use_pthreads) { + } else { var buf: [32]u8 = undefined; const path = try std.fmt.bufPrint(&buf, "/proc/self/task/{d}/comm", .{self.getHandle()}); @@ -195,9 +200,6 @@ pub fn getName(self: Thread, buffer_ptr: *[max_name_len:0]u8) GetNameError!?[]co const data_len = try file.reader().readAll(buffer_ptr[0 .. max_name_len + 1]); return if (data_len >= 1) buffer[0 .. data_len - 1] else null; - } else { - // musl doesn't provide pthread_getname_np and there's no way to retrieve the thread id of an arbitrary thread. - return error.Unsupported; }, .windows => { const buf_capacity = @sizeOf(os.windows.UNICODE_STRING) + (@sizeOf(u16) * max_name_len); From 140ca67ea6d42d635c837558c075d8d9abd4e884 Mon Sep 17 00:00:00 2001 From: r00ster91 Date: Fri, 24 Feb 2023 20:58:36 +0100 Subject: [PATCH 3/3] std.Thread: use pthread_getname_np on musl Starting with version 1.2.3, musl now supports pthread_getname_np: https://github.com/bminor/musl/blob/7a43f6fea9081bdd53d8a11cef9e9fab0348c53d/WHATSNEW#L2293-L2329 --- lib/std/Thread.zig | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/lib/std/Thread.zig b/lib/std/Thread.zig index 795dfb2daa..bbd6862b88 100644 --- a/lib/std/Thread.zig +++ b/lib/std/Thread.zig @@ -179,10 +179,6 @@ pub fn getName(self: Thread, buffer_ptr: *[max_name_len:0]u8) GetNameError!?[]co else => |e| return os.unexpectedErrno(e), } } else { - if (target.abi.isMusl()) { - // musl doesn't provide pthread_getname_np and there's no way to retrieve the thread id of an arbitrary thread. - return error.Unsupported; - } const err = std.c.pthread_getname_np(self.getHandle(), buffer.ptr, max_name_len + 1); switch (err) { .SUCCESS => return std.mem.sliceTo(buffer, 0), @@ -1133,16 +1129,7 @@ test "setName, getName" { error.Unsupported => return error.SkipZigTest, else => return err, }, - else => |tag| if (tag == .linux and use_pthreads and comptime target.abi.isMusl()) { - try thread.setName("foobar"); - - var name_buffer: [max_name_len:0]u8 = undefined; - const res = thread.getName(&name_buffer); - - try std.testing.expectError(error.Unsupported, res); - } else { - try testThreadName(&thread); - }, + else => try testThreadName(&thread), } context.thread_done_event.set();