Simplify thread clean / exit / terminate logic by kleisauke · Pull Request #26857 · emscripten-core/emscripten

kleisauke · 2026-05-05T08:58:26Z

Mostly inspired by musl.

Split out from #13007.

Mostly inspired by musl.

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (2) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_minimal_pthreads.json: 26370 => 26360 [-10 bytes / -0.04%] codesize/test_codesize_minimal_pthreads_memgrowth.json: 26774 => 26764 [-10 bytes / -0.04%] Average change: -0.04% (-0.04% - -0.04%) ```

sbc100 · 2026-05-05T18:36:37Z


 int _emscripten_thread_is_valid(pthread_t thread) {
-  return thread->self == thread;
+  return thread->tid;


This seems more likely to return false positives if called on, e.g. an uninitialized region of memory?

This would align with musl, which uses the ->tid field to determine whether a thread is valid/available.

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

Lines 110 to 117 in 4ceac4e

/* After the kernel thread exits, its tid may be reused. Clear it

* to prevent inadvertent use and inform functions that would use

* it that it's no longer available. At this point the killlock

* may be released, since functions that use it will consistently

* see the thread as having exited. Release it now so that no

* remaining locks (except thread list) are held if we end up

* resetting need_locks below. */

self->tid = 0;

https://github.com/search?q=repo%3Aemscripten-core%2Fmusl+-%3Etid+ESRCH&type=code

In any case, passing an uninitialized pthread_t to functions relying on _emscripten_thread_is_valid() is already undefined behavior.

Hmm, isn't the whole point of _emscripten_thread_is_valid to detect invalid/uninitialized pointers to pthreads?

I think it only makes sense to catch threads that have already terminated/joined. This also aligns with glibc (according to https://stackoverflow.com/a/78311551).

Commit 16c33d2 inlines _emscripten_thread_is_valid() to check for t->tid instead.

I see... but does isn't the memory pointed to by already terminated/joined memory that has been free'd? I.e. it contains basically undefined contents doesn;'t it?

Oh, I think I understand what you mean. In this code:

emscripten/system/lib/pthread/pthread_create.c

Lines 289 to 298 in 16c33d2

// Free the entire thread block (called map_base because

// musl normally allocates this using mmap). This region

// includes the pthread structure itself.

unsigned char* block = t->map_base;

dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);

#ifndef NDEBUG

// To aid in debugging, set the entire region to zero.

memset(block, 0, sizeof(struct pthread));

#endif

emscripten_builtin_free(block);

->tid can end up in an undefined (dangling pointer) state because the entire pthread structure is free()'d. IIUC, this was already an issue before as well, since accessing ->self is also undefined behavior.

In any case, calling pthread_detach() or pthread_join() on a thread that has already been successfully joined is already UB, so it's unclear how much value there is in trying to make this logic "smarter".

Indeed, I think I just wanted to make it smart enough to satisfy the posixtest suite. In practice its undefined behaviour I think.

sbc100 · 2026-05-05T18:40:26Z

  unsigned char* block = t->map_base;
  dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
-  // To aid in debugging, set the entire region to zero.
-  memset(block, 0, sizeof(struct pthread));


Is it not useful for debugging to reset this memory to something ? Maybe 0xfe ? Maybe only in debug builds?

Commit e206e45 does this only for debug builds.

sbc100 · 2026-05-05T18:46:43Z


-  ret = pthread_join(self, NULL);
-  printf("pthread_join -> %s\n", strerror(ret));
-  assert(ret == EINVAL);


Do you know why we were testing this behaviour? Is this something that glibc does? Or maybe that the posixtests expect?

It was only needed for this test, according to this TODO comment:

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

Lines 18 to 20 in 4ceac4e

// TODO: The detached check here is just to satisfy the

// `other.test_{proxy,main}_pthread_join_detach` tests.

if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;

AFAIK, the posixtests didn't require this. I'm not entirely sure why we were testing this, I'll investigate.

sbc100 · 2026-05-07T05:46:35Z

-	// `other.test_{proxy,main}_pthread_join_detach` tests.
-	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;
+	// XXX Emscripten: Add check for invalid (already joined) thread.
+	if (!t->tid) return ESRCH;


I kind of like the explicitness of having a _emscripten_thread_is_valid function.

Re-added with commit b19bce9.

sbc100 · 2026-05-07T05:48:27Z

+  // The pthread struct has a field that points to itself.
  new->self = new;
+
+  // Thread ID, this becomes zero when the thread is no longer available.


.. is no longer valid? Or .. is destroyed?

This comment was based on:

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

Lines 110 to 117 in d137bef

/* After the kernel thread exits, its tid may be reused. Clear it

* to prevent inadvertent use and inform functions that would use

* it that it's no longer available. At this point the killlock

* may be released, since functions that use it will consistently

* see the thread as having exited. Release it now so that no

* remaining locks (except thread list) are held if we end up

* resetting need_locks below. */

self->tid = 0;

sbc100 · 2026-05-07T05:49:02Z

 #endif
+  // The tid may be reused. Clear it to prevent inadvertent use
+  // and inform functions that would use it that it's no longer
+  // available.


/available/valid/?

Same as above, this comment was synced with musl.

sbc100 · 2026-05-07T05:50:57Z

  dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
+#ifndef NDEBUG
  // To aid in debugging, set the entire region to zero.
  memset(block, 0, sizeof(struct pthread));


Should we actually use 0xdd (or some other recognizable value) rather than zero?

I'm not sure, but I think that would break the ->tid check in _emscripten_thread_is_valid().

sbc100 · 2026-05-07T05:53:44Z

+	if (!t->tid) return ESRCH;
+
+	// Note that we don't use pthread_join here, to avoid
+	// returning EDEADLK when attempting to detach itself.


If we had a self-check here could we share more of the code with musl?

Reverted this with commit b19bce9.

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (7) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_ctors1.json: 151887 => 151887 [+0 bytes / +0.00%] codesize/test_codesize_cxx_ctors2.json: 151292 => 151290 [-2 bytes / -0.00%] codesize/test_codesize_cxx_except_wasm.json: 167018 => 167018 [+0 bytes / +0.00%] codesize/test_codesize_cxx_lto.json: 120793 => 120793 [+0 bytes / +0.00%] codesize/test_codesize_cxx_noexcept.json: 153917 => 153917 [+0 bytes / +0.00%] codesize/test_codesize_cxx_wasmfs.json: 179773 => 179773 [+0 bytes / +0.00%] codesize/test_codesize_hello_dylink.json: 43922 => 43922 [+0 bytes / +0.00%] Average change: -0.00% (-0.00% - +0.00%) ```

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (2) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_minimal_pthreads.json: 26409 => 26427 [+18 bytes / +0.07%] codesize/test_codesize_minimal_pthreads_memgrowth.json: 26819 => 26837 [+18 bytes / +0.07%] Average change: +0.07% (+0.07% - +0.07%) ```

…leanup()`

