zig icon indicating copy to clipboard operation
zig copied to clipboard

std.Thread: use dead code

Open wooster0 opened this issue 1 year ago • 7 comments

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.

wooster0 avatar Feb 21 '23 17:02 wooster0

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?

ifreund avatar Feb 21 '23 19:02 ifreund

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.

wooster0 avatar Feb 21 '23 19:02 wooster0

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.

wooster0 avatar Feb 22 '23 09:02 wooster0

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.

wooster0 avatar Feb 22 '23 09:02 wooster0

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.

wooster0 avatar Feb 22 '23 16:02 wooster0

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.

wooster0 avatar Feb 24 '23 19:02 wooster0

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.

wooster0 avatar Feb 24 '23 19:02 wooster0

Thanks @r00ster91!

andrewrk avatar Apr 20 '23 23:04 andrewrk