Skip to content

Use direct table access over dynCall in invoke_xx functions #11979

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

Merged
merged 2 commits into from
Aug 25, 2020
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 src/preamble.js
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@ function createWasm() {
// then exported.
// TODO: do not create a Memory earlier in JS
wasmMemory = exports['memory'];
wasmTable = exports['__indirect_function_table'];
updateGlobalBufferAndViews(wasmMemory.buffer);
#if ASSERTIONS
writeStackCookie();
Expand Down
35 changes: 20 additions & 15 deletions src/support.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ function createInvokeFunction(sig) {
return function() {
var sp = stackSave();
try {
return Module['dynCall_' + sig].apply(null, arguments);
return dynCall(sig, arguments[0], Array.prototype.slice.call(arguments, 1));
} catch(e) {
stackRestore(sp);
if (e !== e+0 && e !== 'longjmp') throw e;
Expand Down Expand Up @@ -602,24 +602,29 @@ function makeBigInt(low, high, unsigned) {

/** @param {Array=} args */
function dynCall(sig, ptr, args) {
if (args && args.length) {
#if ASSERTIONS
// j (64-bit integer) must be passed in as two numbers [low 32, high 32].
assert(args.length === sig.substring(1).replace(/j/g, '--').length);
#endif
#if ASSERTIONS
assert(('dynCall_' + sig) in Module, 'bad function pointer type - no table for sig \'' + sig + '\'');
#endif
return Module['dynCall_' + sig].apply(null, [ptr].concat(args));
} else {
#if ASSERTIONS
assert(sig.length == 1);
#endif
#if !WASM_BIGINT
// Without WASM_BIGINT support we cannot directly call function with i64 as
// part of their signature, so we rely on the dynCall functions generated by
// wasm-emscripten-finalize
if (sig.indexOf('j') != -1) {
#if ASSERTIONS
assert(('dynCall_' + sig) in Module, 'bad function pointer type - no table for sig \'' + sig + '\'');
if (args && args.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (args && args.length) {
if (args) {

an empty array is truthy in JS (but not in python)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code is pre-existing, plus it seems like we need both check here.. we want to check for args being defined and having a length.

If we remove the first check we will get Cannot read property 'length' of undefined.. if we remove the second check as you suggest when the empty array will go down the wrong path (since as you say its truthy).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to land this as is for the above reasons.. if you think i'm wrong we can fix after.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, I was mistaken about args.length, lgtm.

// j (64-bit integer) must be passed in as two numbers [low 32, high 32].
assert(args.length === sig.substring(1).replace(/j/g, '--').length);
} else {
assert(sig.length == 1);
}
#endif
return Module['dynCall_' + sig].call(null, ptr);
if (args && args.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (args && args.length) {
if (args) {

return Module['dynCall_' + sig].apply(null, [ptr].concat(args));
} else {
return Module['dynCall_' + sig].call(null, ptr);
}
}
#endif

return wasmTable.get(ptr).apply(null, args)
}

var tempRet0 = 0;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
__indirect_function_table
_start
memory
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
__indirect_function_table
_start
memory
1 change: 1 addition & 0 deletions tests/other/metadce/mem_O3_STANDALONE_WASM.exports
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
__indirect_function_table
_start
memory
1 change: 1 addition & 0 deletions tests/other/metadce/mem_no_argv_O3_STANDALONE_WASM.exports
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
__indirect_function_table
_start
memory
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
__indirect_function_table
_start
memory
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
__indirect_function_table
_initialize
foo
memory
2 changes: 1 addition & 1 deletion tests/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -7111,7 +7111,7 @@ def test_metadce_mem(self, filename, *args):
[], [], 128), # noqa
# argc/argv support code etc. is in the wasm
'O3_standalone': ('libcxxabi_message.cpp', ['-O3', '-s', 'STANDALONE_WASM'],
[], [], 198), # noqa
[], [], 242), # noqa
})
def test_metadce_libcxxabi_message(self, filename, *args):
self.run_metadce_test(filename, *args)
Expand Down
13 changes: 12 additions & 1 deletion tools/building.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,8 @@ def lld_flags_for_executable(external_symbol_list):
if not Settings.STANDALONE_WASM:
cmd.append('--import-memory')
cmd.append('--import-table')
else:
cmd.append('--export-table')

if Settings.USE_PTHREADS:
cmd.append('--shared-memory')
Expand Down Expand Up @@ -1136,7 +1138,10 @@ def add_to_path(dirname):
logger.error(proc.stderr) # print list of errors (possibly long wall of text if input was minified)

# Exit and print final hint to get clearer output
exit_with_error('closure compiler failed (rc: %d.%s)', proc.returncode, '' if pretty else ' the error message may be clearer with -g1 and EMCC_DEBUG=2 set')
msg = 'closure compiler failed (rc: %d): %s' % (proc.returncode, shared.shlex_join(cmd))
if not pretty:
msg += ' the error message may be clearer with -g1 and EMCC_DEBUG=2 set'
exit_with_error(msg)

if len(proc.stderr.strip()) > 0 and Settings.CLOSURE_WARNINGS != 'quiet':
# print list of warnings (possibly long wall of text if input was minified)
Expand Down Expand Up @@ -1227,6 +1232,12 @@ def metadce(js_file, wasm_file, minify_whitespace, debug_info):
'reaches': [],
'root': True
})
graph.append({
'export': '__indirect_function_table',
'name': 'emcc$export$__indirect_function_table',
'reaches': [],
'root': True
})
# fix wasi imports TODO: support wasm stable with an option?
WASI_IMPORTS = set([
'environ_get',
Expand Down
29 changes: 18 additions & 11 deletions tools/shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -1209,28 +1209,34 @@ def make_jscall(sig):
return ret

@staticmethod
def make_dynCall(sig):
# Optimize dynCall accesses in the case when not building with dynamic
# linking enabled.
if not Settings.MAIN_MODULE and not Settings.SIDE_MODULE:
return 'dynCall_' + sig
def make_dynCall(sig, args):
# wasm2c and asyncify are not yet compatible with direct wasm table calls
if Settings.ASYNCIFY or Settings.WASM2C or not JS.is_legal_sig(sig):
args = ','.join(args)
if not Settings.MAIN_MODULE and not Settings.SIDE_MODULE:
# Optimize dynCall accesses in the case when not building with dynamic
# linking enabled.
return 'dynCall_%s(%s)' % (sig, args)
else:
return 'Module["dynCall_%s"](%s)' % (sig, args)
else:
return 'Module["dynCall_' + sig + '"]'
return 'wasmTable.get(%s)(%s)' % (args[0], ','.join(args[1:]))

@staticmethod
def make_invoke(sig, named=True):
legal_sig = JS.legalize_sig(sig) # TODO: do this in extcall, jscall?
args = ','.join(['a' + str(i) for i in range(1, len(legal_sig))])
args = 'index' + (',' if args else '') + args
args = ['index'] + ['a' + str(i) for i in range(1, len(legal_sig))]
ret = 'return ' if sig[0] != 'v' else ''
body = '%s%s(%s);' % (ret, JS.make_dynCall(sig), args)
body = '%s%s;' % (ret, JS.make_dynCall(sig, args))
# C++ exceptions are numbers, and longjmp is a string 'longjmp'
if Settings.SUPPORT_LONGJMP:
rethrow = "if (e !== e+0 && e !== 'longjmp') throw e;"
else:
rethrow = "if (e !== e+0) throw e;"

ret = '''function%s(%s) {
name = (' invoke_' + sig) if named else ''
ret = '''\
function%s(%s) {
var sp = stackSave();
try {
%s
Expand All @@ -1239,7 +1245,8 @@ def make_invoke(sig, named=True):
%s
_setThrew(1, 0);
}
}''' % ((' invoke_' + sig) if named else '', args, body, rethrow)
}''' % (name, ','.join(args), body, rethrow)

return ret

@staticmethod
Expand Down