Move pthread_cancel to native code. NFC. #15603

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

Split from PR #13007.

sbc100 · 2021-11-23T16:57:27Z

system/lib/pthread/pthread_create.c

@@ -244,7 +244,7 @@ void _emscripten_thread_exit(void* result) {
  }
 }

-// Mark as `no_sanitize("address"` since emscripten_pthread_exit destroys
+// Mark as `no_sanitize("address")` since emscripten_pthread_exit destroys


Looks like all the changes to this file are just cleanups.. can you split this out?

OK, reverted those with commit 72da048. I'll make a new PR with these changes soon (perhaps with [skip ci] in the commit message, since these are all minor changes?).

sbc100 · 2021-11-24T00:01:47Z

src/worker.js

@@ -280,7 +280,6 @@ self.onmessage = function(e) {
      if (Module['_pthread_self']()) {
        Module['__emscripten_thread_exit'](-1/*PTHREAD_CANCELED*/);
      }
-      postMessage({ 'cmd': 'cancelDone' });


I wonder if the removal on cancelDone can be split out into its own change? It seems like a bugfix its own right as __emscripten_thread_exit should always be enough to recycle the thread. That would be very small bugix PR that would simplify this one.

Reverted with commit 3008b16, moved to PR #15624.

sbc100 · 2021-11-24T00:02:37Z

system/lib/pthread/pthread_testcancel.c

 void __pthread_testcancel() {
  struct pthread* self = pthread_self();
  if (self->canceldisable)
    return;
-  if (_pthread_isduecanceled(self)) {


Can the removal of _pthread_isduecanceled also be split out into its own PR?

Reverted with commit 8db22b9, moved to PR #15625.

kleisauke · 2021-11-24T09:43:14Z

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

-#ifdef __EMSCRIPTEN__
-	pthread_testcancel();
-#endif
-


I removed this since it looked redundant. This was introduced in commit d2bb705 (which was based on musl 1.0.5), but when musl was updated to 1.1.15 (commit 31cf2bb), it ends up calling __pthread_testcancel twice, see:

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

Lines 185 to 192 in 7e6d511

if (e == ECANCELED) {

__pthread_testcancel();

__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, 0);

}

#ifdef __EMSCRIPTEN__

pthread_testcancel();

#endif

If preferred, I can also create a separate PR for this.

Reverted with commit 1146ba5, moved to PR #15657.

kleisauke · 2022-01-11T13:27:13Z

Rebased, PTAL. This would make reviewing #15604 easier.

sbc100

Great! (lets just ifdef out a little more of the unused code.. but otherwise looks great!)

sbc100 · 2022-01-11T14:32:35Z

src/library_pthread.js

@@ -408,6 +408,7 @@ var LibraryPThread = {
  },

  __emscripten_thread_cleanup: function(thread) {
+    // Called when a thread needs to be cleaned up so it can be reused.


Perhaps mention that its called in exactly two places: "When a detached thread exits and when an attached thread is joined"?

Mentioned/elaborated with commit f4058e2.

sbc100 · 2022-01-11T14:38:16Z

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

@@ -66,6 +67,7 @@ static void cancel_handler(int sig, siginfo_t *si, void *ctx)

 	__syscall(SYS_tkill, self->tid, SIGCANCEL);
 }
+#endif

 void __testcancel()


If we are including this function, then perhaps we should also be including system/lib/libc/musl/src/thread/pthread_testcancel.c rather than our custom system/lib/pthread/pthread_testcancel.c?

How about ifdef'ing this function out for now and the adding it in as followup PR which includes system/lib/libc/musl/src/thread/pthread_testcancel.c? (i.e. lets keep this PR small and focused.. but we can remove these unused functions).

Ah you're right, I forgot to adjust the ifdef's when I split PR #15604 (which includes pthread_testcancel from musl rather than using a custom one). Fixed with commit f4058e2.

sbc100 · 2022-01-11T14:38:56Z

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

@@ -14,6 +14,7 @@ long __cancel()
 	return -ECANCELED;
 }

+#ifndef __EMSCRIPTEN__


I think can move this up to line 5, no?

Yep, fixed with commit f4058e2.

sbc100 · 2022-01-11T16:46:04Z

Great!

sbc100 · 2022-01-11T23:45:09Z

Can you rebase/merge?

kleisauke · 2022-01-12T09:59:08Z

Rebased as requested.

kleisauke mentioned this pull request Nov 23, 2021

Use pthread_join from musl #15604

Merged

sbc100 reviewed Nov 23, 2021

View reviewed changes

sbc100 reviewed Nov 24, 2021

View reviewed changes

kleisauke changed the title ~~Move pthread_cancel to native code~~ Move pthread_cancel to native code. NFC. Nov 24, 2021

kleisauke commented Nov 24, 2021

View reviewed changes

This was referenced Nov 24, 2021

Remove redundant cancelDone postMessage and command #15624

Merged

Remove _pthread_isduecanceled in favor of ->cancel. NFC. #15625

Merged

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

Cherry-pick emscripten-core#15603

5a42811

kleisauke mentioned this pull request Nov 30, 2021

Remove redundant call to pthread_testcancel. NFC. #15657

Merged

kleisauke force-pushed the pthread_cancel-native branch from 3008b16 to 1146ba5 Compare November 30, 2021 09:17

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

Cherry-pick emscripten-core#15603

cb33a75

kleisauke force-pushed the pthread_cancel-native branch from 1146ba5 to d30f858 Compare December 3, 2021 09:53

kleisauke mentioned this pull request Dec 9, 2021

Use musl implementation of pthread API where possible #15061

Closed

kleisauke force-pushed the pthread_cancel-native branch from d30f858 to 8c66923 Compare January 11, 2022 13:08

kleisauke added a commit to kleisauke/emscripten that referenced this pull request Jan 11, 2022

Cherry-pick emscripten-core#15603

2290d96

kleisauke force-pushed the pthread_cancel-native branch from 8c66923 to 90ef01f Compare January 11, 2022 13:52

kleisauke added a commit to kleisauke/emscripten that referenced this pull request Jan 11, 2022

Cherry-pick emscripten-core#15603

2d8366c

sbc100 approved these changes Jan 11, 2022

View reviewed changes

sbc100 enabled auto-merge (squash) January 11, 2022 16:45

kleisauke added 3 commits January 12, 2022 10:56

Move pthread_cancel to native code. NFC.

473dbaf

Stray declarations are harmless

47e0a68

Incorporate review comments

0b0d88b

kleisauke force-pushed the pthread_cancel-native branch from f4058e2 to 0b0d88b Compare January 12, 2022 09:57

sbc100 merged commit 3add470 into emscripten-core:main Jan 12, 2022

kleisauke deleted the pthread_cancel-native branch January 12, 2022 11:07

	if (e == ECANCELED) {
	__pthread_testcancel();
	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, 0);
	}

	#ifdef __EMSCRIPTEN__
	pthread_testcancel();
	#endif

Move pthread_cancel to native code. NFC. #15603

Move pthread_cancel to native code. NFC. #15603

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!

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 11, 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!

sbc100 commented Jan 11, 2022

Uh oh!

sbc100 commented Jan 11, 2022

Uh oh!

kleisauke commented Jan 12, 2022

Uh oh!

Uh oh!

kleisauke commented Nov 23, 2021 •

edited

Loading