Skip to content

Commit

Permalink
Fix MAIN_THREAD_EM_ASM_INT + CAN_ADDRESS_2GB
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sbc100 committed Feb 8, 2024
1 parent fdc2ea7 commit 792db7d
Show file tree
Hide file tree
Showing 10 changed files with 41 additions and 33 deletions.
1 change: 1 addition & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/jsifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
});
Expand Down
20 changes: 7 additions & 13 deletions src/library.js
Original file line number Diff line number Diff line change
Expand Up @@ -2896,7 +2896,7 @@ addToLibrary({
'$proxyToMainThread'
#endif
],
$runMainThreadEmAsm: (code, sigPtr, argbuf, sync) => {
$runMainThreadEmAsm: (emAsmAddr, sigPtr, argbuf, sync) => {
var args = readEmAsmArgs(sigPtr, argbuf);
#if PTHREADS
if (ENVIRONMENT_IS_PTHREAD) {
Expand All @@ -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, emAsmAddr, 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(emAsmAddr), `No EM_ASM constant found at address ${emAsmAddr}. The loaded WebAssembly file is likely out of sync with the generated JavaScript.`);
#endif
return ASM_CONSTS[code](...args);
return ASM_CONSTS[emAsmAddr](...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: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, 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: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, 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: (emAsmAddr, sigPtr, argbuf) => runMainThreadEmAsm(emAsmAddr, sigPtr, argbuf, 0),
#endif
#if !DECLARE_ASM_MODULE_EXPORTS
Expand Down
24 changes: 15 additions & 9 deletions src/library_pthread.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, emAsmAddr, sync, ...callArgs) => {
// EM_ASM proxying is done by passing a pointer to the address of the EM_ASM
// contant as `emAsmAddr`. JS library proxying is done by passing an index
// into `proxiedJSCallArgs` as `funcIndex`. If `emAsmAddr` 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.
Expand Down Expand Up @@ -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, emAsmAddr, serializedNumCallArgs, args, sync);
});
},

Expand All @@ -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, emAsmAddr, 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
Expand All @@ -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 emAsmAddr
#if HAVE_EM_ASM
var isEmAsmConst = index < 0;
var func = !isEmAsmConst ? proxiedFunctionTable[index] : ASM_CONSTS[-index - 1];
var func = emAsmAddr ? ASM_CONSTS[emAsmAddr] : proxiedFunctionTable[funcIndex];
#else
var func = proxiedFunctionTable[index];
#if ASSERTIONS
assert(!emAsmAddr);
#endif
var func = proxiedFunctionTable[funcIndex];
#endif
#if ASSERTIONS
assert(!(funcIndex && emAsmAddr));
assert(func.length == numCallArgs, 'Call args mismatch in _emscripten_receive_on_main_thread_js');
#endif
PThread.currentProxiedOperationCallerThread = callingThread;
Expand Down
2 changes: 1 addition & 1 deletion src/library_sdl.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/library_sigs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
9 changes: 6 additions & 3 deletions system/lib/pthread/proxying.c
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ em_promise_t emscripten_proxy_promise(em_proxying_queue* q,

typedef struct proxied_js_func_t {
int funcIndex;
void* emAsmAddr;
pthread_t callingThread;
int numArgs;
double* argBuffer;
Expand All @@ -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->emAsmAddr, 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* em_asm_addr,
int num_args,
double* buffer,
int sync) {
proxied_js_func_t f = {
.funcIndex = index,
.funcIndex = func_index,
.emAsmAddr = em_asm_addr,
.callingThread = pthread_self(),
.numArgs = num_args,
.argBuffer = buffer,
Expand Down
2 changes: 1 addition & 1 deletion system/lib/pthread/threading_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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* emAsmAddr, 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
Expand Down
10 changes: 7 additions & 3 deletions test/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
2 changes: 1 addition & 1 deletion tools/emscripten.py
Original file line number Diff line number Diff line change
Expand Up @@ -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_____',
Expand Down

0 comments on commit 792db7d

Please sign in to comment.