Skip to content

Commit 8ded0b8

Browse files
authored
Don't mark a thread a joinable when it calls exit() (#15105)
The previous behavior was that `ExitStatus` was thrown which ends up calling `emscripten_thread_exit` and marking the current thread as join-able. This this bug was masking several failures in the posixtest suite which have always be failing but were not being recognized as such. Note that we have to use the message queue rather than `postMessage` to deliver the `exitProcess` message since `postMessage` will not interrupt the main thread if its blocking on `pthread_join`. Modify the existing test so that it fails if `pthead_join` even returns. The tests that were passing before are basically of the form: ``` void thread_func() { ... exit(NON_ZERO); // <- should exit the entire program without waking joining threads } int mai() { pthread_join(thread_func); return 0; // <- join should never return so we should never get here. } ``` Such a program would return 0 without this fix. Fixes: #14993
1 parent f4e9e42 commit 8ded0b8

File tree

11 files changed

+95
-63
lines changed

11 files changed

+95
-63
lines changed

emcc.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1892,6 +1892,9 @@ def default_setting(name, new_default):
18921892
'_pthread_testcancel',
18931893
'_exit',
18941894
]
1895+
settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += [
1896+
'$exitOnMainThread',
1897+
]
18951898
# Some of these symbols are using by worker.js but otherwise unreferenced.
18961899
# Because emitDCEGraph only considered the main js file, and not worker.js
18971900
# we have explicitly mark these symbols as user-exported so that they will

