Use compareExchange to set detach_state in pthread_join_js

[sbc100]

This is a little more robust in the face of racey/invalid code that does
stuff like joining from muliple threads or detaching a thread while it
is being joined.

[kleisauke]

LGTM! I left a small comment inline regarding that assert.

[kleisauke]

Perhaps we should print an error, instead of abort()-ing when ASSERTIONS are enabled?

Suggested change

	#if ASSERTIONS
	assert(old_state === {{{ cDefine('DT_JOINABLE') }}}, 'pthread_join attempted on thread 0x' + thread.toString(16) + ', which is in an invalid state:' + old_state);
	#else
	if (old_state !== {{{ cDefine('DT_JOINABLE') }}}) return {{{ cDefine('EINVAL') }}};
	#endif
	if (old_state !== {{{ cDefine('DT_JOINABLE') }}}) {
	#if ASSERTIONS
	err('pthread_join attempted on thread 0x' + thread.toString(16) + ', which is in an invalid state:' + old_state);
	#endif
	return {{{ cDefine('EINVAL') }}};
	}

[sbc100]

I considered that yes. But then I looked at the existing musl code it just aborts in this case. I think it means your program is invalid if this happens:

See:

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

Line 17 in d90468c

if (state >= DT_DETACHED) a_crash();

[kleisauke]

Ah yes, it is considered to be undefined behavior. I currently patched that with this:

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

Lines 31 to 40 in 3d39427

	// The thread is (already) detached and therefore not joinable.
	// This also handle cases where the thread becomes detached
	// *during* the join.
	if (state >= DT_DETACHED) {
	// Even though the man page says this is undefined
	// behaviour we ave several tests in the posixtest suite
	// that depend on this.
	r = EINVAL;
	break;
	}

in PR #13007, since the POSIX test suite depend on this (__pthread_timedjoin_np is also used by pthread_detach, so detaching an already-detached thread would then also crash).

[sbc100]

Is it enough to check for this on entry to pthread_join? Its seems like that is enough to make the testsuite happy anyway.

[sbc100]

I have a followup patch that looked very much like

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

Lines 31 to 40 in 3d39427

	// The thread is (already) detached and therefore not joinable.
	// This also handle cases where the thread becomes detached
	// *during* the join.
	if (state >= DT_DETACHED) {
	// Even though the man page says this is undefined
	// behaviour we ave several tests in the posixtest suite
	// that depend on this.
	r = EINVAL;
	break;
	}

Sorry my work seems to be overlapping so with things you have already done :-/

[kleisauke]

Is it enough to check for this on entry to pthread_join?

This assertion cannot hurt I think.

Its seems like that is enough to make the testsuite happy anyway.

Ah interesting. Perhaps it was already caught by this check?:

emscripten/system/lib/pthread/pthread_join.c

Lines 23 to 26 in f065db6

	if (state == DT_DETACHED || state == DT_EXITED) {
	// The thread is detached or already joined, and therefore not joinable
	return EINVAL;
	}

Sorry my work seems to be overlapping so with things you have already done :-/

No worries. I'm still a student, so it's a great learning experience anyway.