Use pthread_join from musl #15604

kleisauke · 2021-11-23T12:27:16Z

Split from PR #13007.

Note: this commit is no longer needed once PR emscripten-core#15604 lands.

sbc100 · 2021-11-30T05:29:54Z

system/lib/libc/musl/src/thread/pthread_cancel.c

 hidden long __cancel(), __syscall_cp_asm(), __syscall_cp_c();
+#endif


I don't think this patch should be needed .. declaring extra stuff is petty harmless.

Reverted that patch with commit 73ed3d4 in PR #15603.

BTW, GitHub does not allow me to stack PRs when I use my own fork to base my branch on (instead of pushing the branches to the main repository). Hence, this PR is currently easiest to review per commit.

sbc100 · 2021-11-30T05:31:46Z

system/lib/libc/musl/src/thread/pthread_detach.c

+#ifdef __EMSCRIPTEN__
+	// Attempt to detach a thread which does not point to a valid thread, or
+	// does not exist anymore.
+	if (!t || t->self != t) return ESRCH;


Is the test for NULL here needed somewhere? Most other pthread function don't have this kind of check IIRC.

If it is needed perhaps this can be its own change?

The reason for this change was that the previous JS code did check for this, see e.g:

emscripten/src/library_pthread.js

Lines 906 to 909 in 85d58ff

if (!thread) {

err('pthread_detach attempted on a null thread pointer!');

return {{{ cDefine('ESRCH') }}};

}

I removed these NULL checks with commit 80a3e11 as it does indeed looks redundant.

sbc100 · 2021-11-30T05:32:16Z

system/lib/libc/musl/src/thread/pthread_cond_timedwait.c

@@ -187,10 +187,6 @@ int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restri
 		__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, 0);
 	}

-#ifdef __EMSCRIPTEN__
-	pthread_testcancel();


Is this needed, or just generally part of a cleanup? If it not needed as part of this change can you split it out into its own change?

See #15603 (comment) for the reason for this change, I just moved this to PR #15657.

sbc100 · 2022-01-20T16:37:11Z

I wonder if we could split the testcancel change into its own PR? It seems unrelated to join, and a lot more straight forward, unless I'm missing something?

sbc100

In general I'm excited to see this land, but maybe we can split it one more time

sbc100 · 2022-01-20T16:28:19Z

system/lib/libc/musl/src/thread/pthread_detach.c

-		return ESRCH;
+#ifdef __EMSCRIPTEN__
+	// Attempt to detach a thread which does not point to a valid thread, or
+	// does not exist anymore.


Why not keep the old comment.. its seems more useful in that it says why we are doing it. Is it not true?

The new comment does say why emscripten needs this patch..

The reason for the doc comment change was that the "(already joined)"-sentence wasn't quite right. The check is there only for invalid or non-existent threads (AFAIK).

I reverted it partially with commit 1e1761b, so that it indicates why the patch is needed.

Oh BTW, sometimes there are 2 spaces after a sentence, I've seen that a few times now in comments. Do I have to follow that convention?

But I believe the check for validity is to protect specifically again detaching already joined thread (which the testsuite does IIRC). So its probably worth continuing to mention why we might care specifically about a validity here, even though its undefined behaviour and we technically should not need to care.

Ah, I understand now. Commit 7806443 brings the comment to its original state. I was misinterpreting that comment as "already-detached", which is checked below.

emscripten/system/lib/libc/musl/src/thread/pthread_detach.c

Lines 11 to 12 in 1e1761b

/* If the cas fails, detach state is either already-detached

* or exiting/exited, and pthread_join will trap or cleanup. */

Already joined threads are just threads that are invalid (non-existent).

sbc100 · 2022-01-20T16:33:24Z

tests/pthread/main_thread_join.cpp

+#else
+  // Delay a bit to force the main thread to actually block, as exited
+  // threads can be joined without blocking.
+  emscripten_thread_sleep(1000);


Is this change actually needed? What happens if you don't make this change? Does the test fail?

As far as I could remember, the test did indeed fail, because exited threads can be joined without any blocking now. Let me re-check that.

I believe we should be calling emscripten_check_blocking_allowed even when a thread can be joined without blocking. The idea is to warn folks, even in the cases when they got lucky with the timing.

i.e. the warning/error should still show even when joining already exited threads.

Reverted that with commit 44776e8. PR description has been updated accordingly.

kleisauke · 2022-01-20T17:47:39Z

I wonder if we could split the testcancel change into its own PR? It seems unrelated to join, and a lot more straight forward, unless I'm missing something?

Ah, it looks like it can independently land now. I reverted the pthread_testcancel with commit abdcdcf and will open a new PR with that change.

sbc100

Nice! Thanks for your patience on this one. I'm excited to see this land.

sbc100 · 2022-01-20T18:19:38Z

system/lib/libc/musl/src/thread/pthread_join.c

+	// does not exist anymore.
+	if (t->self != t) return ESRCH;
+	// Thread is attempting to join to itself.  Already detached threads are
+	// handled below by returning EINVAL instead.


Maybe mention that these extra checks are because we want to pass the posixtest suite? IIRC both these cases count as undefined behaviour so are not strictly necessary.

Do you mean the extra t->detach_state != DT_DETACHED check? AFAIK, that was needed for our test_main_pthread_join_detach and test_proxy_pthread_join_detach tests, and not needed for the posix tests.

I could also remove this check and update that test exception by partially reverting commit ca90a4b, if you want.

Lets leave that as a possible followup.

I can confirm that the t->detach_state != DT_DETACHED check is only needed for the other.test_{proxy,main}_pthread_join_detach tests, see for e.g. work-in-progress commit 493f2e5 that updates its test asserts.

