Skip to content

Commit e43a870

Browse files
authored
Make longjmp/exceptions helpers thread-safe (#12056)
This adds the thread-local annotation to those globals. Previously they were in JS, which is effectively thread-local, but then we moved them to C which meant they were stored in the same shared memory for all threads. A race could happen if threads (or longjmp) operations happened at just the right time, one writing to those globals and another reading. Also make compiler-rt now build with a multithreaded variation, as the implementation of those globals is in that library. Also add a testcase that runs exceptions on multiple threads. It's not a guaranteed way to notice a race, but it may help, and this is an area we didn't have coverage of. Fixes #12035 This has been a possible race condition for a very long time. I think it went unnoticed because exceptions and longjmp tend to be used for exceptional things, and not constantly being executed. And also until we get wasm exceptions support they are also slow, so people have been trying to avoid them as much as possible anyhow.
1 parent 95358a3 commit e43a870

File tree

7 files changed

+87
-17
lines changed

7 files changed

+87
-17
lines changed

Diff for: system/lib/compiler-rt/extras.c renamed to system/lib/compiler-rt/emscripten_exception_builtins.c

+5-5
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@
55
* found in the LICENSE file.
66
*
77
* Support functions for emscripten setjmp/longjmp and exception handling
8-
* support.
8+
* support. References to the things below are generated in the LLVM backend.
99
* See: https://llvm.org/doxygen/WebAssemblyLowerEmscriptenEHSjLj_8cpp.html
1010
*/
1111

12-
/* References to these globals are generated in the llvm backend so they
13-
* cannot be static */
14-
int __THREW__ = 0;
15-
int __threwValue = 0;
12+
#include <threads.h>
13+
14+
thread_local int __THREW__ = 0;
15+
thread_local int __threwValue = 0;
1616

1717
void setThrew(int threw, int value) {
1818
if (__THREW__ == 0) {

Diff for: system/lib/libc/extras.c

-1
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,3 @@ long* _get_timezone() {
3030

3131
void __lock(void* ptr) {}
3232
void __unlock(void* ptr) {}
33-

Diff for: tests/core/pthread/exceptions.cpp

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// Copyright 2019 The Emscripten Authors. All rights reserved.
2+
// Emscripten is available under two separate licenses, the MIT license and the
3+
// University of Illinois/NCSA Open Source License. Both these licenses can be
4+
// found in the LICENSE file.
5+
6+
#include <stdio.h>
7+
#include <stdlib.h>
8+
#include <pthread.h>
9+
#include <assert.h>
10+
#include <emscripten.h>
11+
12+
#include <atomic>
13+
14+
#define NUM_THREADS 2
15+
#define TOTAL 1000
16+
#define THREAD_ADDS 750
17+
#define MAIN_ADDS 5
18+
19+
static std::atomic<int> sum;
20+
static std::atomic<int> total;
21+
22+
void *ThreadMain(void *arg) {
23+
for (int i = 0; i < TOTAL; i++) {
24+
try {
25+
// Throw two different types, to make sure we check throwing and landing
26+
// pad behavior.
27+
if (i & 3) {
28+
throw 3.14159f;
29+
}
30+
throw i;
31+
} catch (int x) {
32+
total += x;
33+
} catch (float f) {
34+
sum++;
35+
// wait for a change, so we see interleaved processing.
36+
int last = sum.load();
37+
while (sum.load() == last) {}
38+
}
39+
}
40+
pthread_exit((void*)TOTAL);
41+
}
42+
43+
pthread_t thread[NUM_THREADS];
44+
45+
void CreateThread(int i)
46+
{
47+
int rc = pthread_create(&thread[i], nullptr, ThreadMain, (void*)i);
48+
assert(rc == 0);
49+
}
50+
51+
void loop() {
52+
static int main_adds = 0;
53+
int worker_adds = sum.load() - main_adds;
54+
sum++;
55+
main_adds++;
56+
printf("main iter %d : %d\n", main_adds, worker_adds);
57+
if (worker_adds == NUM_THREADS * THREAD_ADDS &&
58+
main_adds >= MAIN_ADDS) {
59+
printf("done: %d.\n", total.load());
60+
emscripten_cancel_main_loop();
61+
exit(0);
62+
}
63+
}
64+
65+
int main() {
66+
// Create initial threads.
67+
for(int i = 0; i < NUM_THREADS; ++i) {
68+
printf("maek\n");
69+
CreateThread(i);
70+
}
71+
72+
emscripten_set_main_loop(loop, 0, 0);
73+
}

Diff for: tests/core/pthread/exceptions.out

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
done: 249000.

Diff for: tests/test_core.py

+6-5
Original file line numberDiff line numberDiff line change
@@ -7750,7 +7750,6 @@ def test_stack_overflow_check(self):
77507750
self.emcc_args += ['-DONE_BIG_STRING']
77517751
self.do_runf(path_from_root('tests', 'stack_overflow.cpp'), 'stack overflow', assert_returncode=NON_ZERO)
77527752

7753-
@unittest.skip('let llvm roll in')
77547753
@node_pthreads
77557754
def test_binaryen_2170_emscripten_atomic_cas_u8(self):
77567755
self.emcc_args += ['-s', 'USE_PTHREADS=1']
@@ -8163,7 +8162,6 @@ def test_fpic_static(self):
81638162
self.emcc_args.append('-fPIC')
81648163
self.do_run_in_out_file_test('tests', 'core', 'test_hello_world.c')
81658164

8166-
@unittest.skip('let llvm roll in')
81678165
@node_pthreads
81688166
def test_pthread_create(self):
81698167
self.set_setting('-lbrowser.js')
@@ -8178,18 +8176,21 @@ def test():
81788176
self.emcc_args += ['-DPOOL']
81798177
test()
81808178

8181-
@unittest.skip('let llvm roll in')
8179+
@node_pthreads
8180+
def test_pthread_exceptions(self):
8181+
self.set_setting('PTHREAD_POOL_SIZE', '2')
8182+
self.emcc_args += ['-fexceptions']
8183+
self.do_run_in_out_file_test('tests', 'core', 'pthread', 'exceptions.cpp')
8184+
81828185
def test_emscripten_atomics_stub(self):
81838186
self.do_run_in_out_file_test('tests', 'core', 'pthread', 'emscripten_atomics.c')
81848187

8185-
@unittest.skip('let llvm roll in')
81868188
@no_asan('incompatibility with atomics')
81878189
@node_pthreads
81888190
def test_emscripten_atomics(self):
81898191
self.set_setting('USE_PTHREADS', '1')
81908192
self.do_run_in_out_file_test('tests', 'core', 'pthread', 'emscripten_atomics.c')
81918193

8192-
@unittest.skip('let llvm roll in')
81938194
@no_asan('incompatibility with atomics')
81948195
@node_pthreads
81958196
def test_emscripten_futexes(self):

Diff for: tests/test_other.py

-4
Original file line numberDiff line numberDiff line change
@@ -2563,7 +2563,6 @@ def test_fs_stream_proto(self):
25632563
out = self.run_js('a.out.js', engine=engine)
25642564
self.assertContained('File size: 724', out)
25652565

2566-
@unittest.skip('let llvm roll in')
25672566
def test_node_emscripten_num_logical_cores(self):
25682567
# Test with node.js that the emscripten_num_logical_cores method is working
25692568
create_test_file('src.cpp', r'''
@@ -7890,7 +7889,6 @@ def test_node_js_run_from_different_directory(self):
78907889
self.assertContained('hello, world!', ret)
78917890

78927891
# Tests that a pthreads + modularize build can be run in node js
7893-
@unittest.skip('let llvm roll in')
78947892
def test_node_js_pthread_module(self):
78957893
# create module loader script
78967894
moduleLoader = 'moduleLoader.js'
@@ -8816,7 +8814,6 @@ def test_asan_no_stack_trace(self):
88168814
def test_asan_pthread_stubs(self):
88178815
self.do_smart_test(path_from_root('tests', 'other', 'test_asan_pthread_stubs.c'), emcc_args=['-fsanitize=address', '-s', 'ALLOW_MEMORY_GROWTH=1'])
88188816

8819-
@unittest.skip('let llvm roll in')
88208817
def test_proxy_to_pthread_stack(self):
88218818
with js_engines_modify([NODE_JS + ['--experimental-wasm-threads', '--experimental-wasm-bulk-memory']]):
88228819
self.do_smart_test(path_from_root('tests', 'other', 'test_proxy_to_pthread_stack.c'),
@@ -9212,7 +9209,6 @@ def test_backwards_deps_in_archive(self):
92129209
self.run_process([EMCC, 'empty.c', '-la', '-L.'])
92139210
self.assertContained('success', self.run_js('a.out.js'))
92149211

9215-
@unittest.skip('let llvm roll in')
92169212
def test_warning_flags(self):
92179213
self.run_process([EMCC, '-c', '-o', 'hello.o', path_from_root('tests', 'hello_world.c')])
92189214
cmd = [EMCC, 'hello.o', '-o', 'a.js', '-g', '--closure', '1']

Diff for: tools/system_libs.py

+2-2
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,7 @@ def get_default_variation(cls, **kwargs):
635635
return super(AsanInstrumentedLibrary, cls).get_default_variation(is_asan=shared.Settings.USE_ASAN, **kwargs)
636636

637637

638-
class libcompiler_rt(Library):
638+
class libcompiler_rt(MTLibrary):
639639
name = 'libcompiler_rt'
640640
# compiler_rt files can't currently be part of LTO although we are hoping to remove this
641641
# restriction soon: https://reviews.llvm.org/D71738
@@ -644,9 +644,9 @@ class libcompiler_rt(Library):
644644
cflags = ['-O2', '-fno-builtin']
645645
src_dir = ['system', 'lib', 'compiler-rt', 'lib', 'builtins']
646646
src_files = glob_in_path(src_dir, '*.c')
647-
src_files.append(shared.path_from_root('system', 'lib', 'compiler-rt', 'extras.c'))
648647
src_files.append(shared.path_from_root('system', 'lib', 'compiler-rt', 'stack_ops.s'))
649648
src_files.append(shared.path_from_root('system', 'lib', 'compiler-rt', 'emscripten_setjmp.c'))
649+
src_files.append(shared.path_from_root('system', 'lib', 'compiler-rt', 'emscripten_exception_builtins.c'))
650650

651651

652652
class libc(AsanInstrumentedLibrary, MuslInternalLibrary, MTLibrary):

0 commit comments

Comments
 (0)