This reverts commit a919f40.

sbc100 · 2026-05-09T20:23:18Z

-	if (!_emscripten_thread_is_valid(t))
-		return ESRCH;
+	if (!_emscripten_thread_is_valid(t)) return ESRCH;
 #endif


Seems like you may as well just revert this file now? Unless you think the formatting is important here?

sbc100 · 2026-05-09T20:24:26Z


 int _emscripten_thread_is_valid(pthread_t thread) {
-  return thread->self == thread;
+  return thread->tid;


I still don't understand why would want to change this. The old code will catch more cases of invalid thread data won't it?

sbc100 · 2026-05-09T20:25:08Z

+  // The tid may be reused. Clear it to prevent inadvertent use
+  // and inform functions that would use it that it's no longer
+  // available.
+  t->tid = 0;


t->self = NULL too and revert _emscripten_thread_is_valid?

sbc100 · 2026-05-09T20:28:21Z


-  if (!--libc.threads_minus_1) libc.need_locks = 0;
+  __do_orphaned_stdio_locks();
+  __dl_thread_cleanup();


It seems like added these two function, and moving the emscripten_is_main_runtime_thread up are the two substantive remaining change.

Can you explain why each of those is good thing?

kleisauke added 2 commits May 5, 2026 10:52

Simplify thread clean / exit / terminate logic. NFC

d7d8a61

Mostly inspired by musl.

sbc100 reviewed May 5, 2026

View reviewed changes

kleisauke added 2 commits May 6, 2026 09:37

Incorporate feedback

e206e45

Merge branch 'main' into simplify-thread-exit

2dcae94

kleisauke changed the title ~~Simplify thread clean / exit / terminate logic. NFC~~ Simplify thread clean / exit / terminate logic May 6, 2026

kleisauke added 2 commits May 6, 2026 19:12

Inline _emscripten_thread_is_valid()

16c33d2

Sync comment with pthread_detach()

f2f683e

sbc100 reviewed May 7, 2026

View reviewed changes

kleisauke added 10 commits May 7, 2026 10:18

Merge branch 'main' into simplify-thread-exit

957677d

Sync comments with musl

965108b

Reduce size of pthread structure

a919f40

Feedback

b19bce9

Sync _emscripten_thread_exit() with musl's pthread_exit

5df7db5

Add missing calls to __do_orphaned_stdio_locks() and `__dl_thread_c…

8cb20c1

…leanup()`

Revert "Reduce size of pthread structure"

fc7cf49

This reverts commit a919f40.

Merge branch 'main' into simplify-thread-exit

3008280

kleisauke mentioned this pull request May 9, 2026

Remove unused field from pthread struct. NFC #26900

Open

Simplify

724b8be

sbc100 reviewed May 9, 2026

View reviewed changes

	/* After the kernel thread exits, its tid may be reused. Clear it
	* to prevent inadvertent use and inform functions that would use
	* it that it's no longer available. At this point the killlock
	* may be released, since functions that use it will consistently
	* see the thread as having exited. Release it now so that no
	* remaining locks (except thread list) are held if we end up
	* resetting need_locks below. */
	self->tid = 0;

	// Free the entire thread block (called map_base because
	// musl normally allocates this using mmap). This region
	// includes the pthread structure itself.
	unsigned char* block = t->map_base;
	dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
	#ifndef NDEBUG
	// To aid in debugging, set the entire region to zero.
	memset(block, 0, sizeof(struct pthread));
	#endif
	emscripten_builtin_free(block);

	// TODO: The detached check here is just to satisfy the
	// `other.test_{proxy,main}_pthread_join_detach` tests.
	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;

Conversation

kleisauke commented May 5, 2026

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 May 6, 2026 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kleisauke May 6, 2026 •

edited

Loading