From 561742160beb04dd648a8a02a1ecc851b623b643 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 16 Sep 2020 21:02:09 -0700 Subject: [PATCH 01/25] Add a testcase for #12243 #12244 #12245 [ci skip] --- tests/pthread/test_pthread_proxy_hammer.cpp | 71 +++++++++++++++++++++ tests/test_browser.py | 4 ++ 2 files changed, 75 insertions(+) create mode 100644 tests/pthread/test_pthread_proxy_hammer.cpp diff --git a/tests/pthread/test_pthread_proxy_hammer.cpp b/tests/pthread/test_pthread_proxy_hammer.cpp new file mode 100644 index 0000000000000..a281e76c11fce --- /dev/null +++ b/tests/pthread/test_pthread_proxy_hammer.cpp @@ -0,0 +1,71 @@ +#include +#include +#include +#include +#include +#include + +class random_device +{ + int __f_; +public: + // constructors + explicit random_device(); + ~random_device(); + + // generating functions + unsigned operator()(); +}; + +random_device::random_device() + : __f_(open("/dev/urandom", O_RDONLY)) +{ + if (__f_ < 0) abort(); +} + +random_device::~random_device() +{ + close(__f_); +} + +unsigned +random_device::operator()() +{ + unsigned r; + size_t n = sizeof(r); + char* p = reinterpret_cast(&r); + while (n > 0) + { + ssize_t s = read(__f_, p, 1); + if (s == 0) abort(); + if (s == -1) + { + if (errno != EINTR) abort(); + continue; + } + n -= static_cast(s); + p += static_cast(s); + } + return r; +} + +void* ptr; + +int main() { + int total = 0; + for (int i = 0; i < 128; i++) { + EM_ASM({ out($0, $1) }, i, total); + for (int j = 0; j < 128; j++) { + auto* rd = new random_device; + total += (*rd)(); + ptr = (void*)rd; // make sure the optimizer doesn't remove the allocation + delete rd; + } + } + EM_ASM({ out("done") }); +#ifdef REPORT_RESULT + REPORT_RESULT(0); +#endif + return 0; +} + diff --git a/tests/test_browser.py b/tests/test_browser.py index 39dda6c8807e6..d655a1658ea3b 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4509,6 +4509,10 @@ def run(emcc_args=[]): def test_pthread_reltime(self): self.btest(path_from_root('tests', 'pthread', 'test_pthread_reltime.cpp'), expected='3', args=['-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1']) + @requires_threads + def test_pthread_proxy_hammer(self): + self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD']) + # Tests that it is possible to load the main .js file of the application manually via a Blob URL, and still use pthreads. @requires_threads @no_wasm_backend("WASM2JS does not yet support pthreads") From 2f1819936c7cb0b671b37676f121f263952b266e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 17 Sep 2020 08:26:02 -0700 Subject: [PATCH 02/25] better --- tests/pthread/test_pthread_proxy_hammer.cpp | 24 ++++++++++++--------- tests/runner.py | 20 ++++++++--------- tests/test_browser.py | 19 ++++++++++++---- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/tests/pthread/test_pthread_proxy_hammer.cpp b/tests/pthread/test_pthread_proxy_hammer.cpp index a281e76c11fce..16cc5be82562d 100644 --- a/tests/pthread/test_pthread_proxy_hammer.cpp +++ b/tests/pthread/test_pthread_proxy_hammer.cpp @@ -18,8 +18,8 @@ class random_device }; random_device::random_device() - : __f_(open("/dev/urandom", O_RDONLY)) { + __f_ = open("/dev/urandom", O_RDONLY); if (__f_ < 0) abort(); } @@ -49,20 +49,24 @@ random_device::operator()() return r; } -void* ptr; - int main() { int total = 0; - for (int i = 0; i < 128; i++) { - EM_ASM({ out($0, $1) }, i, total); - for (int j = 0; j < 128; j++) { - auto* rd = new random_device; - total += (*rd)(); - ptr = (void*)rd; // make sure the optimizer doesn't remove the allocation + for (int i = 0; i < 1024; i++) { + // printf causes proxying + printf("%d %d\n", i, total); + for (int j = 0; j < 1024; j++) { + // allocation uses a mutex + auto* rd = new random_device(); + // reading data causes proxying + for (int k = 0; k < 4; k++) { + total += (*rd)(); + } + // make sure the optimizer doesn't remove the allocation + EM_ASM({ out("iter") }, rd); delete rd; } } - EM_ASM({ out("done") }); + printf("done: %d", total); #ifdef REPORT_RESULT REPORT_RESULT(0); #endif diff --git a/tests/runner.py b/tests/runner.py index 62ef202cbc78e..349237969e983 100755 --- a/tests/runner.py +++ b/tests/runner.py @@ -1364,12 +1364,12 @@ def assert_out_queue_empty(self, who): self.harness_out_queue.get() raise Exception('excessive responses from %s' % who) - # @param tries_left: how many more times to try this test, if it fails. browser tests have - # many more causes of flakiness (in particular, they do not run - # synchronously, so we have a timeout, which can be hit if the VM - # we run on stalls temporarily), so we let each test try more than - # once by default - def run_browser(self, html_file, message, expectedResult=None, timeout=None, tries_left=1): + # @param extra_tries: how many more times to try this test, if it fails. browser tests have + # many more causes of flakiness (in particular, they do not run + # synchronously, so we have a timeout, which can be hit if the VM + # we run on stalls temporarily), so we let each test try more than + # once by default + def run_browser(self, html_file, message, expectedResult=None, timeout=None, extra_tries=1): if not has_browser(): return if BrowserCore.unresponsive_tests >= BrowserCore.MAX_UNRESPONSIVE_TESTS: @@ -1408,10 +1408,10 @@ def run_browser(self, html_file, message, expectedResult=None, timeout=None, tri try: self.assertIdenticalUrlEncoded(expectedResult, output) except Exception as e: - if tries_left > 0: + if extra_tries > 0: print('[test error (see below), automatically retrying]') print(e) - return self.run_browser(html_file, message, expectedResult, timeout, tries_left - 1) + return self.run_browser(html_file, message, expectedResult, timeout, extra_tries - 1) else: raise e finally: @@ -1549,7 +1549,7 @@ def btest(self, filename, expected=None, reference=None, force_c=False, reference_slack=0, manual_reference=False, post_build=None, args=[], outfile='test.html', message='.', also_proxied=False, url_suffix='', timeout=None, also_asmjs=False, - manually_trigger_reftest=False): + manually_trigger_reftest=False, extra_tries=1): assert expected or reference, 'a btest must either expect an output, or have a reference image' # if we are provided the source and not a path, use that filename_is_src = '\n' in filename @@ -1583,7 +1583,7 @@ def btest(self, filename, expected=None, reference=None, force_c=False, post_build() if not isinstance(expected, list): expected = [expected] - self.run_browser(outfile + url_suffix, message, ['/report_result?' + e for e in expected], timeout=timeout) + self.run_browser(outfile + url_suffix, message, ['/report_result?' + e for e in expected], timeout=timeout, extra_tries=extra_tries) # Tests can opt into being run under asmjs as well if 'WASM=0' not in args and (also_asmjs or self.also_asmjs): diff --git a/tests/test_browser.py b/tests/test_browser.py index d655a1658ea3b..059a1a0165b71 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4509,10 +4509,6 @@ def run(emcc_args=[]): def test_pthread_reltime(self): self.btest(path_from_root('tests', 'pthread', 'test_pthread_reltime.cpp'), expected='3', args=['-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1']) - @requires_threads - def test_pthread_proxy_hammer(self): - self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD']) - # Tests that it is possible to load the main .js file of the application manually via a Blob URL, and still use pthreads. @requires_threads @no_wasm_backend("WASM2JS does not yet support pthreads") @@ -4915,3 +4911,18 @@ def test_zzz_zzz_4GB_fail(self): # 4GB. self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) + + #@unittest.skip("only run this manually, to test for race conditions") + @requires_threads + def test_manual_pthread_proxy_hammer(self): + # don't run this with the default extra_tries value, as this is *meant* to + # notice flakes. that is, this looks for a race condition. + # the specific symptom of the hangs that were fixed is that the test hangs + # at some point, using 0% CPU. often that occured in 0-200 iterations out + # of the 1024 the test runs by default, but this can vary by machine and + # browser... + self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), + expected='0', + args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD'], + timeout=10000, + extra_tries=0) From 68874d223c8021827172325ab0800ac983caf6c8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 17 Sep 2020 08:26:34 -0700 Subject: [PATCH 03/25] fix --- tests/test_browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index 059a1a0165b71..3f3fd2b39f663 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4912,7 +4912,7 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) - #@unittest.skip("only run this manually, to test for race conditions") + @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): # don't run this with the default extra_tries value, as this is *meant* to From 36be7cd323e41217ee7d58b8873c512fee9c3c9b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 17 Sep 2020 08:27:26 -0700 Subject: [PATCH 04/25] more --- tests/test_browser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index 3f3fd2b39f663..f3504e96648b2 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4915,8 +4915,6 @@ def test_zzz_zzz_4GB_fail(self): @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): - # don't run this with the default extra_tries value, as this is *meant* to - # notice flakes. that is, this looks for a race condition. # the specific symptom of the hangs that were fixed is that the test hangs # at some point, using 0% CPU. often that occured in 0-200 iterations out # of the 1024 the test runs by default, but this can vary by machine and @@ -4925,4 +4923,6 @@ def test_manual_pthread_proxy_hammer(self): expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD'], timeout=10000, + # don't run this with the default extra_tries value, as this is + # *meant* to notice something random, a race condition. extra_tries=0) From 534a2082edff7a27ab770c672b2a68816e29ec12 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:07:34 -0700 Subject: [PATCH 05/25] fixes --- src/library_pthread.js | 33 +++++++++++++-------- src/worker.js | 1 + tests/pthread/test_pthread_proxy_hammer.cpp | 2 +- tests/test_browser.py | 12 ++++---- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index d89e9f3faef64..7e36e91f3574b 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -9,7 +9,6 @@ var LibraryPThread = { $PThread__deps: ['$registerPthreadPtr', '$ERRNO_CODES', 'emscripten_futex_wake', '$killThread', '$cancelThread', '$cleanupThread', - '_main_thread_futex_wait_address' #if USE_ASAN || USE_LSAN , '$withBuiltinMalloc' #endif @@ -28,6 +27,9 @@ var LibraryPThread = { runningWorkers: [], // Points to a pthread_t structure in the Emscripten main heap, allocated on demand if/when first needed. // mainThreadBlock: undefined, + // Stores the memory address that the main thread is waiting on, if any. If + // the main thread is waiting, we wake it up before waking up any workers. + // mainThreadFutex: undefined, initMainThreadBlock: function() { #if ASSERTIONS assert(!ENVIRONMENT_IS_PTHREAD); @@ -69,7 +71,7 @@ var LibraryPThread = { Atomics.store(HEAPU32, (PThread.mainThreadBlock + {{{ C_STRUCTS.pthread.tid }}} ) >> 2, PThread.mainThreadBlock); // Main thread ID. Atomics.store(HEAPU32, (PThread.mainThreadBlock + {{{ C_STRUCTS.pthread.pid }}} ) >> 2, {{{ PROCINFO.pid }}}); // Process ID. - __main_thread_futex_wait_address = _malloc(4); + PThread.mainThreadFutex = _malloc(4); #if PTHREADS_PROFILING PThread.createProfilerBlock(PThread.mainThreadBlock); @@ -87,6 +89,8 @@ var LibraryPThread = { _emscripten_register_main_browser_thread_id(PThread.mainThreadBlock); }, initWorker: function() { + PThread.mainThreadFutex = Module['mainThreadFutex']; + #if USE_CLOSURE_COMPILER // worker.js is not compiled together with us, and must access certain // things. @@ -394,6 +398,7 @@ var LibraryPThread = { // object in Module['mainScriptUrlOrBlob'], or a URL to it, so that pthread Workers can // independently load up the same main application file. 'urlOrBlob': Module['mainScriptUrlOrBlob'] || _scriptDir, + 'mainThreadFutex': PThread.mainThreadFutex, #if WASM2JS // the polyfill WebAssembly.Memory instance has function properties, // which will fail in postMessage, so just send a custom object with the @@ -1106,11 +1111,8 @@ var LibraryPThread = { return 0; }, - // Stores the memory address that the main thread is waiting on, if any. - _main_thread_futex_wait_address: '0', - // Returns 0 on success, or one of the values -ETIMEDOUT, -EWOULDBLOCK or -EINVAL on error. - emscripten_futex_wait__deps: ['_main_thread_futex_wait_address', 'emscripten_main_thread_process_queued_calls'], + emscripten_futex_wait__deps: ['emscripten_main_thread_process_queued_calls'], emscripten_futex_wait: function(addr, val, timeout) { if (addr <= 0 || addr > HEAP8.length || addr&3 != 0) return -{{{ cDefine('EINVAL') }}}; if (ENVIRONMENT_IS_NODE || ENVIRONMENT_IS_WORKER) { @@ -1136,10 +1138,13 @@ var LibraryPThread = { #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); #endif - // Register globally which address the main thread is simulating to be waiting on. When zero, main thread is not waiting on anything, - // and on nonzero, the contents of address pointed by __main_thread_futex_wait_address tell which address the main thread is simulating its wait on. - Atomics.store(HEAP32, __main_thread_futex_wait_address >> 2, addr); + // and on nonzero, the contents of address pointed by PThread.mainThreadFutex tell which address the main thread is simulating its wait on. + var old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, addr); +#if ASSERTIONS + assert(PThread.mainThreadFutex > 0); + assert(old == 0); // we must not have been waiting before. +#endif var ourWaitAddress = addr; // We may recursively re-enter this function while processing queued calls, in which case we'll do a spurious wakeup of the older wait operation. while (addr == ourWaitAddress) { tNow = performance.now(); @@ -1150,7 +1155,7 @@ var LibraryPThread = { return -{{{ cDefine('ETIMEDOUT') }}}; } _emscripten_main_thread_process_queued_calls(); // We are performing a blocking loop here, so must pump any pthreads if they want to perform operations that are proxied. - addr = Atomics.load(HEAP32, __main_thread_futex_wait_address >> 2); // Look for a worker thread waking us up. + addr = Atomics.load(HEAP32, PThread.mainThreadFutex >> 2); // Look for a worker thread waking us up. } #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); @@ -1161,7 +1166,6 @@ var LibraryPThread = { // Returns the number of threads (>= 0) woken up, or the value -EINVAL on error. // Pass count == INT_MAX to wake up all threads. - emscripten_futex_wake__deps: ['_main_thread_futex_wait_address'], emscripten_futex_wake: function(addr, count) { if (addr <= 0 || addr > HEAP8.length || addr&3 != 0 || count < 0) return -{{{ cDefine('EINVAL') }}}; if (count == 0) return 0; @@ -1172,10 +1176,13 @@ var LibraryPThread = { // See if main thread is waiting on this address? If so, wake it up by resetting its wake location to zero. // Note that this is not a fair procedure, since we always wake main thread first before any workers, so // this scheme does not adhere to real queue-based waiting. - var mainThreadWaitAddress = Atomics.load(HEAP32, __main_thread_futex_wait_address >> 2); +#if ASSERTIONS + assert(PThread.mainThreadFutex > 0); +#endif + var mainThreadWaitAddress = Atomics.load(HEAP32, PThread.mainThreadFutex >> 2); var mainThreadWoken = 0; if (mainThreadWaitAddress == addr) { - var loadedAddr = Atomics.compareExchange(HEAP32, __main_thread_futex_wait_address >> 2, mainThreadWaitAddress, 0); + var loadedAddr = Atomics.compareExchange(HEAP32, PThread.mainThreadFutex >> 2, mainThreadWaitAddress, 0); if (loadedAddr == mainThreadWaitAddress) { --count; mainThreadWoken = 1; diff --git a/src/worker.js b/src/worker.js index c589f28612dec..3b100e41f58aa 100644 --- a/src/worker.js +++ b/src/worker.js @@ -76,6 +76,7 @@ this.onmessage = function(e) { #if !MINIMAL_RUNTIME Module['DYNAMIC_BASE'] = e.data.DYNAMIC_BASE; #endif + Module['mainThreadFutex'] = e.data.mainThreadFutex; // Module and memory were sent from main thread #if MINIMAL_RUNTIME diff --git a/tests/pthread/test_pthread_proxy_hammer.cpp b/tests/pthread/test_pthread_proxy_hammer.cpp index 16cc5be82562d..996006ba32cde 100644 --- a/tests/pthread/test_pthread_proxy_hammer.cpp +++ b/tests/pthread/test_pthread_proxy_hammer.cpp @@ -51,7 +51,7 @@ random_device::operator()() int main() { int total = 0; - for (int i = 0; i < 1024; i++) { + for (int i = 0; i < ITERATIONS; i++) { // printf causes proxying printf("%d %d\n", i, total); for (int j = 0; j < 1024; j++) { diff --git a/tests/test_browser.py b/tests/test_browser.py index f3504e96648b2..043cdfa1e6039 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4912,16 +4912,16 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) - @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): - # the specific symptom of the hangs that were fixed is that the test hangs - # at some point, using 0% CPU. often that occured in 0-200 iterations out - # of the 1024 the test runs by default, but this can vary by machine and - # browser... + # the specific symptom of the hang that was fixed is that the test hangs + # at some point, using 0% CPU. often that occured in 0-200 iterations, but + # to make the test fast it runs a lower number. increase "ITERATIONS" when + # bisecting. self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', - args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD'], + args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD', + '-DITERATIONS=1024'], timeout=10000, # don't run this with the default extra_tries value, as this is # *meant* to notice something random, a race condition. From f46a8975609fbf79e87d0dc9b2a0b47689df9556 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:10:00 -0700 Subject: [PATCH 06/25] fixes a regression from #12055 --- tests/test_browser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index 043cdfa1e6039..1802acac5a696 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4912,12 +4912,12 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) + @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): # the specific symptom of the hang that was fixed is that the test hangs # at some point, using 0% CPU. often that occured in 0-200 iterations, but - # to make the test fast it runs a lower number. increase "ITERATIONS" when - # bisecting. + # you may want to adjust "ITERATIONS". self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD', From 7ecf88863994cab57d9b0e48470fb333e8db2001 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:33:29 -0700 Subject: [PATCH 07/25] cleaner --- src/library_pthread.js | 11 +++++++---- src/worker.js | 4 +++- system/lib/pthread/library_pthread.c | 5 +++++ tests/test_browser.py | 4 ++-- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index 7e36e91f3574b..a5b86a5b33d5e 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -5,7 +5,7 @@ */ var LibraryPThread = { - $PThread__postset: 'if (!ENVIRONMENT_IS_PTHREAD) PThread.initMainThreadBlock(); else PThread.initWorker();', + $PThread__postset: 'if (!ENVIRONMENT_IS_PTHREAD) PThread.initMainThreadBlock();', $PThread__deps: ['$registerPthreadPtr', '$ERRNO_CODES', 'emscripten_futex_wake', '$killThread', '$cancelThread', '$cleanupThread', @@ -71,7 +71,7 @@ var LibraryPThread = { Atomics.store(HEAPU32, (PThread.mainThreadBlock + {{{ C_STRUCTS.pthread.tid }}} ) >> 2, PThread.mainThreadBlock); // Main thread ID. Atomics.store(HEAPU32, (PThread.mainThreadBlock + {{{ C_STRUCTS.pthread.pid }}} ) >> 2, {{{ PROCINFO.pid }}}); // Process ID. - PThread.mainThreadFutex = _malloc(4); + PThread.initShared(); #if PTHREADS_PROFILING PThread.createProfilerBlock(PThread.mainThreadBlock); @@ -89,7 +89,7 @@ var LibraryPThread = { _emscripten_register_main_browser_thread_id(PThread.mainThreadBlock); }, initWorker: function() { - PThread.mainThreadFutex = Module['mainThreadFutex']; + PThread.initShared(); #if USE_CLOSURE_COMPILER // worker.js is not compiled together with us, and must access certain @@ -100,6 +100,10 @@ var LibraryPThread = { PThread['threadExit'] = PThread.threadExit; #endif }, + initShared: function() { + PThread.mainThreadFutex = Module['_main_thread_futex']; + assert(PThread.mainThreadFutex > 0); + }, // Maps pthread_t to pthread info objects pthreads: {}, threadExitHandlers: [], // An array of C functions to run when this thread exits. @@ -398,7 +402,6 @@ var LibraryPThread = { // object in Module['mainScriptUrlOrBlob'], or a URL to it, so that pthread Workers can // independently load up the same main application file. 'urlOrBlob': Module['mainScriptUrlOrBlob'] || _scriptDir, - 'mainThreadFutex': PThread.mainThreadFutex, #if WASM2JS // the polyfill WebAssembly.Memory instance has function properties, // which will fail in postMessage, so just send a custom object with the diff --git a/src/worker.js b/src/worker.js index 3b100e41f58aa..2ad7b72fff438 100644 --- a/src/worker.js +++ b/src/worker.js @@ -76,7 +76,6 @@ this.onmessage = function(e) { #if !MINIMAL_RUNTIME Module['DYNAMIC_BASE'] = e.data.DYNAMIC_BASE; #endif - Module['mainThreadFutex'] = e.data.mainThreadFutex; // Module and memory were sent from main thread #if MINIMAL_RUNTIME @@ -119,6 +118,9 @@ this.onmessage = function(e) { importScripts(objectUrl); URL.revokeObjectURL(objectUrl); } + // After the script has been fully loaded, initialize the worker's JS + // runtime. + Module['PThread']['initWorker'](); #if MODULARIZE #if MINIMAL_RUNTIME {{{ EXPORT_NAME }}}(imports).then(function (instance) { diff --git a/system/lib/pthread/library_pthread.c b/system/lib/pthread/library_pthread.c index 6af1da45d6379..18f33ed296101 100644 --- a/system/lib/pthread/library_pthread.c +++ b/system/lib/pthread/library_pthread.c @@ -913,6 +913,11 @@ int llvm_atomic_load_add_i32_p0i32(int* ptr, int delta) { return emscripten_atomic_add_u32(ptr, delta); } +// Stores the memory address that the main thread is waiting on, if any. If +// the main thread is waiting, we wake it up before waking up any workers. +EMSCRIPTEN_KEEPALIVE +int main_thread_futex; + typedef struct main_args { int argc; char** argv; diff --git a/tests/test_browser.py b/tests/test_browser.py index 1802acac5a696..97142f6c3a7d4 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4912,7 +4912,7 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) - @unittest.skip("only run this manually, to test for race conditions") +# @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): # the specific symptom of the hang that was fixed is that the test hangs @@ -4921,7 +4921,7 @@ def test_manual_pthread_proxy_hammer(self): self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD', - '-DITERATIONS=1024'], + '-DITERATIONS=1024', '-g1'], timeout=10000, # don't run this with the default extra_tries value, as this is # *meant* to notice something random, a race condition. From 98baeb1bdd8e4f857ab394b2cedaf8d591a828bc Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:37:16 -0700 Subject: [PATCH 08/25] cleaner --- src/library_pthread.js | 11 +++++++++++ src/postamble.js | 12 +----------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index a5b86a5b33d5e..9b34fb65f7a77 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -90,6 +90,17 @@ var LibraryPThread = { }, initWorker: function() { PThread.initShared(); +#if EMBIND + // Embind must initialize itself on all threads, as it generates support JS. + Module['___embind_register_native_and_builtin_types'](); +#endif // EMBIND +#if MODULARIZE + // The promise resolve function typically gets called as part of the execution + // of the Module `run`. The workers/pthreads don't execute `run` here, they + // call `run` in response to a message at a later time, so the creation + // promise can be resolved, marking the pthread-Module as initialized. + readyPromiseResolve(Module); +#endif // MODULARIZE #if USE_CLOSURE_COMPILER // worker.js is not compiled together with us, and must access certain diff --git a/src/postamble.js b/src/postamble.js index 4769c6574bd72..4cc1403bf3592 100644 --- a/src/postamble.js +++ b/src/postamble.js @@ -470,17 +470,7 @@ if (!ENVIRONMENT_IS_PTHREAD) // EXIT_RUNTIME=0 only applies to default behavior if (!ENVIRONMENT_IS_PTHREAD) { run(); } else { -#if EMBIND - // Embind must initialize itself on all threads, as it generates support JS. - Module['___embind_register_native_and_builtin_types'](); -#endif // EMBIND -#if MODULARIZE - // The promise resolve function typically gets called as part of the execution - // of the Module `run`. The workers/pthreads don't execute `run` here, they - // call `run` in response to a message at a later time, so the creation - // promise can be resolved, marking the pthread-Module as initialized. - readyPromiseResolve(Module); -#endif // MODULARIZE + PThread.initWorker(); } #else run(); From b47e01f0d5895a9a8020ea1f3c2a1b28d6e344cd Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:47:39 -0700 Subject: [PATCH 09/25] fix --- src/worker.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/worker.js b/src/worker.js index 2ad7b72fff438..c589f28612dec 100644 --- a/src/worker.js +++ b/src/worker.js @@ -118,9 +118,6 @@ this.onmessage = function(e) { importScripts(objectUrl); URL.revokeObjectURL(objectUrl); } - // After the script has been fully loaded, initialize the worker's JS - // runtime. - Module['PThread']['initWorker'](); #if MODULARIZE #if MINIMAL_RUNTIME {{{ EXPORT_NAME }}}(imports).then(function (instance) { From 213ef27d455572670837dc2362e82b0bf1faf930 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 15:54:41 -0700 Subject: [PATCH 10/25] restore --- tests/test_browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index c81da96278a27..6601cac07fc3d 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4919,7 +4919,7 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) -# @unittest.skip("only run this manually, to test for race conditions") + @unittest.skip("only run this manually, to test for race conditions") @requires_threads def test_manual_pthread_proxy_hammer(self): # the specific symptom of the hang that was fixed is that the test hangs From 5d8fca8c7c269c683dea64123577ef068e235d80 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 18:05:18 -0700 Subject: [PATCH 11/25] unset --- src/library_pthread.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/library_pthread.js b/src/library_pthread.js index ee77e8a0a6425..ca98e7af88276 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1162,6 +1162,11 @@ var LibraryPThread = { if (tNow > tEnd) { #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); +#endif + // Stop marking ourselves as waiting. + old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); +#if ASSERTIONS + assert(old == addr); #endif return -{{{ cDefine('ETIMEDOUT') }}}; } From 34b2ff2f0c06096f4769827320f63e51b647a262 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 18:06:33 -0700 Subject: [PATCH 12/25] fix --- src/library_pthread.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/library_pthread.js b/src/library_pthread.js index ca98e7af88276..46d719f067033 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -113,7 +113,9 @@ var LibraryPThread = { }, initShared: function() { PThread.mainThreadFutex = Module['_main_thread_futex']; +#if ASSERTIONS assert(PThread.mainThreadFutex > 0); +#endif }, // Maps pthread_t to pthread info objects pthreads: {}, From 88bf9ff1f8ef713290e937703b46f68ed7122f7c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 18:09:38 -0700 Subject: [PATCH 13/25] fix --- src/library_pthread.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index 46d719f067033..8228f426d2065 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1168,7 +1168,9 @@ var LibraryPThread = { // Stop marking ourselves as waiting. old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); #if ASSERTIONS - assert(old == addr); + // The old value must have been our address, or perhaps in a race + // another thread just allowed us to run, after our timeout. + assert(old == addr || old == 0); #endif return -{{{ cDefine('ETIMEDOUT') }}}; } From e256f9246de1e39de89a24e127866e26d23d7c63 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 18:17:23 -0700 Subject: [PATCH 14/25] fix --- src/postamble_minimal.js | 6 +++++- src/worker.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/postamble_minimal.js b/src/postamble_minimal.js index 77e5ffbc4b85d..4da7dd814f83f 100644 --- a/src/postamble_minimal.js +++ b/src/postamble_minimal.js @@ -64,7 +64,11 @@ function initRuntime(asm) { Module['registerPthreadPtr'] = registerPthreadPtr; Module['_pthread_self'] = _pthread_self; - if (ENVIRONMENT_IS_PTHREAD) return; + if (ENVIRONMENT_IS_PTHREAD) { + PThread.initWorker(); + return; + } + // Pass the thread address inside the asm.js scope to store it for fast access that avoids the need for a FFI out. registerPthreadPtr(PThread.mainThreadBlock, /*isMainBrowserThread=*/!ENVIRONMENT_IS_WORKER, /*isMainRuntimeThread=*/1); _emscripten_register_main_browser_thread_id(PThread.mainThreadBlock); diff --git a/src/worker.js b/src/worker.js index 77507729ec40e..9b82149b1b091 100644 --- a/src/worker.js +++ b/src/worker.js @@ -244,7 +244,7 @@ this.onmessage = function(e) { } } catch(ex) { err('worker.js onmessage() captured an uncaught exception: ' + ex); - if (ex.stack) err(ex.stack); + if (ex && ex.stack) err(ex.stack); throw ex; } }; From 64467264d0500896d97a9f773f753b7bfb5da104 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 22 Sep 2020 20:12:05 -0700 Subject: [PATCH 15/25] fix --- src/library_pthread.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index 8228f426d2065..e169d68273387 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1174,7 +1174,16 @@ var LibraryPThread = { #endif return -{{{ cDefine('ETIMEDOUT') }}}; } - _emscripten_main_thread_process_queued_calls(); // We are performing a blocking loop here, so must pump any pthreads if they want to perform operations that are proxied. + // We are performing a blocking loop here, so must pump any pthreads if + // they want to perform operations that are proxied. + // Note that we have to do so carefully, as we may take a lock while + // doing so, which can recurse into this function; stop marking + // ourselves as waiting while we do so. + old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); +#if ASSERTIONS + assert(old == addr || old == 0); +#endif + _emscripten_main_thread_process_queued_calls(); addr = Atomics.load(HEAP32, PThread.mainThreadFutex >> 2); // Look for a worker thread waking us up. } #if PTHREADS_PROFILING From faa60bed5a6ee5ebb3768e3727d943a790dc9e75 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Wed, 23 Sep 2020 07:14:09 -0700 Subject: [PATCH 16/25] another assert --- src/library_pthread.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index e169d68273387..007e9c7c030a6 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1128,7 +1128,8 @@ var LibraryPThread = { emscripten_futex_wait__deps: ['emscripten_main_thread_process_queued_calls'], emscripten_futex_wait: function(addr, val, timeout) { if (addr <= 0 || addr > HEAP8.length || addr&3 != 0) return -{{{ cDefine('EINVAL') }}}; - if (ENVIRONMENT_IS_NODE || ENVIRONMENT_IS_WORKER) { + // We can do a normal blocking wait anywhere but on the main browser thread. + if (!ENVIRONMENT_IS_WEB) { #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); #endif @@ -1211,6 +1212,13 @@ var LibraryPThread = { var mainThreadWaitAddress = Atomics.load(HEAP32, PThread.mainThreadFutex >> 2); var mainThreadWoken = 0; if (mainThreadWaitAddress == addr) { +#if ASSERTIONS + // We only use mainThreadFutex on the main browser thread, where we + // cannot block while we wait. Therefore we should only see it set from + // other threads, and not on the main thread itself. In other words, the + // main thread must never try to wake itself up! + assert(!ENVIRONMENT_IS_WEB); +#endif var loadedAddr = Atomics.compareExchange(HEAP32, PThread.mainThreadFutex >> 2, mainThreadWaitAddress, 0); if (loadedAddr == mainThreadWaitAddress) { --count; From 5f41a59e9841f3f5638a5d9c1f0d86be74c1a7d1 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 11:06:40 -0700 Subject: [PATCH 17/25] fix --- src/library_pthread.js | 102 ++++++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 17 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index 007e9c7c030a6..9b6e4a574660c 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1142,37 +1142,60 @@ var LibraryPThread = { if (ret === 'ok') return 0; throw 'Atomics.wait returned an unexpected value ' + ret; } else { - // Atomics.wait is not available in the main browser thread, so simulate it via busy spinning. - var loadedVal = Atomics.load(HEAP32, addr >> 2); - if (val != loadedVal) return -{{{ cDefine('EWOULDBLOCK') }}}; + // First, check if the value is correct for us to wait on. + if (Atomics.load(HEAP32, addr >> 2) != val) { + return -{{{ cDefine('EWOULDBLOCK') }}}; + } + // Atomics.wait is not available in the main browser thread, so simulate it via busy spinning. var tNow = performance.now(); var tEnd = tNow + timeout; #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); #endif - // Register globally which address the main thread is simulating to be waiting on. When zero, main thread is not waiting on anything, - // and on nonzero, the contents of address pointed by PThread.mainThreadFutex tell which address the main thread is simulating its wait on. - var old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, addr); + // Register globally which address the main thread is simulating to be + // waiting on. When zero, main thread is not waiting on anything, and on + // nonzero, the contents of address pointed by PThread.mainThreadFutex + // tell which address the main thread is simulating its wait on. + // We need to be careful of recursion here: If we wait on a futex, and + // then call _emscripten_main_thread_process_queued_calls() below, that + // will call code that takes the proxying mutex - which can once more + // reach this code in a nested call. In case of nesting, lastAddr will + // be nonzero, and to avoid needing to reason about a stack of futexes the + // main thread is waiting on, avoid using this mechanism in nested calls. + // That means that the main thread has no special priority for the second + // and any further futexes it waits on. TODO: a more complex stack of + // futexes, or marking a futex as "waited on by the main thread" may help + // achieve better responsiveness. However, as the second futex is likely + // the proxying mutex, that one tends to not be held for very short + // periods of time, so as long as the underlying locking mechanism is + // reasonably fair, we should be ok. #if ASSERTIONS assert(PThread.mainThreadFutex > 0); - assert(old == 0); // we must not have been waiting before. #endif - var ourWaitAddress = addr; // We may recursively re-enter this function while processing queued calls, in which case we'll do a spurious wakeup of the older wait operation. - while (addr == ourWaitAddress) { + var lastAddr = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, addr); +#if ASSERTIONS + // We must not have already been waiting, see comment above. + assert(lastAddr == 0); +#endif + + while (1) { + // Check for a timeout. tNow = performance.now(); if (tNow > tEnd) { #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); #endif - // Stop marking ourselves as waiting. - old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); + // We timed out, so stop marking ourselves as waiting. + lastAddr = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); #if ASSERTIONS - // The old value must have been our address, or perhaps in a race - // another thread just allowed us to run, after our timeout. - assert(old == addr || old == 0); + // The current value must have been our address which we set, or + // in a race it was set to 0 which means another thread just allowed + // us to run, but (tragically) that happened just a bit too late. + assert(lastAddr == addr || lastAddr == 0); #endif + } return -{{{ cDefine('ETIMEDOUT') }}}; } // We are performing a blocking loop here, so must pump any pthreads if @@ -1180,12 +1203,57 @@ var LibraryPThread = { // Note that we have to do so carefully, as we may take a lock while // doing so, which can recurse into this function; stop marking // ourselves as waiting while we do so. - old = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); + lastAddr = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, 0); #if ASSERTIONS - assert(old == addr || old == 0); + assert(lastAddr == addr || lastAddr == 0); #endif + if (lastAddr == 0) { + // We were told to stop waiting, so stop. + break; + } _emscripten_main_thread_process_queued_calls(); - addr = Atomics.load(HEAP32, PThread.mainThreadFutex >> 2); // Look for a worker thread waking us up. + + // Check the value, as if we were starting the futex all over again. + // This handles the following case: + // + // * wait on futex A + // * recurse into emscripten_main_thread_process_queued_calls(), + // which waits on futex B. that sets the mainThreadFutex address to + // futex B, and there is no longer any mention of futex A. + // * a worker is done with futex A. it checks mainThreadFutex but does + // not see A, so it does nothing special for the main thread. + // * a worker is done with futex B. it flips mainThreadMutex from B + // to 0, ending the wait on futex B. + // * we return to the wait on futex A. mainThreadFutex is 0, but that + // is because of futex B being done - we can't tell from + // mainThreadFutex whether A is done or not. therefore, check the + // memory value of the futex. + // + // That case motivates the design here. Given that, checking the memory + // address is also necessary for other reasons: we unset and re-set our + // address in mainThreadFutex around calls to + // emscripten_main_thread_process_queued_calls(), and a worker could + // attempt to wake us up right before/after such times. + // + // Note that checking the memory value of the futex is valid to do: we + // could easily have been delayed (relative to the worker holding on + // to futex A), which means we could be starting all of our work at the + // later time when there is no need to block. The only "odd" thing is + // that we may have caused side effects in that "delay" time. But the + // only side effects we can have are to call + // emscripten_main_thread_process_queued_calls(). That is always ok to + // do on the main thread. So in effect, what happened on the main thread + // is the same as calling emscripten_main_thread_process_queued_calls() + // a few times times before calling emscripten_futex_wait(). + if (Atomics.load(HEAP32, addr >> 2) != val) { + return -{{{ cDefine('EWOULDBLOCK') }}}; + } + + // Mark us as waiting once more, and continue the loop. + lastAddr = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, addr); +#if ASSERTIONS + assert(lastAddr == 0); +#endif } #if PTHREADS_PROFILING PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); From c956cfd9b3e82aec230ddb697e4b1ecb32a6f7a2 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 11:11:13 -0700 Subject: [PATCH 18/25] fix --- src/library_pthread.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index 9b6e4a574660c..b6aa699b0a1c7 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1195,7 +1195,6 @@ var LibraryPThread = { // us to run, but (tragically) that happened just a bit too late. assert(lastAddr == addr || lastAddr == 0); #endif - } return -{{{ cDefine('ETIMEDOUT') }}}; } // We are performing a blocking loop here, so must pump any pthreads if From 6a3f32d6c6d15e1a5e5f1b7aae61f61b63ef7bb0 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 11:16:55 -0700 Subject: [PATCH 19/25] more --- tests/test_browser.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index 6601cac07fc3d..a8f3fe35642e2 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4919,16 +4919,20 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) - @unittest.skip("only run this manually, to test for race conditions") + #@unittest.skip("only run this manually, to test for race conditions") + @parameterized({ + 'normal': ([],), + 'assertions': (['-s', 'ASSERTIONS'],) + }) @requires_threads - def test_manual_pthread_proxy_hammer(self): + def test_manual_pthread_proxy_hammer(self, args): # the specific symptom of the hang that was fixed is that the test hangs # at some point, using 0% CPU. often that occured in 0-200 iterations, but # you may want to adjust "ITERATIONS". self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'), expected='0', args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD', - '-DITERATIONS=1024', '-g1'], + '-DITERATIONS=1024', '-g1'] + args, timeout=10000, # don't run this with the default extra_tries value, as this is # *meant* to notice something random, a race condition. From f3f48c3c8d61bb94bec1e12541ed1b97bd86f9ab Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 11:19:20 -0700 Subject: [PATCH 20/25] comment --- src/library_pthread.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index b6aa699b0a1c7..f2453f0d21d36 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1241,8 +1241,10 @@ var LibraryPThread = { // that we may have caused side effects in that "delay" time. But the // only side effects we can have are to call // emscripten_main_thread_process_queued_calls(). That is always ok to - // do on the main thread. So in effect, what happened on the main thread - // is the same as calling emscripten_main_thread_process_queued_calls() + // do on the main thread (it's why it is ok for us to call it in the + // middle of this function, and elsewhere). So if we check the value + // here and return, it's the same is if what happened on the main thread + // was the same as calling emscripten_main_thread_process_queued_calls() // a few times times before calling emscripten_futex_wait(). if (Atomics.load(HEAP32, addr >> 2) != val) { return -{{{ cDefine('EWOULDBLOCK') }}}; From 67d305a86135ae93b83015b7bcea14a44b003015 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 11:53:16 -0700 Subject: [PATCH 21/25] undo --- tests/test_browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_browser.py b/tests/test_browser.py index a8f3fe35642e2..03a3aca5f7b92 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -4919,7 +4919,7 @@ def test_zzz_zzz_4GB_fail(self): self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0'] self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE]) - #@unittest.skip("only run this manually, to test for race conditions") + @unittest.skip("only run this manually, to test for race conditions") @parameterized({ 'normal': ([],), 'assertions': (['-s', 'ASSERTIONS'],) From c1374ce8ef5228e6259fb67cbfd531edf22579a0 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 12:47:56 -0700 Subject: [PATCH 22/25] fix comment --- src/library_pthread.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index f2453f0d21d36..a749db193e0cd 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1161,16 +1161,11 @@ var LibraryPThread = { // We need to be careful of recursion here: If we wait on a futex, and // then call _emscripten_main_thread_process_queued_calls() below, that // will call code that takes the proxying mutex - which can once more - // reach this code in a nested call. In case of nesting, lastAddr will - // be nonzero, and to avoid needing to reason about a stack of futexes the - // main thread is waiting on, avoid using this mechanism in nested calls. - // That means that the main thread has no special priority for the second - // and any further futexes it waits on. TODO: a more complex stack of - // futexes, or marking a futex as "waited on by the main thread" may help - // achieve better responsiveness. However, as the second futex is likely - // the proxying mutex, that one tends to not be held for very short - // periods of time, so as long as the underlying locking mechanism is - // reasonably fair, we should be ok. + // reach this code in a nested call. To avoid interference between the + // two, unmark ourselves before calling the potentially-recursive call. + // See below for how we handle the case of our futex being notified + // during the time in between when we are not set as the value of + // mainThreadFutex. #if ASSERTIONS assert(PThread.mainThreadFutex > 0); #endif From a27ef73e1e6d469b6f9f14f5df817308882a8349 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 13:01:50 -0700 Subject: [PATCH 23/25] Fix a test --- tests/pthread/test_pthread_proxying_in_futex_wait.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/pthread/test_pthread_proxying_in_futex_wait.cpp b/tests/pthread/test_pthread_proxying_in_futex_wait.cpp index 4a2f485de84f2..3eb11a274a325 100644 --- a/tests/pthread/test_pthread_proxying_in_futex_wait.cpp +++ b/tests/pthread/test_pthread_proxying_in_futex_wait.cpp @@ -3,6 +3,7 @@ // University of Illinois/NCSA Open Source License. Both these licenses can be // found in the LICENSE file. +#include #include #include #include @@ -47,9 +48,12 @@ int main() int rc = pthread_create(&thread, &attr, ThreadMain, 0); assert(rc == 0); rc = emscripten_futex_wait(&main_thread_wait_val, 1, 15 * 1000); - if (rc != 0) + // An rc of 0 means no error, and of EWOULDBLOCK means that the value is + // not the expected one, which can happen if the pthread manages to set it + // before we reach the futex_wait. + if (rc != 0 && rc != -EWOULDBLOCK) { - printf("ERROR! futex wait timed out!\n"); + printf("ERROR! futex wait errored %d!\n", rc); result = 2; #ifdef REPORT_RESULT REPORT_RESULT(result); From 875a0e90afee8ba7b8d1c039fae7895cf2bc0dcb Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 13:09:21 -0700 Subject: [PATCH 24/25] comments --- src/library_pthread.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/library_pthread.js b/src/library_pthread.js index a749db193e0cd..f27e5a039094e 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -1155,23 +1155,23 @@ var LibraryPThread = { PThread.setThreadStatusConditional(_pthread_self(), {{{ cDefine('EM_THREAD_STATUS_RUNNING') }}}, {{{ cDefine('EM_THREAD_STATUS_WAITFUTEX') }}}); #endif // Register globally which address the main thread is simulating to be - // waiting on. When zero, main thread is not waiting on anything, and on - // nonzero, the contents of address pointed by PThread.mainThreadFutex + // waiting on. When zero, the main thread is not waiting on anything, and on + // nonzero, the contents of the address pointed by PThread.mainThreadFutex // tell which address the main thread is simulating its wait on. // We need to be careful of recursion here: If we wait on a futex, and // then call _emscripten_main_thread_process_queued_calls() below, that // will call code that takes the proxying mutex - which can once more // reach this code in a nested call. To avoid interference between the - // two, unmark ourselves before calling the potentially-recursive call. - // See below for how we handle the case of our futex being notified - // during the time in between when we are not set as the value of - // mainThreadFutex. + // two (there is just a single mainThreadFutex at a time), unmark + // ourselves before calling the potentially-recursive call. See below for + // how we handle the case of our futex being notified during the time in + // between when we are not set as the value of mainThreadFutex. #if ASSERTIONS assert(PThread.mainThreadFutex > 0); #endif var lastAddr = Atomics.exchange(HEAP32, PThread.mainThreadFutex >> 2, addr); #if ASSERTIONS - // We must not have already been waiting, see comment above. + // We must not have already been waiting. assert(lastAddr == 0); #endif @@ -1192,8 +1192,8 @@ var LibraryPThread = { #endif return -{{{ cDefine('ETIMEDOUT') }}}; } - // We are performing a blocking loop here, so must pump any pthreads if - // they want to perform operations that are proxied. + // We are performing a blocking loop here, so we must handle proxied + // events from pthreads, to avoid deadlocks. // Note that we have to do so carefully, as we may take a lock while // doing so, which can recurse into this function; stop marking // ourselves as waiting while we do so. From eb6066efdd42481535d1f2f418e0b5a2efb92011 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 23 Sep 2020 16:12:55 -0700 Subject: [PATCH 25/25] use a pointer --- system/lib/pthread/library_pthread.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/lib/pthread/library_pthread.c b/system/lib/pthread/library_pthread.c index 18f33ed296101..ea0006d3c141c 100644 --- a/system/lib/pthread/library_pthread.c +++ b/system/lib/pthread/library_pthread.c @@ -916,7 +916,7 @@ int llvm_atomic_load_add_i32_p0i32(int* ptr, int delta) { // Stores the memory address that the main thread is waiting on, if any. If // the main thread is waiting, we wake it up before waking up any workers. EMSCRIPTEN_KEEPALIVE -int main_thread_futex; +void* main_thread_futex; typedef struct main_args { int argc;