Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MAIN_THREAD_EM_ASM + CAN_ADDRESS_2GB #21292

Merged
merged 1 commit into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Member

@kripken kripken Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these perhaps be two separate functions, one to proxy em_asm and one for other things? I don't feel strongly though.

Either way, "codePtr" suggests to me an actual C function pointer, but these are indexes in the EM_ASM array in JS, I think? How about emAsmIndex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its is actually a data pointer pointing the address of the EM_ASM code. The EM_ASM is an map that is indexed by data pointer. I guess it could be emAsmAddr?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see... well, sgtm as is then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes, emAsmAddr is nicer.

// 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
Loading