From f6593b5030b41a926f1c6e6fd334fcc70f38d790 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Wed, 7 Feb 2024 11:11:57 -0800 Subject: [PATCH] Fix MAIN_THREAD_EM_ASM_INT + CAN_ADDRESS_2GB We were using the sign bit of an integer to distinguish between data pointers and fixed JS function indexes, but that doesn't work once that data address can be larger than 2^31. Technically this is very unlikely in practice since in order to get an EM_ASM address over 2^31 you would either need 2Gb of static data to be using `-sGLOBAL_BASE=2gb` like we do in the tests. An alternative approach here would be assume we have fewer than `GLOBAL_BASE` (1024 is most cases) proxied JS library functions and then we could assume that small integers we JS library functions and larger ones were data pointers (EM_ASM functions). However that seems fragile too. Passing an extra argument around seems like a small cost here. --- .circleci/config.yml | 1 + src/jsifier.js | 2 +- src/library.js | 20 +++++++------------- src/library_pthread.js | 24 +++++++++++++++--------- src/library_sdl.js | 2 +- src/library_sigs.js | 2 +- system/lib/pthread/proxying.c | 9 ++++++--- system/lib/pthread/threading_internal.h | 2 +- test/test_core.py | 10 +++++++--- tools/emscripten.py | 2 +- 10 files changed, 41 insertions(+), 33 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 35fae823205af..a662a9d9bef0c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -817,6 +817,7 @@ jobs: browser_2gb.test_fulles2_sdlproc browser_2gb.test_cubegeom* browser_2gb.test_html5_webgl_create_context* + browser_2gb.test_main_thread_async_em_asm " test-browser-chrome-wasm64-4gb: executor: bionic diff --git a/src/jsifier.js b/src/jsifier.js index 02580d0befe13..c172adc21a159 100644 --- a/src/jsifier.js +++ b/src/jsifier.js @@ -272,7 +272,7 @@ function(${args}) { return ` function(${args}) { if (ENVIRONMENT_IS_PTHREAD) - return ${proxyFunc}(${proxiedFunctionTable.length}, ${+sync}${args ? ', ' : ''}${args}); + return ${proxyFunc}(${proxiedFunctionTable.length}, 0, ${+sync}${args ? ', ' : ''}${args}); ${body} }\n` }); diff --git a/src/library.js b/src/library.js index 88cc447b59859..870b98a23ee57 100644 --- a/src/library.js +++ b/src/library.js @@ -2896,7 +2896,7 @@ addToLibrary({ '$proxyToMainThread' #endif ], - $runMainThreadEmAsm: (code, sigPtr, argbuf, sync) => { + $runMainThreadEmAsm: (codePtr, sigPtr, argbuf, sync) => { var args = readEmAsmArgs(sigPtr, argbuf); #if PTHREADS if (ENVIRONMENT_IS_PTHREAD) { @@ -2909,29 +2909,23 @@ addToLibrary({ // of using __proxy. (And dor simplicity, do the same in the sync // case as well, even though it's not strictly necessary, to keep the two // code paths as similar as possible on both sides.) - // -1 - code is the encoding of a proxied EM_ASM, as a negative number - // (positive numbers are non-EM_ASM calls). - return proxyToMainThread(-1 - code, sync, ...args); + return proxyToMainThread(0, codePtr, sync, ...args); } #endif #if ASSERTIONS - assert(ASM_CONSTS.hasOwnProperty(code), `No EM_ASM constant found at address ${code}. The loaded WebAssembly file is likely out of sync with the generated JavaScript.`); + assert(ASM_CONSTS.hasOwnProperty(codePtr), `No EM_ASM constant found at address ${codePtr}. The loaded WebAssembly file is likely out of sync with the generated JavaScript.`); #endif - return ASM_CONSTS[code](...args); + return ASM_CONSTS[codePtr](...args); }, emscripten_asm_const_int_sync_on_main_thread__deps: ['$runMainThreadEmAsm'], - emscripten_asm_const_int_sync_on_main_thread: (code, sigPtr, argbuf) => { - return runMainThreadEmAsm(code, sigPtr, argbuf, 1); - }, + emscripten_asm_const_int_sync_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 1), emscripten_asm_const_ptr_sync_on_main_thread__deps: ['$runMainThreadEmAsm'], - emscripten_asm_const_ptr_sync_on_main_thread: (code, sigPtr, argbuf) => { - return runMainThreadEmAsm(code, sigPtr, argbuf, 1); - }, + emscripten_asm_const_ptr_sync_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 1), emscripten_asm_const_double_sync_on_main_thread: 'emscripten_asm_const_int_sync_on_main_thread', emscripten_asm_const_async_on_main_thread__deps: ['$runMainThreadEmAsm'], - emscripten_asm_const_async_on_main_thread: (code, sigPtr, argbuf) => runMainThreadEmAsm(code, sigPtr, argbuf, 0), + emscripten_asm_const_async_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 0), #endif #if !DECLARE_ASM_MODULE_EXPORTS diff --git a/src/library_pthread.js b/src/library_pthread.js index 346484af7d79e..400a3b339d094 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -962,8 +962,12 @@ var LibraryPThread = { $proxyToMainThread__deps: ['$withStackSave', '_emscripten_run_on_main_thread_js'].concat(i53ConversionDeps), $proxyToMainThread__docs: '/** @type{function(number, (number|boolean), ...number)} */', - $proxyToMainThread: (index, sync, ...callArgs) => { - // Additional arguments are passed after those two, which are the actual + $proxyToMainThread: (funcIndex, codePtr, sync, ...callArgs) => { + // EM_ASM proxying is done by passing a pointer to the address of the EM_ASM + // contant as `codePtr`. JS library proxying is done by passing and index + // into proxiedJSCallArgs as `funcIndex`. If `codePtr` is non-zero then + // `funcIndex` will be ignored. + // Additional arguments are passed after the first three are the actual // function arguments. // The serialization buffer contains the number of call params, and then // all the args here. @@ -995,7 +999,7 @@ var LibraryPThread = { HEAPF64[b + i] = arg; #endif } - return __emscripten_run_on_main_thread_js(index, serializedNumCallArgs, args, sync); + return __emscripten_run_on_main_thread_js(funcIndex, codePtr, serializedNumCallArgs, args, sync); }); }, @@ -1005,7 +1009,7 @@ var LibraryPThread = { _emscripten_receive_on_main_thread_js__deps: [ '$proxyToMainThread', '$proxiedJSCallArgs'], - _emscripten_receive_on_main_thread_js: (index, callingThread, numCallArgs, args) => { + _emscripten_receive_on_main_thread_js: (funcIndex, codePtr, callingThread, numCallArgs, args) => { // Sometimes we need to backproxy events to the calling thread (e.g. // HTML5 DOM events handlers such as // emscripten_set_mousemove_callback()), so keep track in a globally @@ -1028,15 +1032,17 @@ var LibraryPThread = { proxiedJSCallArgs[i] = HEAPF64[b + i]; #endif } - // Proxied JS library funcs are encoded as positive values, and - // EM_ASMs as negative values (see include_asm_consts) + // Proxied JS library funcs use funcIndex and EM_ASM functions use codePtr #if HAVE_EM_ASM - var isEmAsmConst = index < 0; - var func = !isEmAsmConst ? proxiedFunctionTable[index] : ASM_CONSTS[-index - 1]; + var func = codePtr ? ASM_CONSTS[codePtr] : proxiedFunctionTable[funcIndex]; #else - var func = proxiedFunctionTable[index]; +#if ASSERTIONS + assert(!codePtr); +#endif + var func = proxiedFunctionTable[funcIndex]; #endif #if ASSERTIONS + assert(!(funcIndex && codePtr)); assert(func.length == numCallArgs, 'Call args mismatch in _emscripten_receive_on_main_thread_js'); #endif PThread.currentProxiedOperationCallerThread = callingThread; diff --git a/src/library_sdl.js b/src/library_sdl.js index 42eeb2a6e331e..ed6b4f22dc48f 100644 --- a/src/library_sdl.js +++ b/src/library_sdl.js @@ -1606,7 +1606,7 @@ var LibrarySDL = { var data = surfData.image.data; var buffer = surfData.buffer; assert(buffer % 4 == 0, 'Invalid buffer offset: ' + buffer); - var src = buffer >> 2; + var src = {{{ getHeapOffset('buffer', 'i32') }}}; var dst = 0; var isScreen = surf == SDL.screen; var num; diff --git a/src/library_sigs.js b/src/library_sigs.js index d7d796d6c2102..f19460957bff1 100644 --- a/src/library_sigs.js +++ b/src/library_sigs.js @@ -328,7 +328,7 @@ sigs = { _emscripten_notify_mailbox_postmessage__sig: 'vppp', _emscripten_push_main_loop_blocker__sig: 'vppp', _emscripten_push_uncounted_main_loop_blocker__sig: 'vppp', - _emscripten_receive_on_main_thread_js__sig: 'dipip', + _emscripten_receive_on_main_thread_js__sig: 'dippip', _emscripten_runtime_keepalive_clear__sig: 'v', _emscripten_set_offscreencanvas_size__sig: 'ipii', _emscripten_system__sig: 'ip', diff --git a/system/lib/pthread/proxying.c b/system/lib/pthread/proxying.c index 0e8a3eaef1147..31de7cc2d2eea 100644 --- a/system/lib/pthread/proxying.c +++ b/system/lib/pthread/proxying.c @@ -585,6 +585,7 @@ em_promise_t emscripten_proxy_promise(em_proxying_queue* q, typedef struct proxied_js_func_t { int funcIndex; + void* codePtr; pthread_t callingThread; int numArgs; double* argBuffer; @@ -595,19 +596,21 @@ typedef struct proxied_js_func_t { static void run_js_func(void* arg) { proxied_js_func_t* f = (proxied_js_func_t*)arg; f->result = _emscripten_receive_on_main_thread_js( - f->funcIndex, f->callingThread, f->numArgs, f->argBuffer); + f->funcIndex, f->codePtr, f->callingThread, f->numArgs, f->argBuffer); if (f->owned) { free(f->argBuffer); free(f); } } -double _emscripten_run_on_main_thread_js(int index, +double _emscripten_run_on_main_thread_js(int func_index, + void* code_ptr, int num_args, double* buffer, int sync) { proxied_js_func_t f = { - .funcIndex = index, + .funcIndex = func_index, + .codePtr = code_ptr, .callingThread = pthread_self(), .numArgs = num_args, .argBuffer = buffer, diff --git a/system/lib/pthread/threading_internal.h b/system/lib/pthread/threading_internal.h index 5db2a93205ed7..b62ebdae18b27 100644 --- a/system/lib/pthread/threading_internal.h +++ b/system/lib/pthread/threading_internal.h @@ -95,7 +95,7 @@ int __pthread_create_js(struct __pthread *thread, const pthread_attr_t *attr, vo int _emscripten_default_pthread_stack_size(); void __set_thread_state(pthread_t ptr, int is_main, int is_runtime, int can_block); -double _emscripten_receive_on_main_thread_js(int functionIndex, pthread_t callingThread, int numCallArgs, double* args); +double _emscripten_receive_on_main_thread_js(int funcIndex, void* codePtr, pthread_t callingThread, int numCallArgs, double* args); // Return non-zero if the calling thread supports Atomic.wait (For example // if called from the main browser thread, this function will return zero diff --git a/test/test_core.py b/test/test_core.py index 52f9cc2f565c6..6eb924b2fe9db 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -1892,15 +1892,19 @@ def test_em_asm_2(self): # test_em_asm_2, just search-replaces EM_ASM to MAIN_THREAD_EM_ASM on the test # file. That way if new test cases are added to test_em_asm_2.cpp for EM_ASM, # they will also get tested in MAIN_THREAD_EM_ASM form. - def test_main_thread_em_asm(self): + @parameterized({ + '': ([],), + 'pthread': (['-pthread', '-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME'],), + }) + def test_main_thread_em_asm(self, args): src = read_file(test_file('core/test_em_asm_2.cpp')) create_file('test.cpp', src.replace('EM_ASM', 'MAIN_THREAD_EM_ASM')) expected_result = read_file(test_file('core/test_em_asm_2.out')) create_file('test.out', expected_result.replace('EM_ASM', 'MAIN_THREAD_EM_ASM')) - self.do_run_in_out_file_test('test.cpp') - self.do_run_in_out_file_test('test.cpp', force_c=True) + self.do_run_in_out_file_test('test.cpp', emcc_args=args) + self.do_run_in_out_file_test('test.cpp', emcc_args=args, force_c=True) @needs_dylink @parameterized({ diff --git a/tools/emscripten.py b/tools/emscripten.py index 53e3c678f01dd..b1806b0b96b0d 100644 --- a/tools/emscripten.py +++ b/tools/emscripten.py @@ -940,7 +940,7 @@ def create_pointer_conversion_wrappers(metadata): '_wasmfs_identify': '_p', '_wasmfs_read_file': 'pp', '__dl_seterr': '_pp', - '_emscripten_run_on_main_thread_js': '___p_', + '_emscripten_run_on_main_thread_js': '__p_p_', '_emscripten_proxy_execute_task_queue': '_p', '_emscripten_thread_exit': '_p', '_emscripten_thread_init': '_p_____',