src/library_pthread.js

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,12 @@ var LibraryPThread = {
127127
#endif
128128

129129
terminateAllThreads: function() {
130+
#if ASSERTIONS
131+
assert(!ENVIRONMENT_IS_PTHREAD, 'Internal Error! terminateAllThreads() can only ever be called from main application thread!');
132+
#endif
133+
#if PTHREADS_DEBUG
134+
err('terminateAllThreads');
135+
#endif
130136
for (var t in PThread.pthreads) {
131137
var pthread = PThread.pthreads[t];
132138
if (pthread && pthread.worker) {
@@ -277,20 +283,6 @@ var LibraryPThread = {
277283
assert(detached);
278284
#endif
279285
PThread.returnWorkerToPool(worker);
280-
} else if (cmd === 'exitProcess') {
281-
// A pthread has requested to exit the whole application process (runtime).
282-
#if ASSERTIONS
283-
err("exitProcess requested by worker");
284-
#endif
285-
#if MINIMAL_RUNTIME
286-
_exit(d['returnCode']);
287-
#else
288-
try {
289-
_exit(d['returnCode']);
290-
} catch (e) {
291-
handleException(e);
292-
}
293-
#endif
294286
} else if (cmd === 'cancelDone') {
295287
PThread.returnWorkerToPool(worker);
296288
} else if (d.target === 'setimmediate') {
@@ -460,6 +452,9 @@ var LibraryPThread = {
460452

461453
$killThread__desp: ['$freeThreadData'],
462454
$killThread: function(pthread_ptr) {
455+
#if PTHREADS_DEBUG
456+
err('killThread 0x' + pthread_ptr.toString(16));
457+
#endif
463458
#if ASSERTIONS
464459
assert(!ENVIRONMENT_IS_PTHREAD, 'Internal Error! killThread() can only ever be called from main application thread!');
465460
assert(pthread_ptr, 'Internal Error! Null pthread_ptr in killThread!');
@@ -1193,6 +1188,7 @@ var LibraryPThread = {
11931188
return size <= 4 && (size & (size-1)) == 0 && (ptr&(size-1)) == 0;
11941189
},
11951190

1191+
__call_main__deps: ['exit', '$exitOnMainThread'],
11961192
__call_main: function(argc, argv) {
11971193
var returnCode = {{{ exportedAsmFunc('_main') }}}(argc, argv);
11981194
#if EXIT_RUNTIME
@@ -1201,8 +1197,7 @@ var LibraryPThread = {
12011197
#if PTHREADS_DEBUG
12021198
err('Proxied main thread 0x' + _pthread_self().toString(16) + ' finished with return code ' + returnCode + '. EXIT_RUNTIME=1 set, quitting process.');
12031199
#endif
1204-
postMessage({ 'cmd': 'exitProcess', 'returnCode': returnCode });
1205-
return returnCode;
1200+
exitOnMainThread(returnCode);
12061201
}
12071202
#else
12081203
// EXIT_RUNTIME==0 set on command line, keeping main thread alive.
@@ -1257,6 +1252,28 @@ var LibraryPThread = {
12571252
#endif
12581253
},
12591254

1255+
$exitOnMainThread__deps: ['exit',
1256+
#if !MINIMAL_RUNTIME
1257+
'$handleException',
1258+
#endif
1259+
],
1260+
$exitOnMainThread__proxy: 'async',
1261+
$exitOnMainThread: function(returnCode) {
1262+
// A pthread has requested to exit the whole application process (runtime).
1263+
#if PTHREADS_DEBUG
1264+
err('exitOnMainThread');
1265+
#endif
1266+
#if MINIMAL_RUNTIME
1267+
_exit(returnCode);
1268+
#else
1269+
try {
1270+
_exit(returnCode);
1271+
} catch (e) {
1272+
handleException(e);
1273+
}
1274+
#endif
1275+
},
1276+
12601277
emscripten_proxy_to_main_thread_js__deps: ['emscripten_run_in_main_runtime_thread_js'],
12611278
emscripten_proxy_to_main_thread_js__docs: '/** @type{function(number, (number|boolean), ...(number|boolean))} */',
12621279
emscripten_proxy_to_main_thread_js: function(index, sync) {

src/postamble.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,14 +408,14 @@ function exit(status, implicit) {
408408
if (!implicit) {
409409
if (ENVIRONMENT_IS_PTHREAD) {
410410
#if PTHREADS_DEBUG
411-
err('Pthread 0x' + _pthread_self().toString(16) + ' called exit(), posting exitProcess.');
411+
err('Pthread 0x' + _pthread_self().toString(16) + ' called exit(), posting exitOnMainThread.');
412412
#endif
413413
// When running in a pthread we propagate the exit back to the main thread
414414
// where it can decide if the whole process should be shut down or not.
415415
// The pthread may have decided not to exit its own runtime, for example
416416
// because it runs a main loop, but that doesn't affect the main thread.
417-
postMessage({ 'cmd': 'exitProcess', 'returnCode': status });
418-
throw new ExitStatus(status);
417+
exitOnMainThread(status);
418+
throw 'unwind';
419419
} else {
420420
#if PTHREADS_DEBUG
421421
err('main thread called exit: keepRuntimeAlive=' + keepRuntimeAlive() + ' (counter=' + runtimeKeepaliveCounter + ')');

system/lib/libc/musl/include/unistd.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,11 @@ int eaccess(const char *, int);
256256
#define _POSIX_THREAD_ATTR_STACKADDR _POSIX_VERSION
257257
#define _POSIX_THREAD_ATTR_STACKSIZE _POSIX_VERSION
258258
#define _POSIX_THREAD_PRIORITY_SCHEDULING _POSIX_VERSION
259+
#ifdef __EMSCRIPTEN__
260+
#define _POSIX_THREAD_CPUTIME -1
261+
#else
259262
#define _POSIX_THREAD_CPUTIME _POSIX_VERSION
263+
#endif
260264
#define _POSIX_TIMERS _POSIX_VERSION
261265
#define _POSIX_TIMEOUTS _POSIX_VERSION
262266
#define _POSIX_MONOTONIC_CLOCK _POSIX_VERSION

system/lib/pthread/emscripten_proxy_main.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,13 @@
1414
static int _main_argc;
1515
static char** _main_argv;
1616

17-
extern int __call_main(int argc, char** argv);
17+
extern void __call_main(int argc, char** argv);
1818

1919
static void* _main_thread(void* param) {
2020
// This is the main runtime thread for the application.
2121
emscripten_set_thread_name(pthread_self(), "Application main thread");
22-
return (void*)__call_main(_main_argc, _main_argv);
22+
__call_main(_main_argc, _main_argv);
23+
return NULL;
2324
}
2425

2526
int emscripten_proxy_main(int argc, char** argv) {

tests/browser_reporting.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,15 @@ if (typeof window === 'object' && window) {
5555
}
5656

5757
if (hasModule) {
58-
Module['onExit'] = function(status) {
59-
maybeReportResultToServer('exit:' + status);
58+
if (!Module['onExit']) {
59+
Module['onExit'] = function(status) {
60+
maybeReportResultToServer('exit:' + status);
61+
}
6062
}
6163

62-
Module['onAbort'] = function(reason) {
63-
maybeReportResultToServer('abort:' + reason);
64+
if (!Module['onAbort']) {
65+
Module['onAbort'] = function(reason) {
66+
maybeReportResultToServer('abort:' + reason);
67+
}
6468
}
6569
}
Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
#include <assert.h>
2+
#include <stdatomic.h>
3+
#include <stdbool.h>
24
#include <pthread.h>
35
#include <stdlib.h>
46
#include <stdio.h>
7+
#include <emscripten/emscripten.h>
58

69
pthread_t t;
710

@@ -10,23 +13,24 @@ void* thread_main_exit(void* arg) {
1013
exit(42);
1114
}
1215

16+
// This location should never get set to true.
17+
// We verify that it false from JS after the program exits.
18+
atomic_bool join_returned = false;
19+
20+
EMSCRIPTEN_KEEPALIVE atomic_bool* join_returned_address() {
21+
return &join_returned;
22+
}
23+
1324
int main() {
1425
printf("main\n");
1526
int rc = pthread_create(&t, NULL, thread_main_exit, NULL);
1627
assert(rc == 0);
1728
void* thread_rtn = 0;
1829
rc = pthread_join(t, &thread_rtn);
1930
assert(rc == 0);
20-
#if EXIT_RUNTIME
21-
printf("done join -- should never get here\n");
22-
return 1;
23-
#else
24-
// Since EXIT_RUNTIME is not set the exit() in the thread is not expected to
25-
// bring down the whole process, only itself.
26-
printf("done join -- thread exited with %ld\n", (intptr_t)thread_rtn);
27-
#ifdef REPORT_RESULT
28-
REPORT_RESULT(43);
29-
#endif
30-
return 43;
31-
#endif
31+
// pthread_join should never return becasue the runtime should
32+
// exit first.
33+
join_returned = true;
34+
printf("done join %d -- should never get here\n", rc);
35+
__builtin_trap();
3236
}
Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,17 @@
1-
Module.preRun = function() {
2-
Module['onExit'] = function(status) {
3-
out('onExit status: ' + status);
4-
if (typeof reportResultToServer !== 'undefined') {
5-
reportResultToServer('onExit status: ' + status);
6-
}
7-
};
1+
var address = 0;
2+
3+
Module.onRuntimeInitialized = function() {
4+
address = Module['_join_returned_address']();
5+
assert(address);
6+
assert(HEAP8[address] == 0);
87
}
8+
9+
Module.onExit = function(status) {
10+
out('onExit status: ' + status);
11+
// Verify that the join never returned
12+
assert(address);
13+
assert(HEAP8[address] == 0, 'pthread_join should not have returned!');
14+
if (typeof reportResultToServer !== 'undefined') {
15+
reportResultToServer('onExit status: ' + status);
16+
}
17+
};

tests/test_browser.py

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4146,17 +4146,6 @@ def test_pthread_exit_process(self):
41464146
args += ['--pre-js', test_file('core/pthread/test_pthread_exit_runtime.pre.js')]
41474147
self.btest(test_file('core/pthread/test_pthread_exit_runtime.c'), expected='onExit status: 42', args=args)
41484148

4149-
@requires_threads
4150-
def test_pthread_no_exit_process(self):
4151-
# Same as above but without EXIT_RUNTIME. In this case we don't expect onExit to
4152-
# ever be called.
4153-
args = ['-s', 'USE_PTHREADS',
4154-
'-s', 'PROXY_TO_PTHREAD',
4155-
'-s', 'PTHREAD_POOL_SIZE=2',
4156-
'-O0']
4157-
args += ['--pre-js', test_file('core/pthread/test_pthread_exit_runtime.pre.js')]
4158-
self.btest(test_file('core/pthread/test_pthread_exit_runtime.c'), expected='43', args=args)
4159-
41604149
# Tests MAIN_THREAD_EM_ASM_INT() function call signatures.
41614150
def test_main_thread_em_asm_signatures(self):
41624151
self.btest_exit(test_file('core/test_em_asm_signatures.cpp'), assert_returncode=121, args=[])

tests/test_core.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8382,14 +8382,6 @@ def test_pthread_exit_process(self):
83828382
self.emcc_args += ['-DEXIT_RUNTIME', '--pre-js', test_file('core/pthread/test_pthread_exit_runtime.pre.js')]
83838383
self.do_run_in_out_file_test('core/pthread/test_pthread_exit_runtime.c', assert_returncode=42)
83848384

8385-
@node_pthreads
8386-
@disabled('https://github.com/emscripten-core/emscripten/issues/12945')
8387-
def test_pthread_no_exit_process(self):
8388-
# Same as above but without EXIT_RUNTIME
8389-
self.set_setting('PROXY_TO_PTHREAD')
8390-
self.emcc_args += ['--pre-js', test_file('core/pthread/test_pthread_exit_runtime.pre.js')]
8391-
self.do_run_in_out_file_test('core/pthread/test_pthread_exit_runtime.c', assert_returncode=43)
8392-
83938385
@node_pthreads
83948386
@no_wasm2js('wasm2js does not support PROXY_TO_PTHREAD (custom section support)')
83958387
def test_pthread_offset_converter(self):

tests/test_posixtest.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ def get_pthread_tests():
6868
}
6969

7070
unsupported = {
71+
'test_pthread_attr_setinheritsched_2_2': 'scheduling policy/parameters are not supported',
72+
'test_pthread_attr_setinheritsched_2_3': 'scheduling policy/parameters are not supported',
7173
'test_pthread_attr_setinheritsched_2_3': 'scheduling policy/parameters are not supported',
7274
'test_pthread_attr_setinheritsched_2_4': 'scheduling policy/parameters are not supported',
7375
'test_pthread_attr_setschedparam_1_3': 'scheduling policy/parameters are not supported',
@@ -89,7 +91,10 @@ def get_pthread_tests():
8991
'test_pthread_create_8_1': 'signals are not supported',
9092
'test_pthread_create_8_2': 'signals are not supported',
9193
'test_pthread_create_10_1': 'signals are not supported',
94+
'test_pthread_create_11_1': '_POSIX_THREAD_CPUTIME not supported',
95+
'test_pthread_getcpuclockid_1_1': 'pthread_getcpuclockid not supported',
9296
'test_pthread_getschedparam_1_3': 'scheduling policy/parameters are not supported',
97+
'test_pthread_getschedparam_1_2': 'scheduling policy/parameters are not supported',
9398
'test_pthread_kill_1_2': 'signals are not supported',
9499
'test_pthread_mutexattr_getprioceiling_1_2': 'pthread_mutexattr_setprioceiling is not supported',
95100
'test_pthread_mutexattr_getprotocol_1_2': 'pthread_mutexattr_setprotocol is not supported',
@@ -111,6 +116,10 @@ def get_pthread_tests():
111116
'test_pthread_rwlock_timedwrlock_6_1': 'signals are not supported',
112117
'test_pthread_rwlock_timedwrlock_6_2': 'signals are not supported',
113118
'test_pthread_rwlock_wrlock_2_1': 'signals are not supported',
119+
'test_pthread_setschedparam_1_1': 'scheduling policy/parameters are not supported',
120+
'test_pthread_setschedparam_1_2': 'scheduling policy/parameters are not supported',
121+
'test_pthread_setschedparam_4_1': 'scheduling policy/parameters are not supported',
122+
'test_pthread_setschedprio_1_1': 'scheduling policy/parameters are not supported',
114123
'test_pthread_spin_init_2_1': 'shm_open and shm_unlink are not supported',
115124
'test_pthread_spin_init_2_2': 'shm_open and shm_unlink are not supported',
116125
}

0 commit comments

Comments
 (0)