zig
zig copied to clipboard
std.Thread: use dead code
I noticed this comment saying the intent of the code's author was unclear.
setName
:
prctl
can set or get the name of the "calling thread", i.e. the thread that calls the function, hence we don't need to pass a thread id to it. In the first if
statement they forgot an is_gnu
because that function is actually a GNU extension which allows setting or getting any thread's name. If the thread is the calling thread, it also works and it'll just delegate to prctl
internally, is what I read.
In the else if
below self.getHandle() == std.c.pthread_self()
means "are we the calling thread", and that's when you can call prctl
.
Otherwise, it just does the "/proc/self/task/{d}/comm"
thing in the else
below.
getName
:
It's similar except there was an additional else if (!use_pthreads)
which seemed strange because 1. setName
does not have that either and 2. in all other cases it seems more proper to just do it the "/proc/self/task/{d}/comm"
thing again there. I don't really understand what that "musl doesn't provide pthread_getname_np" is supposed to mean there because we can still do the "/proc/self/task/{d}/comm"
thing because what does that have to do with whether we use musl libc or not? That was my reasoning.
Would you mind copying your wonderful PR description into the body of the commit message so that it can be more easily found with git blame
?
You're right. I usually prefer doing it that way but I thought maybe it would be too long but I guess it's fine.
Just realized that the "setName, getName" test actually expects error.Unsupported
specifically if we're using musl libc (and remember I removed the else
for that).
Again, am I wrong about this? It specifically doesn't expect getName
to succeed if we're using pthreads and are using musl libc, but why? Why would the else
not work even if we're using pthreads? I guess we'll find out when I change the test to expect it to succeed.
Also, looking at the implementation of for example pthread_getname_np
in musl libc (so doesn't that mean it does support it? My source is some patch that adds it from 2021), it seems to pretty much just do what we do in the else
anyway, so I guess I don't even understand what the point of calling these functions/using pthreads is. Anyway, I'm probably just going to change it back to the way it was before but with an explicit isMusl
check.
Ok, I think I got it now. It turned out to be a bit different from initially assumed. This time I've actually tested this locally as well so I am 99% confident the CI will pass.
Sorry, I had put the PR description slightly changed in the commit message but after my later findings it looked like the solution to the problem here was much simpler so I chose a simpler commit description. Anyway, I put back in a new, longer, more descriptive commit description. May this be helpful to any future soul investigating this thread code.
NOTE: instead of reading this wall of text I recommend reading the descriptions of the new 3 commits of this PR
One last thing is that in getName
we have
https://github.com/ziglang/zig/blob/97b9facb98cffa05064315d69a11b96836aa5be3/lib/std/Thread.zig#L172 but in setName
we have https://github.com/ziglang/zig/blob/97b9facb98cffa05064315d69a11b96836aa5be3/lib/std/Thread.zig#L64
We don't have that is_gnu
check in setName
.
The reason we have that in the first place I believe is 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.
So shouldn't we additionally check for is_gnu
in all places where we call pthread_setname_np
or pthread_getname_np
? Or I guess it only applies for Linux?
Honestly, I'd be more inclined to just drop the is_gnu
check. Tests pass for me without the check on both x86_64-linux-musl
and x86_64-linux-gnu
. What do you think?
Also:
https://github.com/ziglang/zig/blob/97b9facb98cffa05064315d69a11b96836aa5be3/lib/std/Thread.zig#L196-L197
musl 1.2.3 now supports pthread_getname_np
:
https://github.com/bminor/musl/blob/7a43f6fea9081bdd53d8a11cef9e9fab0348c53d/WHATSNEW#L2293-L2297
Is it ok to use this function now? musl 1.2.3 is the most recent musl release. Can I or do I have to check the musl version somehow? Is it ok to always assume the newest version? I'm not sure how this works.
Actually, yes, this is perfectly fine because we have musl 1.2.3 in lib/libc/musl.
Thanks @r00ster91!