Skip to content

Commit bd3fb8d

Browse files
committed
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.
1 parent 7bba399 commit bd3fb8d

File tree

9 files changed

+35
-31
lines changed

9 files changed

+35
-31
lines changed

.circleci/config.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,6 +817,7 @@ jobs:
817817
browser_2gb.test_fulles2_sdlproc
818818
browser_2gb.test_cubegeom*
819819
browser_2gb.test_html5_webgl_create_context*
820+
browser_2gb.test_main_thread_async_em_asm
820821
"
821822
test-browser-chrome-wasm64-4gb:
822823
executor: bionic

src/jsifier.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ function(${args}) {
272272
return `
273273
function(${args}) {
274274
if (ENVIRONMENT_IS_PTHREAD)
275-
return ${proxyFunc}(${proxiedFunctionTable.length}, ${+sync}${args ? ', ' : ''}${args});
275+
return ${proxyFunc}(${proxiedFunctionTable.length}, 0, ${+sync}${args ? ', ' : ''}${args});
276276
${body}
277277
}\n`
278278
});

src/library.js

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2896,7 +2896,7 @@ addToLibrary({
28962896
'$proxyToMainThread'
28972897
#endif
28982898
],
2899-
$runMainThreadEmAsm: (code, sigPtr, argbuf, sync) => {
2899+
$runMainThreadEmAsm: (codePtr, sigPtr, argbuf, sync) => {
29002900
var args = readEmAsmArgs(sigPtr, argbuf);
29012901
#if PTHREADS
29022902
if (ENVIRONMENT_IS_PTHREAD) {
@@ -2909,29 +2909,23 @@ addToLibrary({
29092909
// of using __proxy. (And dor simplicity, do the same in the sync
29102910
// case as well, even though it's not strictly necessary, to keep the two
29112911
// code paths as similar as possible on both sides.)
2912-
// -1 - code is the encoding of a proxied EM_ASM, as a negative number
2913-
// (positive numbers are non-EM_ASM calls).
2914-
return proxyToMainThread(-1 - code, sync, ...args);
2912+
return proxyToMainThread(0, codePtr, sync, ...args);
29152913
}
29162914
#endif
29172915
#if ASSERTIONS
2918-
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.`);
2916+
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.`);
29192917
#endif
2920-
return ASM_CONSTS[code](...args);
2918+
return ASM_CONSTS[codePtr](...args);
29212919
},
29222920
emscripten_asm_const_int_sync_on_main_thread__deps: ['$runMainThreadEmAsm'],
2923-
emscripten_asm_const_int_sync_on_main_thread: (code, sigPtr, argbuf) => {
2924-
return runMainThreadEmAsm(code, sigPtr, argbuf, 1);
2925-
},
2921+
emscripten_asm_const_int_sync_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 1),
29262922

29272923
emscripten_asm_const_ptr_sync_on_main_thread__deps: ['$runMainThreadEmAsm'],
2928-
emscripten_asm_const_ptr_sync_on_main_thread: (code, sigPtr, argbuf) => {
2929-
return runMainThreadEmAsm(code, sigPtr, argbuf, 1);
2930-
},
2924+
emscripten_asm_const_ptr_sync_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 1),
29312925

29322926
emscripten_asm_const_double_sync_on_main_thread: 'emscripten_asm_const_int_sync_on_main_thread',
29332927
emscripten_asm_const_async_on_main_thread__deps: ['$runMainThreadEmAsm'],
2934-
emscripten_asm_const_async_on_main_thread: (code, sigPtr, argbuf) => runMainThreadEmAsm(code, sigPtr, argbuf, 0),
2928+
emscripten_asm_const_async_on_main_thread: (codePtr, sigPtr, argbuf) => runMainThreadEmAsm(codePtr, sigPtr, argbuf, 0),
29352929
#endif
29362930

29372931
#if !DECLARE_ASM_MODULE_EXPORTS

src/library_pthread.js

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -962,7 +962,7 @@ var LibraryPThread = {
962962
963963
$proxyToMainThread__deps: ['$withStackSave', '_emscripten_run_on_main_thread_js'].concat(i53ConversionDeps),
964964
$proxyToMainThread__docs: '/** @type{function(number, (number|boolean), ...number)} */',
965-
$proxyToMainThread: (index, sync, ...callArgs) => {
965+
$proxyToMainThread: (funcIndex, codePtr, sync, ...callArgs) => {
966966
// Additional arguments are passed after those two, which are the actual
967967
// function arguments.
968968
// The serialization buffer contains the number of call params, and then
@@ -995,7 +995,7 @@ var LibraryPThread = {
995995
HEAPF64[b + i] = arg;
996996
#endif
997997
}
998-
return __emscripten_run_on_main_thread_js(index, serializedNumCallArgs, args, sync);
998+
return __emscripten_run_on_main_thread_js(funcIndex, codePtr, serializedNumCallArgs, args, sync);
999999
});
10001000
},
10011001
@@ -1005,7 +1005,7 @@ var LibraryPThread = {
10051005
_emscripten_receive_on_main_thread_js__deps: [
10061006
'$proxyToMainThread',
10071007
'$proxiedJSCallArgs'],
1008-
_emscripten_receive_on_main_thread_js: (index, callingThread, numCallArgs, args) => {
1008+
_emscripten_receive_on_main_thread_js: (funcIndex, codePtr, callingThread, numCallArgs, args) => {
10091009
// Sometimes we need to backproxy events to the calling thread (e.g.
10101010
// HTML5 DOM events handlers such as
10111011
// emscripten_set_mousemove_callback()), so keep track in a globally
@@ -1028,15 +1028,17 @@ var LibraryPThread = {
10281028
proxiedJSCallArgs[i] = HEAPF64[b + i];
10291029
#endif
10301030
}
1031-
// Proxied JS library funcs are encoded as positive values, and
1032-
// EM_ASMs as negative values (see include_asm_consts)
1031+
// Proxied JS library funcs use funcIndex and EM_ASM functions use codePtr
10331032
#if HAVE_EM_ASM
1034-
var isEmAsmConst = index < 0;
1035-
var func = !isEmAsmConst ? proxiedFunctionTable[index] : ASM_CONSTS[-index - 1];
1033+
var func = codePtr ? ASM_CONSTS[codePtr] : proxiedFunctionTable[funcIndex];
10361034
#else
1037-
var func = proxiedFunctionTable[index];
1035+
#if ASSERTIONS
1036+
assert(!codePtr);
1037+
#endif
1038+
var func = proxiedFunctionTable[funcIndex];
10381039
#endif
10391040
#if ASSERTIONS
1041+
assert(!(funcIndex && codePtr));
10401042
assert(func.length == numCallArgs, 'Call args mismatch in _emscripten_receive_on_main_thread_js');
10411043
#endif
10421044
PThread.currentProxiedOperationCallerThread = callingThread;

src/library_sdl.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1606,7 +1606,7 @@ var LibrarySDL = {
16061606
var data = surfData.image.data;
16071607
var buffer = surfData.buffer;
16081608
assert(buffer % 4 == 0, 'Invalid buffer offset: ' + buffer);
1609-
var src = buffer >> 2;
1609+
var src = {{{ getHeapOffset('buffer', 'i32') }}};
16101610
var dst = 0;
16111611
var isScreen = surf == SDL.screen;
16121612
var num;

src/library_sigs.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ sigs = {
328328
_emscripten_notify_mailbox_postmessage__sig: 'vppp',
329329
_emscripten_push_main_loop_blocker__sig: 'vppp',
330330
_emscripten_push_uncounted_main_loop_blocker__sig: 'vppp',
331-
_emscripten_receive_on_main_thread_js__sig: 'dipip',
331+
_emscripten_receive_on_main_thread_js__sig: 'dippip',
332332
_emscripten_runtime_keepalive_clear__sig: 'v',
333333
_emscripten_set_offscreencanvas_size__sig: 'ipii',
334334
_emscripten_system__sig: 'ip',

system/lib/pthread/proxying.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,7 @@ em_promise_t emscripten_proxy_promise(em_proxying_queue* q,
585585

586586
typedef struct proxied_js_func_t {
587587
int funcIndex;
588+
void* codePtr;
588589
pthread_t callingThread;
589590
int numArgs;
590591
double* argBuffer;
@@ -595,19 +596,21 @@ typedef struct proxied_js_func_t {
595596
static void run_js_func(void* arg) {
596597
proxied_js_func_t* f = (proxied_js_func_t*)arg;
597598
f->result = _emscripten_receive_on_main_thread_js(
598-
f->funcIndex, f->callingThread, f->numArgs, f->argBuffer);
599+
f->funcIndex, f->codePtr, f->callingThread, f->numArgs, f->argBuffer);
599600
if (f->owned) {
600601
free(f->argBuffer);
601602
free(f);
602603
}
603604
}
604605

605-
double _emscripten_run_on_main_thread_js(int index,
606+
double _emscripten_run_on_main_thread_js(int func_index,
607+
void* code_ptr,
606608
int num_args,
607609
double* buffer,
608610
int sync) {
609611
proxied_js_func_t f = {
610-
.funcIndex = index,
612+
.funcIndex = func_index,
613+
.codePtr = code_ptr,
611614
.callingThread = pthread_self(),
612615
.numArgs = num_args,
613616
.argBuffer = buffer,

system/lib/pthread/threading_internal.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ int __pthread_create_js(struct __pthread *thread, const pthread_attr_t *attr, vo
9595
int _emscripten_default_pthread_stack_size();
9696
void __set_thread_state(pthread_t ptr, int is_main, int is_runtime, int can_block);
9797

98-
double _emscripten_receive_on_main_thread_js(int functionIndex, pthread_t callingThread, int numCallArgs, double* args);
98+
double _emscripten_receive_on_main_thread_js(int funcIndex, void* codePtr, pthread_t callingThread, int numCallArgs, double* args);
9999

100100
// Return non-zero if the calling thread supports Atomic.wait (For example
101101
// if called from the main browser thread, this function will return zero

test/test_core.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1892,15 +1892,19 @@ def test_em_asm_2(self):
18921892
# test_em_asm_2, just search-replaces EM_ASM to MAIN_THREAD_EM_ASM on the test
18931893
# file. That way if new test cases are added to test_em_asm_2.cpp for EM_ASM,
18941894
# they will also get tested in MAIN_THREAD_EM_ASM form.
1895-
def test_main_thread_em_asm(self):
1895+
@parameterized({
1896+
'': ([],),
1897+
'pthread': (['-pthread', '-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME'],),
1898+
})
1899+
def test_main_thread_em_asm(self, args):
18961900
src = read_file(test_file('core/test_em_asm_2.cpp'))
18971901
create_file('test.cpp', src.replace('EM_ASM', 'MAIN_THREAD_EM_ASM'))
18981902

18991903
expected_result = read_file(test_file('core/test_em_asm_2.out'))
19001904
create_file('test.out', expected_result.replace('EM_ASM', 'MAIN_THREAD_EM_ASM'))
19011905

1902-
self.do_run_in_out_file_test('test.cpp')
1903-
self.do_run_in_out_file_test('test.cpp', force_c=True)
1906+
self.do_run_in_out_file_test('test.cpp', emcc_args=args)
1907+
self.do_run_in_out_file_test('test.cpp', emcc_args=args, force_c=True)
19041908

19051909
@needs_dylink
19061910
@parameterized({

0 commit comments

Comments
 (0)