Skip to content

Use compareExchange to set detach_state in pthread_join_js #15481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbc100 merged 1 commit into from
Nov 11, 2021
Merged

Use compareExchange to set detach_state in pthread_join_js #15481

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 21 additions & 26 deletions src/library_pthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,26 +125,6 @@ var LibraryPThread = {
},
#endif

#if PTHREADS_DEBUG
detachStateToString: function(state) {
if (state === {{{ cDefine('DT_EXITED') }}}) return 'DT_EXITED';
if (state === {{{ cDefine('DT_EXITING') }}}) return 'DT_EXITING';
if (state === {{{ cDefine('DT_JOINABLE') }}}) return 'DT_JOINABLE';
if (state === {{{ cDefine('DT_DETACHED') }}}) return 'DT_DETACHED';
assert(false);
},
#endif

setDetachState: function(thread, newstate) {
#if PTHREADS_DEBUG
var oldstate = Atomics.load(HEAPU32, (thread + {{{ C_STRUCTS.pthread.detach_state }}}) >> 2);
var oldname = PThread.detachStateToString(oldstate);
var newname = PThread.detachStateToString(newstate);
console.log('thread 0x' + thread.toString(16) + ' state change: ' + oldname + ' -> ' + newname);
#endif
Atomics.store(HEAPU32, (thread + {{{ C_STRUCTS.pthread.detach_state }}}) >> 2, newstate);
},

terminateAllThreads: function() {
#if ASSERTIONS
assert(!ENVIRONMENT_IS_PTHREAD, 'Internal Error! terminateAllThreads() can only ever be called from main application thread!');
Expand Down Expand Up @@ -598,7 +578,7 @@ var LibraryPThread = {
var tis = pthread.threadInfoStruct >> 2;
// spawnThread is always called with a zero-initialized thread struct so
// no need to set any valudes to zero here.
PThread.setDetachState(pthread.threadInfoStruct, threadParams.initialState);
Atomics.store(HEAPU32, tis + ({{{ C_STRUCTS.pthread.detach_state }}} >> 2), threadParams.initialState);
Atomics.store(HEAPU32, tis + ({{{ C_STRUCTS.pthread.stack_size }}} >> 2), threadParams.stackSize);
Atomics.store(HEAPU32, tis + ({{{ C_STRUCTS.pthread.stack }}} >> 2), stackHigh);

Expand Down Expand Up @@ -892,24 +872,39 @@ var LibraryPThread = {
#endif

for (;;) {
var detach_state = Atomics.load(HEAPU32, (thread + {{{ C_STRUCTS.pthread.detach_state }}} ) >> 2);
if (detach_state == {{{ cDefine('DT_EXITING') }}}) { // Exiting?
// The thread we are joining with must be either DT_JOINABLE or
// DT_EXITING. If its DT_EXITING then we move it to DT_EXITED and
// we are done. If its DT_JOINABLE we keep waiting.
var old_state = Atomics.compareExchange(HEAP32,
(thread + {{{ C_STRUCTS.pthread.detach_state }}}) >> 2,
{{{ cDefine('DT_EXITING') }}},
{{{ cDefine('DT_EXITED') }}}
);
if (old_state == {{{ cDefine('DT_EXITING') }}}) {
#if PTHREADS_DEBUG
err('thread 0x' + thread.toString(16) + ' successfully joined');
#endif
// We successfully marked the tread as DT_EXITED
if (status) {
var result = Atomics.load(HEAPU32, (thread + {{{ C_STRUCTS.pthread.result }}} ) >> 2);
{{{ makeSetValue('status', 0, 'result', 'i32') }}};
}
// Mark the thread as exited.
PThread.setDetachState(thread, {{{ cDefine('DT_EXITED') }}});
if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread);
else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
return 0;
}
#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
Comment on lines +896 to +900
Copy link
Copy Markdown
Collaborator

@kleisauke kleisauke Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') }}};
}

Copy link
Copy Markdown
Collaborator Author

@sbc100 sbc100 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Collaborator

@kleisauke kleisauke Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator Author

@sbc100 sbc100 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@sbc100 sbc100 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-/

Copy link
Copy Markdown
Collaborator

@kleisauke kleisauke Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


_pthread_testcancel();
// In main runtime thread (the thread that initialized the Emscripten C
// runtime and launched main()), assist pthreads in performing operations
// that they need to access the Emscripten main runtime for.
if (!ENVIRONMENT_IS_PTHREAD) _emscripten_main_thread_process_queued_calls();
_emscripten_futex_wait(thread + {{{ C_STRUCTS.pthread.detach_state }}}, detach_state, ENVIRONMENT_IS_PTHREAD ? 100 : 1);
_emscripten_futex_wait(thread + {{{ C_STRUCTS.pthread.detach_state }}}, old_state, ENVIRONMENT_IS_PTHREAD ? 100 : 1);
}
},

Expand Down