Move pthread_tryjoin_np to native code. NFC#15458
Conversation
8cafa2e to
a352e35
Compare
a352e35 to
9955b30
Compare
| extern int __pthread_join_js(pthread_t thread, void **retval); | ||
|
|
||
| int __pthread_join(pthread_t thread, void **retval) { | ||
| if (thread->self != thread) { |
There was a problem hiding this comment.
The JS version checked if thread was NULL before doing these, do we not still want that?
There was a problem hiding this comment.
None of the musl entry points to this.. so it seems unnecessary.. calling these functions with NULL in undefined behaviour, I suppose we could at a check in debug builds, but we might also need to then add a debug configuration of libc. I'm tempted to go with musl on this one.
There was a problem hiding this comment.
Fair enough. ASan should catch bugs there anyhow.
| // not exist anymore. | ||
| return ESRCH; | ||
| } | ||
| if (thread->detach_state == DT_DETACHED) { |
There was a problem hiding this comment.
Do we not need to access this atomically? JS seemed to (on detached, which I can't seem to find.) It seems to be defined as "volatile", not atomic, in pthread_impl.h
There was a problem hiding this comment.
Yes, musl is using volatile here for these locations rather than atomics. This is what they do for all their atomics api in src/internal/atomic.h.
I think this pre-check is equivalent to the existing one. We still need to handle the case where the thread becomes detached while are joining it.. and I don't thing the JS code currently does that.. but that is a separate issue I think.
There was a problem hiding this comment.
I changed this to use emscripten_atomic_load_u32 to match the existing JS impl. And I added TODO.
There was a problem hiding this comment.
having made that change I'm not sure its really necessary.
musl code internally seems perfectly happy to read this variable without any special atomic functions:
and:
In these cases I think as long as we don't get tearing then its fine to read out-of-date values (and race with other threads).
see
9955b30 to
5a4604b
Compare
| extern int __pthread_join_js(pthread_t thread, void **retval); | ||
|
|
||
| int __pthread_join(pthread_t thread, void **retval) { | ||
| if (thread->self != thread) { |
There was a problem hiding this comment.
Fair enough. ASan should catch bugs there anyhow.
kleisauke
left a comment
There was a problem hiding this comment.
LGTM! Though, I wonder about that posixtest.test_pthread_detach_1_2 test failure (IIRC, the same issue is also present in #13007 when I moved the whole pthread_join to native code, it was the only test I had to temporarily disable).
5a4604b to
6e2d5d2
Compare
I found and fixed that issue. |
dd29a37 to
25fc1bd
Compare
This matches more closely the musl version.
25fc1bd to
dee8e72
Compare
|
I made a few final changes to this CL (mostly adding the extra |
|
Do you have a diff of those final changes? Adding more commits to PRs instead of constantly rebasing into a single commit would have made that easier to figure out. I do think that is a good model to follow in general as I've said. |
|
Sorry github doesn't keep track of that information very well it seems. I wish there was an easy way in the UI to show the differences. I didn't realized you were suggesting that I avoid squashing in general went we chatting about this on the musl update PR. I can try to move in that directions but it will require some changes to my workflow. I'm very much about keeping each PR in a single commit and using |
|
Ah, I see. Well, I don't want to disrupt your general workflow, in specific PRs where this comes up we can figure something out I guess. For this particular PR, do you recall what those last changes were? |
|
Skimming the code now it looks reasonable, but I didn't read it as thoroughly as before where I compared each line to the previous JS for it etc. |
For this PR, the final change was to re-introduce the third argument to pthread_join_js so the JS could know if was being called from |
|
I see, thanks. That change lgtm. |
This matches more closely the musl version.
This matches more closely the musl version.