Do you want to block is this PR on that change or should we co ahead and land with a TODO here?

I added a TODO with commit 5e65f91, since removing this check would also mean that it causes an EDEADLK error when already-detached threads are attempting to detach itself (as pthread_detach re-uses code from pthread_join).

kleisauke · 2022-01-28T14:41:42Z

I opened issue #16136 to investigate the flakiness of the browser.test_pthread_run_on_main_thread test (which also occurs on this PR).

sbc100

Is the change ready to go?

Regarding the flaky test.. does this change make it more flaky?

sbc100 · 2022-01-28T15:04:49Z

tests/pthread/main_thread_join.cpp

@@ -33,7 +33,7 @@ void *ThreadMain(void *arg) {
  // succeeding.
  while (tries.load() < EXPECTED_TRIES) {}
 #endif
-	pthread_exit((void*)0);
+  pthread_exit((void*)0);


Revert this to keep this PR pure

sbc100 · 2022-01-28T15:06:14Z

system/lib/libc/musl/src/thread/pthread_detach.c

 weak_alias(__pthread_detach, emscripten_builtin_pthread_detach);
+#endif


Revert this file to keep this PR pure. Perhaps we can discuss/followup about using ifdef consistently in musl patches?

5e65f91, I'll prepare a PR that unifies all these ifdef's in musl.

sbc100 · 2022-01-28T15:07:45Z

system/lib/libc/musl/src/thread/pthread_join.c

+	// does not exist anymore.
+	if (t->self != t) return ESRCH;
+	// Thread is attempting to join to itself.  Already detached threads are
+	// handled below by returning EINVAL instead.


Do you want to block is this PR on that change or should we co ahead and land with a TODO here?

kleisauke · 2022-01-28T19:12:41Z

Is the change ready to go?

I think it's ready to land.

Regarding the flaky test.. does this change make it more flaky?

I'm not sure about this, I didn't fully investigate that flakiness. All tests succeeded prior to the rebase (see commit 898c613), let's see if it passes now.

This broke the emscripten-releases roller as well as recent test runs on emscripten CI. I have no idea how it actually passed in #15604. That is rather worrying.

(#16141) This broke the emscripten-releases roller as well as recent test runs on emscripten CI. I have no idea how it actually passed in #15604. That is rather worrying.

kleisauke added a commit to kleisauke/emscripten that referenced this pull request Nov 23, 2021

Fix function signature mismatch

7d7bfe6

Note: this commit is no longer needed once PR emscripten-core#15604 lands.

kleisauke added a commit to kleisauke/emscripten that referenced this pull request Nov 29, 2021

Fix function signature mismatch

1a19bb6

Note: this commit is no longer needed once PR emscripten-core#15604 lands.

kleisauke force-pushed the pthread_testcancel_join-musl branch from 4a99292 to 41d1373 Compare November 29, 2021 14:42

sbc100 reviewed Nov 30, 2021

View reviewed changes

kleisauke force-pushed the pthread_testcancel_join-musl branch from 41d1373 to 80a3e11 Compare November 30, 2021 09:19

kleisauke mentioned this pull request Dec 9, 2021

Use musl implementation of pthread API where possible #15061

Closed

kleisauke force-pushed the pthread_testcancel_join-musl branch from 80a3e11 to 64257a6 Compare January 11, 2022 13:08

kleisauke mentioned this pull request Jan 11, 2022

Move pthread_cancel to native code. NFC. #15603

Merged

kleisauke force-pushed the pthread_testcancel_join-musl branch 2 times, most recently from 6872d66 to fc636df Compare January 12, 2022 11:12

kleisauke force-pushed the pthread_testcancel_join-musl branch from fc636df to 8cf9c8b Compare January 20, 2022 12:37

sbc100 reviewed Jan 20, 2022

View reviewed changes

kleisauke changed the title ~~Use pthread_{testcancel,join} from musl~~ Use pthread_join from musl Jan 20, 2022

sbc100 approved these changes Jan 20, 2022

View reviewed changes

Use pthread_join from musl

5eade80

kleisauke force-pushed the pthread_testcancel_join-musl branch from 898c613 to 5eade80 Compare January 28, 2022 11:37

kleisauke mentioned this pull request Jan 28, 2022

Use pthread_testcancel from musl #16134

Merged

sbc100 reviewed Jan 28, 2022

View reviewed changes

Incorporate review comments

5e65f91

sbc100 enabled auto-merge (squash) January 28, 2022 20:31

sbc100 merged commit 1ac2418 into emscripten-core:main Jan 28, 2022

sbc100 mentioned this pull request Jan 28, 2022

Update expectations for other.test_metadce_minimal_pthreads after #15604 #16141

Merged

kleisauke deleted the pthread_testcancel_join-musl branch January 29, 2022 10:44

		hidden long __cancel(), __syscall_cp_asm(), __syscall_cp_c();
		#endif

	if (!thread) {
	err('pthread_detach attempted on a null thread pointer!');
	return {{{ cDefine('ESRCH') }}};
	}

	/* If the cas fails, detach state is either already-detached
	* or exiting/exited, and pthread_join will trap or cleanup. */

		weak_alias(__pthread_detach, emscripten_builtin_pthread_detach);
		#endif

Use pthread_join from musl #15604

Use pthread_join from musl #15604

Uh oh!

Conversation

kleisauke commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Jan 20, 2022

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Jan 20, 2022

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Jan 28, 2022

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Nov 23, 2021 •

edited

Loading

kleisauke Nov 30, 2021 •

edited

Loading

kleisauke Jan 28, 2022 •

edited

Loading